Unify constraint-extension logic
- Dominant language
- C
- Stars
- 12.8k
- Forks
- 794
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 31
Description
Digging into #2484, I found that the logic of constraint extension was always a bit subtle, which is what probably led us to introduce a bug in the first place.
# Archaeology
The original logic dates back _at least_ to our first open-source release. The following comment explains how it was supposed to work:
https://github.com/citusdata/citus/blob/136306a1fe3a79cc14e4fcbb38db7f93ecd64d4c/src/backend/distributed/relay/relay_event_utility.c#L73-L78
This was followed by a loop, which implements this logic by _not_ calling `AppendShardIdToName` unless the constraint being processed has an index associated with it…
https://github.com/citusdata/citus/blob/136306a1fe3a79cc14e4fcbb38db7f93ecd64d4c/src/backend/distributed/relay/relay_event_utility.c#L90-L104
The default behavior was _don't extend_.
During the introduction of `create_distributed_table` foreign key support (#888), this conditional snuck in:
https://github.com/citusdata/citus/blob/5a03acf2bf9671abcbb1a36ec1cea76e3f238a48/src/backend/distributed/relay/relay_event_utility.c#L101-L103
It looks innocuous unless you know the old behavior was to fall through without extension. In short, before this change, only the names of index-related constraints were extended, and after, all constraint names were extended. At least that's internally consistent, right?
Welp, you can create constraints with `CHECK` clauses inside of a table declaration, and those clauses are pushed down _as-is_, meaning they are not even processed with this loop (which is for `ALTER TABLE ... (ADD|DROP|VALIDATE) CONSTRAINT`.
Surprisingly, this wasn't reported for over two years, in #2484.
# Options
We can either (a) go back to the _only extend if needed_ mode, or (b) move into the _always extend_ world. I _was_ favoring (a), until I realized I'd need to grok the requirements of foreign key name extension to implement it, which seems fraught. Rather than making that change (and implicitly hoping all future coders don't undo it), I think we should extend all constraint names with shard identifiers. It's possible the impetus for not doing so in 5.0 was because constraint names are often very long, but now we have #783 to mitigate that.
# Tasks
Implementing this would look like:
- [ ] Add a migration step (in SQL) to rename all non-extended constraints using their extended equivalents
- [ ] Add tests to ensure all constraint creation (whether `CHECK` for columns or the whole table in `CREATE TABLE`, or through e.g. `ALTER TABLE`, or through foreign keys) results in shard-extended constraint names
- [ ] Remove the logic in #2651 and always deparse statements with shard-extended constraint names
While #2651 is suitable for backporting, the migration step needed for this "fuller" solution necessitates it being part of a major release.
Contributor guide
Assessment
This issue has not been assessed yet.