Add disconnection logging

[wildtayne]

This PR adds disconnection logging, as well as logging of SessionID and ApplicationName. It also updates some core objects that are broken by changes to ClassDB.ConnectionActivity

• ClassDB.importConnectionLog() adds rows with ActivityType D for disconnections
• ClassDB.ConnectionActivity has had attributes added to hold the new log information
• Upgrade code for ClassDB.ConnectionActivity has been added
• ClassDB.User has been updated for compatibility with updated ClassDB.ConnectionActivity (but it does not display any new info)
• A new helper ClassDB.isColumnDefined() is added, because there is no way to conditionally rename a column (need to check if it exists first)
• The tests have been updated to test disconnection logging
• Views in addFrequentViewsReco.sql have been updated to work with disconnection logging
• Currently, the various getActivity functions and views return rows related to disconnection logging. Activity summaries currently don't return any info related to disconnections because they draw from ClassDB.User which does not expose this information.

This PR should not be reviewed yet; I am uploading it to get some initial feedback on these solutions. Additionally, this PR will show some additional changes until #214 is merged, because it is branched from that.

I would like to get feedback on the new schema for ClassDB.ConnectionActivity. When we are happy with it, I will update the conceptual schema and add it to this PR.

I plan on updating the activity views in a separate PR. Currently, most views in addFrequentViewsReco.sql will be broken after merging this PR. I accidentally added the views fixes to this branch, so I guess they will live here.

Closes: #163, #206

[smurthys]

Thanks @srrollo for getting these changes started. I see that quite a bit of cross-script coordination is needed to add the desired functionality.

Eyeballing, I have the following observations (will arrive in multiple comments).

addClassDBRolesViewsCore.sql:

• L78: Should be ActivityAtUTC (character case)

addHelpersCore.sql:

• I like the new helper function isColumnDefined.
• L470: Consider using SELECT column_name instead of SELECT *
• Add RETURNS NULL ON NULL INPUT to the new helper function

addUserMgmtCore.sql:

• L71: It is better to say The following table alterations are needed...
• L88: Add constraint NOT NULL to the new column ActivityType
• L91-97: Need comments explaining the rationale from an application point of view: we want to make sure session id is never NULL, but rows may already exist
• L100: I apologize I don't recall all the discussion, but just want to be sure ClassDB.IDNameDomain is the right type for column ApplicationName
• L104: The comment about there being no viable primary key seems incorrect, because pg doc says session_id and session_line_num form PK, and we import only one line for every session with ActivityType being different. So it seems fields SessionID and ActivityType together do form the PK. Is there any chance we import multiple lines? If not, should the import process be limited to just line 1?
• L112-L113: These lines can be spliced
• L115: Add an end-of-line comment that this value is expected to be NULL for connection entries
• I recommend adding a comment prior to CREATE TABLE with a brief list of changes made from 2.0 to 2.1
  • one rename; three additions

[smurthys]

Continuing my observations.

I like to reiterate that these changes are really well coordinated. The number of things that need attention are surprisingly few for the nature of the changes. Thanks @srrollo.

addConnectionActivityLoggingReco.sql:

• L116: Missing log parameter log_disconnections in the IN list
• L224
• I wonder if it is better if function importConnectionLog returns numConnections and numDisconnections in addition to (or instead of) numEntries

addFrequentViewsReco.sql:

• I wonder if the field ActivityType returned by functions and views be the words Connection and Disconnection instead of single letters. I think this because these functions and views are meant for end-user consumption and are likely to be used in reports.
• I recommend all functions and views that return ActivityType use the type VARCHAR (no size) to eliminate the need to break API compatibility if we decide to change the exact value returned in future.
• L390: Type VARCHAR(10) cannot hold the value Disconnection. I recommend changing the type to VARCHAR (no size) to future proof.
• L396: I recommend changing 'DDL' to 'DDL Query'
• L400-L406: The two parts of the UNION query can be just one query over getUserConnectionActivity, using a CASE expression in SELECT
• Reviewing function getUserActivity, I believe table DDLActivity should include session id. See new issue #237

I have not reviewed the test scripts. I will review them in the next cycle when hopefully PR #214 will already be merged and we can easily spot the new content.

[wildtayne]

Thanks for the great feedback @smurthys. I agree with and will implement most of these changes. Some thoughts:

• I can't easily link on mobile, but @afig provided a link to documentation about using IDNameDomain for ApplicationName.
• I like the idea of storing SeasionID for DDL activity, but I think we may have some issues with getting SessionID, I will comment in #237
• I agree that (SessionID, ActivityType) is a good CK for ConnectionActivity. It is actually a bug if there are any duplicate rows with these attributes matching. I'm not sure we can add the constraint to an existing table, however, due to the existing rows not having unique SessionID

[wildtayne]

Sidenote: it looks like I may need toerge dev into this locally to correct the diff - it still shows changes for #214 as well

[smurthys]

Good point @srrollo about the trouble with making (SessionID, ActivityType) the PK in table ConnectionActivity. This issue can be addressed using a partial index to enforce uniqueness only on new rows. The following query should do:

