citusdata / citusdata/citus

Unify constraint-extension logic

Open
#2,667 3 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.