Update monitor functions

[wildtayne]

This PR updates the DDL monitor and log import functions to support the new DDLActivity and ConnectionActivity tables. I have not tested this functionality yet, but I am creating the PR so others can begin to review it.

Basically, both functions now perform INSERT statements instead of UPDATE statements, which has greatly simplified their code.

[smurthys]

Thanks @srrollo for getting this change started. I will share my observations over the next few comments, probably organized by file.

The following observations are about addDDLMonitors.sql:

• L25 and L80: The keyword SELECT is not required
• I wonder if function updateDDLActivity should be renamed to logDDLActivity
• L55-L60: Use plpgsql's form of SELECT INTO to fill a variable with the result of a scalar-value sub-query. The current form used is meant to populate a table
• L53-L66: It might be possible to replace with a CASE expression, but that is not important
• L76: Indent AND such that it is left-aligned with the first part of the predicate. Better yet, move up the end-of-line comment and place the entire predicate on one line
• L77-L84: The query can occupy fewer lines
• L88: VOLATILE is default: no need to specify
• Use ClassDB instead of classdb
• Need to add ownership and grants as appropriate

Also, I recommend adding functions enableDDLActivityLogging() and disableDDLActivityLogging() to respectively enable and disable the triggers. Instructors and DB managers should be able to execute these functions.

[wildtayne]

Thanks @smurthys for the review. The commits up to 43b20e1 address the issues raised. I added the enableDDLActivityLogging() and disableDDLActivityLogging() functions as well. One thing to note about them is that they remain owned by the creating user, and not ClassDB. This is because only superusers may CREATE/DROP event triggers, and ClassDB is not a superuser. However, we are asserting at the beginning of the script that the user running it is.

[smurthys]

Quick thoughts on the updated addDDLMonitors.sql:

• L53: Too much indentation
• L67: Can use a blank line after
• L57-L58 and L78-L80: In general, comment/blank lines in the middle of a query tend to decrease readability. The recommendation is to place a block comment prior to the query or code chunk.
• L88-L89: Trivial, but the order of statements is inconsistent with the order of such statements elsewhere in the code base.
• Enabling and disabling triggers should just use ALTER EVENT TRIGGER.
  • The earlier CREATE EVENT TRIGGER is still required.
  • Enabling and disabling should check if trigger exists and raise exception if the trigger does not exist.
• For script idempotence, CREATE EVENT TRIGGER should be guarded by a block to create the trigger only if it does not exist (because there is no IF NOT EXISTS option)

[wildtayne]

7109499 addresses the points brought up in this thread. Currently, it is assuming that ClassDB.isTriggerDefined() will be available, but this can easily be changed to match whatever we end up implementing. I've also added the files disableServerLogging.sql and addConnectionLogging.psql based on today's discussion.

[smurthys]

On function isTriggerDefined, please see my recent comment on another PR.

[wildtayne]

The latest commits add a unit test for the DDL event triggers, and a significant improvement to importLog().

• The startDate parameter for importLog will now always be used as the start date for log importing unless it is null.
• The 'best-guess' start date is now generated by getting the latest timestamp from ConnectionActivity, and converting it to the server timezone. This should be accurate in almost all circumstances, since the dates on log files should match up with the server's time zone.
• The final fallback start date is simply the current date.
• The insert statement now converts both the latest timestamp and candidate insert timestamp to UTC before comparing them.

[smurthys]

@srrollo 0955c84 actually "Added terrible unit test" ? 😕

[wildtayne]

Yeah, I was pretty frustrated with the unit test last night 😩. The latest update is much better. The two unit tests are now somewhat comprehensive:

• testAddDDLMonitors.sql creates several ClassDB roles of each type, and uses SET SESSION AUTHORIZATION to execute DDL statements as each

• Additionally, it creates a non-ClassDB user, and executes DDL statements using it

• The DDL activity log is checked for the correct number of entries, correct object names, and correct operation names. Additionally, the presence of each ClassDB user, and absence of the non-ClassDB user is checked

• Similarly, testAddLogMgmt.psql creates several ClassDB roles, and one non-ClassDB role

• Using psql's \c command, the connection is switched to each user, causing a log entry

• This requires the user to manually enter the password for each user (annoying), but each user has a password of ' ' (less annoying)

• The test checks if the number of new connections in the log table matches the expected number

I am having one more issue with testAddLogMgmt.psql - I can't get my non-ClassDB user to login to the database (I get password authentication failed). The code to create the user is this:

CREATE USER conNonClassDB ENCRYPTED PASSWORD ' ';
EXECUTE FORMAT('GRANT CONNECT ON DATABASE %I TO conNonClassDB', CURRENT_DATABASE());

[wildtayne]

The latest commits also remove isTriggerDefined(), as per our discussion. I've removed the creation of the non-ClassDB user in the test for now, as it is not strictly necessary, and can be addressed later (after M2 if necessary).

[smurthys]

Quickly: Functions enableDDLActivityLogging and disableDDLActivityLogging are missing owner assignment.

FYI: I have a meeting and other work related to a different project starting now to EOD.

[wildtayne]

enableDDLActivityLogging and disableDDLActivityLogging must be owned by a superuser, in this case the user executing addDDLMonitors.sql, since all event trigger DDL operations can only be performed by superusers.

I've added some extra comments to better document this requirement.

[smurthys]

Thanks @srrollo for the reminder about event triggers needing super user.

[smurthys]

FYI, I'm reviewing the files right now. EDIT: I'll share my observations over multiple comments.

[smurthys]

Here are some observations:

addConnectionLogging.psql:

• L1: Incorrect file name
• Comments should probably say the script should be run from psql. prepareDB.sql provides a model
• Given the comment on L14-L15 in enableServerLogging.sql, I feel this script should not include enableServerLogging.sql.
  • If enableServerLogging.sql is excluded from this script, the script itself will not be needed.

enableServerLogging.sql

• Given the comment starting on L17, I wonder if this file should have a .psql extension

EDIT: Same observations in disableServerLogging.sql as for enableServerLogging.sql. Plus:

• L1: file name is incorrect
• Comments on L13-L15 in enableServerLogging.sql apply even here

[smurthys]

addLogMgmt.sql is really streamlined. Excellent work @srrollo.

Some observations:

• L39: comment says the table temporary, but the table is not
• A question I have long had: I believe table postgresLog should be temporary, especially because that table is truncated at the end of function importLog. Should probably not make this change right now, but rather add an issue and address in the next release.
• Should probably have two blank lines before the start of functions and table definitions
• L47 and L60 can benefit from end-of-line comments that these are the fields of interest for connection activity. (As of now that is all we use this table for, though it could be used for other things.)
• L94: I believe ClassDB.ChangeTimeZone should be called on the SELECT MAX(...)...
• L94-L99: It is better to delay executing the SELECT query until it is really required, because table ConnectionActivity can get rather large over a period of time. For example, L99 could probably be written as an IF-ELSE block. Alternatively, lastConDate in the COALESCE expression could be replaced with the SELECT query, but that won't be very readable.
• Should we log connection_from (I assume indicates a "machine") and application_name to ConnectionActivity? Should probably not make this change right now, but rather add an issue and address in the next release
• L114 should be indented inside L113; L117-119 should be indented inside L116
• L117 is too long and requires comments
  • If end-of-line comments don't work, please add comments prior to the start of the entire query
• L118: End-of-line comment should be more direct such as pick up only connection-related entries
• L120: The need for ORDER BY is unclear.
• L132-L134 fits on one line
• L136-L138 fits on two lines
• L136: I think execution should be granted to ClassDB role as well.
  • This should in general be the case for functions that cannot be owned by ClassDB, but are executable by ClassDB roles.

[smurthys]

For now I am unable to review the test scripts due to other urgent work. I think @afig's review on that suffices.

[wildtayne]

Thanks @smurthys for the thorough review. I've made the following changes: addConnectionLogging.psql:

• I agree with your assessment, and ended up removing this. I think our original rationale for having this script was the same - it combines server-level and DB-level operations.

enableServerLogging.sql/disableServerLogging.sql

• Fixed filenames
• Change extensions to psql. While these files do not require psql, most other clients I have used do not execute them correctly. Since we know psql works, I think the psql extension is warranted. I've also added a comment explicitly saying we recommend using psql for this script.

addDDLMonitors.sql

• Not in the comments, but I removed the DO block around the CREATE EVENT TRIGGER statements, since it is no longer needed

addLogMgmt.sql

• Added clarifying comments where suggests
• Refactored importLog():
  • Added two separate variables: lastConTimestampUTC and lastConDateLocal
  • Since both of these values are used multiple times, this removes the need to repeat the aggregate sub-query on ClassDB.ConnectionActivity in the INSERT query
  • Since LastConTimestampUTC will always be needed in the INSERT query, I've elected to always compute it instead of only computing it if no startDate is supplied

All files:

• Made the formatting improvements suggested
• Granted EXECUTE on all superuser-owned functions to ClassDB
• All tests still pass 😄

[wildtayne]

I've added changeTimeZone() to addHelpers.sql in this PR. Originally, it was going to be added in #152, however it is now required by importLog(), and it looks like this PR will be merged before #152.

[smurthys]

All good changes @srrollo:

In addDDLMonitors.sql:

• In function logDDLActivity, it seems the entire functionality can be guarded by IF ClassDB.isUser(SESSION_USER::ClassDB.IDNameDomain)

In addLogMgmt.sql:

• Comment on L99 seems misplaced. Perhaps it should be merged with the comment in L95 with a statement that the timestamp of the most recent recorded connection activity is being determined in local time zone.
• L104: Should we use the minimum of startDate and lastConDateLocal so connection entries aren't skipped over?
• L112: Comment adds no real value: perhaps say something like "import entries from the day's server log"
• L122-L123 should also be indented inside L120, lining up different parts of the SELECT query that feed into the INSERT query.

In *ServerLogging.psql, file name is incorrect on L1 in both files.

On file names:

• I feel addDDLMonitors.sql should be renamed to something like addDDLActivityLogging.sql because there will be a separate DDL trigger (in progress) to monitor schema drop etc. as part of RoleBase mgmt.
• Likewise, addLogMgmt.sql should be renamed to addConnectionActivityLogging.sql.
• disableServerLogging.psql should be renamed to disableConnectionLogging.psql because the script only disables connection logging. In solidarity, and to avoid confusion, the enable counterpart should probably also be renamed similarly though that script enables logging as well (however, the scripts intent is to enable connection logging)

[smurthys]

Also, perhaps the function importLog should be renamed to importConnectionActivity.

[wildtayne]

Should addLogMgmt.sql maybe be named addConnectionActivityMgmt.sql instead? I feel like addConnectionActivityLogging.sql doesn't convey the intent of the script, since it only adds the management functions, and doesn't directly influence the logging system.

I'm also not sure about taking the minimum of startDate and lastConDateLocal. startDate is intended to override the date check if users want to skip days. I think the documentation also states that except for the first run, you should generally just use importConnectionLog().

[wildtayne]

I just pushed the commits addressing the recent comments.

[smurthys]

All changes look good @srrollo. I have only one observation on addHelpers.sql: It is not necessary to revoke permission from anyone to execute function changeTimeZone. Just granting ownership to ClassDB suffices.

[wildtayne]

Thanks @afig and @smurthys for the reviews. I've removed the unnecessary REVOKE and fixed the errors in the comments.