New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve DatabaseMetaData.getSQLKeywords() #940

Merged
merged 5 commits into from Mar 11, 2018

Conversation

Projects
None yet
5 participants
@jorsol
Contributor

jorsol commented Sep 9, 2017

Use a more complete list of keywords for Pg 8.2 - 8.4, return the correct list of keywords for PostgreSQL 9.0+ using pg_catalog.pg_get_keywords()

@codecov-io

This comment has been minimized.

codecov-io commented Sep 9, 2017

Codecov Report

Merging #940 into master will increase coverage by 0.26%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master     #940      +/-   ##
============================================
+ Coverage     67.38%   67.65%   +0.26%     
- Complexity     3687     3762      +75     
============================================
  Files           170      170              
  Lines         15638    15906     +268     
  Branches       2531     2604      +73     
============================================
+ Hits          10538    10761     +223     
- Misses         3917     3933      +16     
- Partials       1183     1212      +29

@jorsol jorsol force-pushed the jorsol:get-keywords branch 2 times, most recently from 9baa1eb to 5e7ba93 Sep 19, 2017

@vlsi vlsi added the needs-review label Sep 25, 2017

@jorsol jorsol force-pushed the jorsol:get-keywords branch 4 times, most recently from 32965fd to 361d50a Oct 9, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Oct 11, 2017

This is not really useful and a bit ugly.

@davecramer

This comment has been minimized.

Member

davecramer commented Feb 20, 2018

I think this makes more sense as it will be specific to the version of the database as well

jorsol and others added some commits Sep 8, 2017

@vlsi vlsi force-pushed the jorsol:get-keywords branch from edd214a to dbc9731 Mar 10, 2018

@vlsi vlsi added this to the 42.2.2 milestone Mar 10, 2018

@vlsi vlsi removed the needs-review label Mar 11, 2018

@vlsi vlsi merged commit 7a586b6 into pgjdbc:master Mar 11, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: improve DatabaseMetaData.getSQLKeywords() (pgjdbc#940)
Fetch keywords from pg_catalog.pg_get_keywords()

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: improve DatabaseMetaData.getSQLKeywords() (pgjdbc#940)
Fetch keywords from pg_catalog.pg_get_keywords()
@fredgalvao

This comment has been minimized.

fredgalvao commented Sep 22, 2018

I just stumbled upon an issue with this, which in my opinion makes this change quite breaking.
Upgrading from 42.2.0 to 42.2.5 should not break anything, by any versioning model.

My case: Liquibase uses a column named LOCKED for a lock table, and the abstract query manager there peeks at the keywords from the DatabaseMetaData to see if it needs quoting as a identifier. As of 42.2.2 it will now see LOCKED as a keyword that can't be used as an identifier, which is not true[1], and then proceed to mistakenly quote it as "LOCKED", and then fail as the original column name was actually locked, lowercase.

[1]: PostgreSQL describes in detail how lenient it actually is with keywords in here, and we actually have access to that through the catdesc column on select * from pg_catalog.pg_get_keywords(), which has "unreserved" for locked.

If we're gonna update the keyword list to a modern one, which is awesome, we should at least only return the ones that are actually /^reserved.*/.

Of course some of the above is my opinion, so please let me know if I got anything wrong. Otherwise, we should improve [this implementation].

@jorsol

This comment has been minimized.

Contributor

jorsol commented Sep 22, 2018

@fredgalvao , what version of Liquibase are you using?

While it may make sense to filter by catcode in ('R', 'T'), there is nothing in the JDBC specification that requires to return only "reserved keywords". The javadoc reads:

getSQLKeywords​()
Retrieves a comma-separated list of all of this database's SQL keywords that are NOT also SQL:2003 keywords.

and some SQL:2003 keywords are "reserved keywords", so in this context getSQLKeywords​() can't be used to get words that should be used for quoting.

Researching on the subject, I first asked you about the version of Liquibase because since 2013, Liquibase don't use getSQLKeywords​() since version 3.0.7: http://www.liquibase.org/2013/10/liquibase-3-0-7-release.html

Check the item CORE-1530
and the commit: liquibase/liquibase@ea08ed9#diff-33e0a638d9cb559a5626234af67dc777

They are now using a static list of keywords:
https://github.com/liquibase/liquibase/blob/1ce3798a8cee083d80d4297d4400f6703468346f/liquibase-core/src/main/java/liquibase/database/core/PostgresDatabase.java#L42-L54

If Liquibase is generating dynamic sql and adding quoting based on this, my best recommendation is that they should use quote_ident(string text).

@fredgalvao

This comment has been minimized.

fredgalvao commented Sep 22, 2018

@jorsol That cleared a bunch of things for me, thanks for the additional research!

I'd still say that the wording and meaning behind terms such as reserved, keyword, and quoting scopes are not as clear as they could be, but at least the implementation is as agnostic as the javadoc asks it to be, which is fine I guess.

So, unfortunately I'm stuck for a while on org.liquibase:liquibase-core:2.0.5 on a few projects because of a chain of old dependencies I can't update right away. That leaves me in an intermediate state where I can't use newer jdbc drivers for postgres just yet.

I'll document my dependency with this issue and update when liquibase can be chain-updated.

Thanks again!

@jorsol jorsol deleted the jorsol:get-keywords branch Sep 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment