prepareUserLogging.sql is not organized/documented well enough (WM)

[smurthys]

prepareUserLogging.sqlhas the following issues:

• [x] Current user's privilege is checked too late (W)
• [x] Some comments are placed after the code to which the comment applies (e.g., Set the log file name...) (W)
• [x] Could place one common set of comments before the ALTER SYSTEM statements, at least for the first two (W)
• [x] Missing comment for use of pg_reload_conf() (also could perhaps be PERFORM instead of SELECT) (M)
• [x] DROP EVENT TRIGGER statements are not placed with corresponding CREATE EVENT TRIGGER statements (W)
• [x] Consider replacing $1 with actual parameter name for clarity: I believe the parameter could be used in this context. (W)
• [x] The prologue comment for function updateStudentActivity does not provide sufficient details (M)

[wildtayne]

I agree with most of these issues, however I have additional comments about two:

1. The ALTER SYSTEM statements can't be placed inside a transaction block, so placing the superuser check before them has no affect. Additionally, ALTER SYSTEM can only be run as a superuser, so they will fail anyway. I can add an additional comment to further clarify this. I could add the pg_reload_conf to the transaction, however.

2. I will definitely add a comment about pg_reload_conf. PERFORM is a plpgsql statement, so it would have to be placed inside a do block. I can add this if you think it would improve the readability of the code.

3. I can move the DROP statements for the event triggers above classdb.updateStudentActivity, however placing them below this cause the DROP FUNCTION statement for classdb.updateStudentActivity to fail when rerunning the script, because the event trigger depends on that function.
   An alternate solution would be to drop the function with cascade, and place the drop event trigger statements next to the create event trigger statements. I feel like this is less clear, however. Either way, I will add comments detailing the solution.

[smurthys]

I am not sure why testing the current user's privilege needs to be inside a transaction: couldn't that test simply be move to the beginning of the script?

We want to add test even in all scripts though they are not required in order to make the script fail early and to get a clear error message.

PERFORM is always better if a function's return value is not being used, but SELECT is acceptable if it simplifies code.

Does the DROP FUNCTION really cause failure or just raise notices on re-run? In either case, thank you for letting me know this problem. (I wonder if this problem exists in prepareClassDB.sql as well.)

An alternative solution, especially if we have working uninstall scripts, perhaps CREATE OR REPLACE is the better choice. I don't recall a discussion of this concern when we chose to use DROP instead of REPLACE, but then we also did not have uninstall scripts.

[wildtayne]

The exception thrown by the superuser check only causes the current transaction to fail. If there is no explicit transaction in progress, the client is able to send the next statement even if a previous exception was thrown. So, the superuser check can only cause statements in the same transaction to fail.

The DROP FUNCTION is only an issue here because of the event triggers that use it. When you re-run prepareUserLogging.sql, the two event triggers that use it already exist. So, when you try and drop the function before the event triggers, it fails due to that dependency. I don't think there are any other dependencies like this in ClassDB. It might make more sense to use CREATE OR REPLACE here because of that, however.

[smurthys]

That trifecta of limitations is quite interesting: must be superuser, cannot abort a batch, cannot run something in a transaction. I would not have imagined that.

Yes, please add comments explaining this clearly. In general, we need to add detailed comments wherever we use vendor-specific syntax or features, because even the more knowledgeable people won't know all the details (and there is no need to make people work for it.)

Yes, to using CREATE OR REPLACE here for the functions.

[wildtayne]

Fixed by #79