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

Guard GRANT to CURRENT_USER in pg9.4 or earlier #270

Closed smurthys closed 6 years ago

smurthys commented 6 years ago

This PR changes two uses of GRANT query involving CURRENT_USER:

Fixes #229

KevinKelly25 commented 6 years ago

Thank you for the updates @smurthys . I really like how easy it was to update testRoleBaseMgmt.sql and it really shows how useful the helper functions can be.

I do see two issues in intializeDBCore.sql:

1) on ln 95 the value should be 90500 as it works from 9.5 and up. This is seen here:

   IF 90500 >= (SELECT setting::integer FROM pg_catalog.pg_settings
                WHERE name = 'server_version_num'

2) On 10.4 this works but on 9.4 the second part of the else statement on ln 101 fails the syntax test even though that part of the statement is impossible to get to in 9.4. This is true even when I locally added 90500 to the above issue. I believe this will have to either be a dynamic sql statement or a separate script will need to be used.

smurthys commented 6 years ago

@KevinKelly25 Thanks for pointing out for the compilation issue. I am disappointed I failed on that front again. (I failed also on PR #258.)

The solution obviously is to always use dynamic SQL. That is what the recent change does.

BTW, compilation issue aside, I believe the logic to test pg version was correct.

KevinKelly25 commented 6 years ago

Thank you for the update and I was partially wrong with my assessment on the test pg version. Just for possible future usage of this method I will explain the problem more. I believe this is correct way.

   IF 90500 > (SELECT setting::integer FROM pg_catalog.pg_settings
                WHERE name = 'server_version_num'

Rather then:

   IF 90400 >= (SELECT setting::integer FROM pg_catalog.pg_settings
                WHERE name = 'server_version_num'

The problem with the bottom version is that it doesn't account for minor versions that add to the integer. For example, I am running version 9.4.19 this is my output for the above query:

classdb=# SELECT setting::integer FROM pg_catalog.pg_settings
classdb-#                 WHERE name = 'server_version_num';
 setting
---------
   90419

This means I would be greater then 90400 and pass the bottom method putting me into the wrong section of the IF ELSE statement. The first query just checks to make sure it is below 90500 thus allowing any 9.4 minor versions to go the correct path.

However, this is no longer an issue in the PR and as stated above I just wanted to point it out for any possible future use in a similar manner.

smurthys commented 6 years ago

@KevinKelly25 Your analysis is correct. Testing against 90400 only guards against pg 9.3 not 9.4. Thank you for helping me see that.

smurthys commented 6 years ago

Thank you for the reviews