DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Guard uses of ALTER SYSTEM for pg9.3 compatability #265

Closed afig closed 6 years ago

afig commented 6 years ago

This PR adds a guard that prevents any statements located in enableconnectionLoggingReco.psql and disableConnectionLoggingReco.psql from being run if the Postgres server version is < 9.4. This guard is needed because the ALTER SYSTEM statements were not available prior to pg9.4, and these settings need to instead be manually set in the postgresql.conf file. Also note that because of how the guard is implemented, it is no longer possible to use the addAllToServer.psql script to add everything to the server [when using Postgres 9.3]* (which is okay since there is only one other server file apart from these two).

*Edited to mention that the issue with addAllToServer.psql only applies to 9.3

As mentioned in the script comments, ClassDB's server comparers are not accessible. An equivalent call to one of these functions is included in a comment to make locating the backwards-comparable alternative easier.

Currently, I believe it makes more sense to include details about manually setting these server parameters in ClassDB's Setup page. However, please feel free to suggest an alternative location.

Closes: #228

smurthys commented 6 years ago

Thanks @afig for the contribution. These changes are inline with the team's decision on how to resolve this issue.

I have two suggestions:

afig commented 6 years ago

Thanks for the suggestions @smurthys. I agree with both parts and have pushed corresponding changes.

smurthys commented 6 years ago

Nice changes. I recommend moving the meta-command out of assertAlterSystemAvailability.sql into each of the calling script so that:

Edit: Actually, on the last bullet, I just figured out we can have the customization and the assertion script still be SQL (if that is a goal). Nothing to do with this PR though.

smurthys commented 6 years ago

@afig Would you please add a brief note to the Activity Logging docs page about altering connection settings in pre9.4 pg?

afig commented 6 years ago

Thanks again for the suggestion. The use of a meta-command in a .sql script should have sounded some alarms before I pushed the changes.

I had added a note to the Setup page about altering the connection settings. However, I think it makes more sense to have it under the Activity Logging page as suggested and add a link to it on the Setup page.

smurthys commented 6 years ago

Thanks @afig for the changes. I like the note on the setup page and the detailed note on the logging page. (FYI, I tinkered with those pages for consistency and clarity.)

I like clarification on two things: Have you run addAllToServer.psql on pg9.3 and verified the script stops after showing the exception.

Related, would you please expand on the following text in the PR description? Is it saying addAllToServer.psql can no longer be used with any pg version? I hope not.

Also note that because of how the guard is implemented, it is no longer possible to use the addAllToServer.psql script to add everything to the server (which is okay since there is only one other server file apart from these two).

afig commented 6 years ago

On my installation with Postgres server version 9.3.24, both psql client versions 9.3.24 and 10.5 correctly show the exception and stop when addAllToServer.psql is run. Running either client version with Postgres server 9.4.19 results in the script completing successfully (with an unrelated patch to workaround issue #229).

The PR description should have been clearer. addAllToServer.psql can no longer be used with Postgres version 9.3 or below, but it can still be used with versions 9.4 or higher.

smurthys commented 6 years ago

Thanks @afig for the clarification. I apologize but I am now a bit confused by the first paragraph in your recent comment:

On my installation with Postgres server version 9.3.24, both psql client versions 9.3.24 and 10.5 correctly show the exception and stop when addAllToServer.psql is run.

Do you really mean the changes cause an exception and stop in pg10.5? They should not.

afig commented 6 years ago

That was referring to psql client versions 9.3.24 and 10.5 both connecting to a Postgres 9.3.24 server. (Using a newer client to connect to an old server)

smurthys commented 6 years ago

Ah, psql version 10.5. Thanks I notice it now. Sorry I insisted on your reply.

afig commented 6 years ago

Thank you for reviews everyone.