CREATE UNIQUE INDEX IF NOT EXISTS idx_SessionID_ActivityType
ON ClassDB.ConnectionActivity(SessionID,ActivityType)
WHERE SessionID <> '00000000.00000000'

Here is the pseudo-code I recommend to upgrade table ConnectionActivity from 2.0 to 2.1:

• If ConnectionActivity is not empty
  • Alter table as is done so far in this PR (I believe the ALTER statements used so far are idempotent)
  • Create the partial index as shown above if the table does not already have a PK (to make the script idempotent)
• Else If ConnectionActivity is empty, drop the table
• Create ConnectionActivity if it does not exist as is done so far in this PR. In the CREATE query make (SessionID, ActivityType) the PK

[wildtayne]

I have not yet pushed any new changes to this branch, however I think I have successfully merged in external changes. The diff should be correct now.

[wildtayne]

OK, I've pushed some new commits that improve disconnection logging based on the feedback from @smurthys. I've made all changes pointed out. A summary of changes that deviated a bit: addUserMgmtCore.sql

• Added the new conditional upgrade/creation of ClassDB.ConnectionActivity
• Note that currently it only checks if ClassDB.ConnectionActivity exists. I think then checking if it is empty requires a nested IF. I tried to do:

  IF ClassDB.isColumnDefined('ClassDB', 'ConnectionActivity', 'UserName')
  AND EXISTS (SELECT * FROM ClassDB.ConnectionActivity)

• But this throws an error that it can't select from a non-existent table.

addFrequentViews.sql

• I added the CASE changing C/D to Connection/Disconnection in getUserConnectionActivity(), since that will cascade to all functions/views that get connection activity.

[smurthys]

Thanks @srrollo for the revision. These changes add large value to the product's feature and quality.

Here are some observations in addUserMgmtCore.sql. I will add another comment if I have observations to share in other scripts:

• L83: Helpful to explicitly say that the presence/absence of column UserName is actually used to test if table ConnectionActivity exists
• L84-L144: I see a few issues with the approach. Our goal should be to keep/make the table schema identical to version 2.1 in as many situations as possible so later maintenance and future upgrades are easier. This can be achieved by dropping the current table if it is empty and then running CREATE OR REPLACE TABLE. Our goals should be:
  • Separate patching code from normal code and force execution of normal code as much as possible
  • Favor primary key over unique partial index
  • Avoid default value for SessionID that is meant to patch existing rows, but ends up permitting an INSERT that omits value for SessionID, which in turn goes unnoticed due to the partial index

I suggest the following code structure to meet the aforementioned goals. A downside with this approach is that dropping an empty table and re-creating it gives the table a new oid, but that is not an issue because this is an installation script: not a patch script. Also, thus far, our assumption is that all relevant scripts will be (re)run in any installation, and that is possible due to the idempotent nature of our scripts.

IF table exists THEN
  IF table empty THEN
    DROP TABLE
  ELSE
    upgrade 2.0 to 2.1-- a temp function already created
  END IF
END IF

CREATE OR REPLACE TABLE...

ALTER ownership
REVOKE from public
GRANT SELECT ...

[smurthys]

I have the following observations in addConnectionActivityLoggingReco.sql:

• L103: Please remove the word Returns and just say #of connections imported for the log date
• L104: Add an end-of-line comment as in L103, except say disconnections

[wildtayne]

Thanks for the feedback on the upgrade code. I've updated it to reflect the suggested layout. The one change I had to make was dropping the user views ClassDB.Student, ClassDB.Instructor, etc. because they all depend on ClassDB.ConnectionActivity. This is OK if the user is running one of the addAll scripts, but I suppose could cause an issue if running addUserMgmtCore.sql on its own.

I've added a WARNING about dropping the user views, however I'm not sure if this is sufficient, since it gets buried when running an install all script (especially addAllToDB.psql).

[smurthys]

Hmm. I didn't think about dependencies. I am thinking about it now and will comment again in a few minutes.

[smurthys]

Thanks again to @srrollo for mentioning the cascading effect of dropping ConnectionActivity during upgrade.

I have thought about this issue and feel this is a good opportunity to learn and practice dealing with likely upgrade issues. Because I caused a bit of extra effort for @srrollo, I took the responsibility to change the upgrade to make it provide the best result for the schema/data that exists when the script is run.

I just pushed the following changes. I have verified these changes produce the desired result with this branch:

• Reverted @srrollo's commits f6ea290, 147d691, and ccdda1d
• Retained @srrollo's commit 7ce4075
• Added my own commit 5bb2f99

Please review.

[wildtayne]

Thanks for the changes @smurthys. I think think this makes each possible version of ClassDB.ConnectionActivity as close as possible.

[smurthys]

Just pushed spelling corrections and some minor edits to comments in addUserMgmtCore.sql.

BTW, this is the first time I used ALTER TABLE ... ADD PRIMARY KEY. It is really neat how a unique index just becomes a PK. Watching the before and after changes in pgAdmin is quite satisfying. 🤓

[wildtayne]

Thanks for the catch @afig , I added the client_min_messages supression