Skip to content
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

Double-quoting column names in ON CONFLICT statement (PostgreSQL) #48

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

steverovsky
Copy link

@steverovsky steverovsky commented Dec 12, 2019

PostgreSQL supports uppercase in column names, but Worker class has a bug with ON CONFLICT statement.

Case: When column name in uppercase or contains uppercase letter, we get error like excluded.title does not exist (original column name is Title).

Reason: All identifiers (including column names) that are not double-quoted are folded to lower case in PostgreSQL.

Solution: Column names must to be double-quoted.

@steverovsky steverovsky changed the title use double-quote for column-name in update duplicates psql Double-quoting column names in ON CONFLICT condition (PostgreSQL) Dec 12, 2019
@steverovsky steverovsky changed the title Double-quoting column names in ON CONFLICT condition (PostgreSQL) Double-quoting column names in ON CONFLICT statement (PostgreSQL) Dec 12, 2019
@steverovsky
Copy link
Author

Last Job in Check failed before starting test, freeze on line:

ruby-head - #importing gemset /home/travis/.rvm/gemsets/global.gems

Copy link
Collaborator

@mberlanda mberlanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steverovsky @ziggy1 Thank you very much for the time taken to write and review this PR.
The current test matrix is actually broken and it has been fixed in #51.
Once this would be merged, you may be able to rebase and get your tests working.

Would it be possible to provide an example from the psql console of the double-quoting issue on the ON CONFLICT statement?
As per today, the output of this gem is not tested yet against real datastores (that's my bad since I never finished #37 🤦‍♂ )

Thanks again for the time spent on this pr and do not hesitate to request my review

Copy link
Collaborator

@mberlanda mberlanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @steverovsky @ziggy1 I updated the test matrix to validate any future change. Could you please rebase on the master branch and submit the PR again? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants