Fix regression in allowed foreign keys on distributed tables #6550
Conversation
JelteF
changed the title
src/test/regress/expected/multi_alter_table_add_constraints.out
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #6550 +/- ##
==========================================
- Coverage 93.02% 92.89% -0.14%
==========================================
Files 259 259
Lines 55859 55475 -384
==========================================
- Hits 51965 51532 -433
- Misses 3894 3943 +49 |
src/backend/distributed/commands/cascade_table_operation_for_connected_relations.c
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
For the sake of simplicity, it looks like we could remove more code in addition to auto_explain workaround.
We signal postgres not to do any validation by setting
Constraint::skip_validation to true
causing not any select queries triggered by postgres.
This is intented to be done in two situations:
- The user requests it explicitly by setting GUC skip_constraint_validation to on which sets SkipConstraintValidation = true internally.
- Citus decides to skip validation implicitly in SkipForeignKeyValidationIfConstraintIsFkey function in various places.
Can we remove src/backend/distributed/executor/multi_executor.c:203:
if (AlterTableConstraintCheck(queryDesc))
block in the citus executor given that no unintended validation queries should end up here anymore since they are not planned in the first place?
| if (skip_validation && IsA(parseTree, AlterTableStmt)) | ||
| { | ||
| EnableSkippingConstraintValidation(); |
There was a problem hiding this comment.
EnableSkippingConstraintValidation is not used anywhere anymore. Can we remove it?
|
Anything blocking this PR? |
In commit eadc88a we changed how we skip foreign key validation. The goal was to skip it in more cases. However, one change had the unintended regression of introducing failures when trying to create certain foreign keys. This reverts that part of the change. The new way of skipping validation of foreign keys skipped the validation during execution. The reason that this caused this regression was because some foreign key validation queries already fail during planning. In those cases it never gets to the execution step where it would later be skipped. Fixes #6543
JelteF
force-pushed
the
fix-6543
branch
2 times, most recently
from
05a505e to
3cb9d66
Compare
|
@emelsimsek I think that cleanup might work, but I'd have to check the code better to see if we're not missing anything. Based on the comment in multi_executor.c it suggests there are certain cases where this is not enough. In any case such a refactor is probably to risky to backport (without good reason). I think we should merge this PR and then we can work on top of it. |
|
Makes sense. I am curious if we are clear on what this GUC provides on top of existing postgres ways of deferring validation checks. (https://www.dbi-services.com/blog/enabling-disabling-and-validating-foreign-key-constraints-in-postgresql/) It looks like we want to default to skipping for empty shell tables always and do the validation on the shards at workers without the client having to set this GUC. |
The main reason is we want to actually not do the check at all to reduce the spend reduce CPU and IO time. AS INVALID simply postpones the check for later when less heavy locks are held, but it still needs to actually check that the constraint holds. Our skipping completely skips the check, because we know that it will be valid anyway. We used AS INVALID together with some manual catalog changes before 11.1 for shard moves, but that had the problem that AS INVALID is not supported on partitioned tables. |
I just checked, and it did not: https://github.com/citusdata/citus/tree/cleanup-constraint-validation It seems we'd need more logic in the utility hook to skip foreign keys in certain cases, at least related to partitioning (as the comment suggests is the reason for the constraint check in the executor function). |
DESCRIPTION: Fix regression in allowed foreign keys on distributed tables In commit eadc88a we changed how we skip foreign key validation. The goal was to skip it in more cases. However, one change had the unintended regression of introducing failures when trying to create certain foreign keys. This reverts that part of the change. The way of skipping validation of foreign keys that was introduced in eadc88a was skipping validation during execution. The reason that this caused this regression was because some foreign key validation queries already fail during planning. In those cases it never gets to the execution step where it would later be skipped. Fixes #6543
DESCRIPTION: Fix foreign key validation skip at the end of shard move In eadc88a we started completely skipping foreign key constraint validation at the end of a non blocking shard move, instead of only for foreign keys to reference tables. However, it turns out that this didn't work at all because of a hard to notice bug: By resetting the SkipConstraintValidation flag at the end of our utility hook, we actually make the SET command that sets it a no-op. This fixes that bug by removing the code that resets it. This is fine because #6543 removed the only place where we set the flag in C code. So the resetting of the flag has no purpose anymore. This PR also adds a regression test, because it turned out we didn't have any otherwise we would have caught that the feature was completely broken. It also moves the constraint validation skipping to the utility hook. The reason is that #6550 showed us that this is the better place to skip it, because it will also skip the planning phase and not just the execution.
DESCRIPTION: Fix regression in allowed foreign keys on distributed tables In commit eadc88a we changed how we skip foreign key validation. The goal was to skip it in more cases. However, one change had the unintended regression of introducing failures when trying to create certain foreign keys. This reverts that part of the change. The way of skipping validation of foreign keys that was introduced in eadc88a was skipping validation during execution. The reason that this caused this regression was because some foreign key validation queries already fail during planning. In those cases it never gets to the execution step where it would later be skipped. Fixes #6543 (cherry picked from commit 7a7880a)
DESCRIPTION: Fix foreign key validation skip at the end of shard move In eadc88a we started completely skipping foreign key constraint validation at the end of a non blocking shard move, instead of only for foreign keys to reference tables. However, it turns out that this didn't work at all because of a hard to notice bug: By resetting the SkipConstraintValidation flag at the end of our utility hook, we actually make the SET command that sets it a no-op. This fixes that bug by removing the code that resets it. This is fine because #6543 removed the only place where we set the flag in C code. So the resetting of the flag has no purpose anymore. This PR also adds a regression test, because it turned out we didn't have any otherwise we would have caught that the feature was completely broken. It also moves the constraint validation skipping to the utility hook. The reason is that #6550 showed us that this is the better place to skip it, because it will also skip the planning phase and not just the execution. (cherry picked from commit 81dcddd)
DESCRIPTION: Fix regression in allowed foreign keys on distributed tables In commit eadc88a we changed how we skip foreign key validation. The goal was to skip it in more cases. However, one change had the unintended regression of introducing failures when trying to create certain foreign keys. This reverts that part of the change. The way of skipping validation of foreign keys that was introduced in eadc88a was skipping validation during execution. The reason that this caused this regression was because some foreign key validation queries already fail during planning. In those cases it never gets to the execution step where it would later be skipped. Fixes #6543
DESCRIPTION: Fixes a regression in allowed foreign keys on distributed tables
In commit eadc88a we changed how we skip foreign key validation. The
goal was to skip it in more cases. However, one change had the
unintended regression of introducing failures when trying to create
certain foreign keys. This reverts that part of the change.
The way of skipping validation of foreign keys that was introduced in eadc88a was skipping
validation during execution. The reason that this caused this regression
was because some foreign key validation queries already fail during
planning. In those cases it never gets to the execution step where it
would later be skipped.
Fixes #6543