Add ddl session logging

DASSL / ClassDB

An open-source system to let students experiment with relational data

https://dassl.github.io/ClassDB/

Other

7 stars 2 forks source link

Add ddl session logging #240

Closed wildtayne closed 6 years ago

wildtayne commented 6 years ago

This commit adds logging of SessionID in the DDL activity trigger and updates the frequent user views for compatibility. Additionally, a helper function ClassDB.getSessionID() was added. Note that this branch contains all commits from #235 due to the nature of the changes. As such, this will be blocked by #235.

Closes #237, Closes #243

smurthys commented 6 years ago

Thanks @srrollo for initiating these changes. They look quite good.

Please consider the following observations.

addHelpersCore.sql:

Please add a link and comments about the origin of the code in function getSessionID. The link and suitable comment text is available in the description of Issue #237

addUserMgmtCore.sql:

As with the discussion on table ConnectionActivity in PR #235, it is better to re-create table DDLActivity if it exists and is empty.

addFrequentViewsReco.sql:

In the table returned by function getUserDDLActivity, column SessionID should be immediately after StatementStartedAt for compatibility with function getUserActivity. (I wonder if we should also return ApplicationName, but then we need to think about join efficiency: so, not now.)

wildtayne commented 6 years ago

Once we decide on a solution to the upgrade issues pointed out in #235, I will update this branch to use the same solution.

wildtayne commented 6 years ago

I pushed a candidate ER Schema to this branch that reflects the changes to ClassDB.ConnectionActivity and ClassDB.DDLActivity. I am wondering, however, if ClassDB.ConnectionActivity is no longer a weak entity due to the new PK (or index).

EDIT: I just pushed a few more commits that address the comments by @smurthys and rewrite the DDL activity logging tests to be more in line with the other newer test scripts.

smurthys commented 6 years ago

I am glad @srrollo spotted and logged issue #243. The fix also seems adequate. I think a brief end-of-line comment in code on L680 in addHelpers.sql is helpful so future maintainers know why this function must be owned by a superuser.

The changes to table DDLActivity in addUserMgmtCore.sql also look good. I do observe the following things:

L59, L89, L97: Column SessionID is missing CHECK constraint to prevent empty string
L112: I see one extra indentation space

wildtayne commented 6 years ago

I have added the CHECK constraint to the SessionID columns.

wildtayne commented 6 years ago

I just added the latest ER-Schema, as discussed

wildtayne commented 6 years ago

I just pulled in the new changes from dev, so the diff should be clean now.