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

Fix updateable result set when there are primary keys and unique keys #2228

Merged
merged 5 commits into from Sep 3, 2021

Conversation

chalmagr
Copy link

All Submissions:

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?
    • Some tests were failing locally due to incorrectly setup docker DB or test (replication cannot be executed) and had to manually fix the test user to have replication role as well as wal setting to 'logical' instead of 'replica'

…ue keys

Fix issues introduced in PR# 2199 causing problems when updating tables with PK and UK.
Originally updateable result set supports only primary keys, wich the forementioned change
and this fix unique keys are also eligible to enable updateable result set. This change will
find the better suited primary or unique key that has all the available columns in the query, but
if no match is found it will continue to fail as it used to.

Closes issue 2196
@chalmagr
Copy link
Author

@davecramer Created this PR that covers issues mentioned before

Add missing @nullable annotations to primaryKeyValue lists
@davecramer
Copy link
Member

I would not worry about the GSS run not passing but the checker framework needs to be fixed

@chalmagr
Copy link
Author

chalmagr commented Aug 20, 2021

I would not worry about the GSS run not passing but the checker framework needs to be fixed

@davecramer Yes - noticed this failed in the CI tasks and it was fixed - should be passing, but the workflow needs approval for it to run (it passes locally)

@chalmagr
Copy link
Author

@davecramer we have been recently working on migrating from Oracle to Postgres, and today we find out that unique keys are handled totally different between each engine... this leads me to think that this PR is not valid since I was under the assumption that Unique indices would enforce uniqueness even for null values, but this is not the case (like Oracle does). e.g. Having table (id1 int not null, id2 int null, value int null) with unique index on id1, id2 them allows to have rows {1,null,10},{1,null,20},etc... this was not the case in Oracle - so this change would actually now be updating all of those entries instead of just the one... this will need to be re-visited and patched accordingly

@chalmagr
Copy link
Author

@davecramer I've updated the code to include your logic for nullable columns and prevent the usage of those for updateable result sets. It may be possible to allow and just have the logic to equals to the values, which would never match for null scenarios... however, this would introduce a bad user experience since we say it is updateable but there are no updates possible.

Just realized that the logic added to check for IS NULL vs = ? no longer applies and can be cleaned up - will do this tomorrow / when I get a chance

Marquez Grabia, Christian added 2 commits August 23, 2021 20:11
…ith nullable columns

Adding logic to ensure all columns in the constraint (primary/unique) are non-nullable (always true for PKs) to guarantee 1-1 updates
No need to have IS NULL when nullable keys are not allowed to guarantee 1-1 updates
Indices with expressions cannot be used (unless expression parsing / resolution
is implemented properly - currently don't see a way to get individual list elements
from pg_index.indexprs to enable proper expression filtering (pg_get_expr gives the
expression, but unable to map this to a specific attribute in the index)
@chalmagr
Copy link
Author

Recently was also attempting to create an index on the table without a constraint but using expressions to make it not-null so that uniqueness is enforced, and found out that this would be causing problems in the logic.

Added logic to exclude the indices that have expressions - other option would be to only allow indices that have a constraint linked to the table (using pg_constraint table in JOIN) - I think it would be possible to support such expressions, but I am unaware on how to extract the specific expression for the attribute (since indexprs is a 'LIST' of expressions, but cannot see how to get individual elements from it so that those could be mapped to the proper attrs of the table)

@davecramer
Copy link
Member

psql -E can give you a lot of information :)

@davecramer
Copy link
Member

@chalmagr so do you consider this complete ? I'd like to push this and get a release out soon(ish)

@davecramer davecramer merged commit c596587 into pgjdbc:release/42.2 Sep 3, 2021
@davecramer
Copy link
Member

Need to commit to master as well

@chalmagr
Copy link
Author

chalmagr commented Sep 25, 2021

@chalmagr so do you consider this complete ? I'd like to push this and get a release out soon(ish)

@davecramer Sorry - been a bit off recently :) - yes, I think this is complete and it seems to be working so far - using expressions will be quite tricky to do as not only we would need to figure out which columns are used, but when building the where condition it would need to re-create the expression and match the value that would also be an expression... I think this is just too much

@davecramer
Copy link
Member

@chalmagr It has been released in 42.2.24. 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.

None yet

2 participants