On 13.02.22 14:10, Tomas Vondra wrote:
> Here's the remaining part, rebased, with a small tweak in the TAP test
> to eliminate the issue with not waiting for sequence increments. I've
> kept the tweak in a separate patch, so that we can throw it away easily
> if we happen to resolve the issue.
This basically looks fine to me. You have identified a few XXX and
FIXME spots that should be addressed.
Here are a few more comments:
* general
Handling of owned sequences, as previously discussed. This would
probably be a localized change somewhere in get_rel_sync_entry(), so it
doesn't affect the overall structure of the patch.
pg_dump support is missing.
Some psql \dxxx support should probably be there. Check where existing
publication-table relationships are displayed.
* src/backend/catalog/system_views.sql
+ LATERAL pg_get_publication_sequences(P.pubname) GPT,
The GPT presumably stood for "get publication tables", so should be changed.
(There might be a few more copy-and-paste occasions like this around. I
have not written down all of them.)
* src/backend/commands/publicationcmds.c
This adds a new publication option called "sequence". I would have
expected it to be named "sequences", but that's just my opinion.
But in any case, the option is not documented at all.
Some of the new functions added in this file are almost exact duplicates
of the analogous functions for tables. For example,
PublicationAddSequences()/PublicationDropSequences() are almost
exactly the same as PublicationAddTables()/PublicationDropTables().
This could be handled with less duplication by just adding an ObjectType
argument to the existing functions.
* src/backend/commands/sequence.c
Could use some refactoring of ResetSequence()/ResetSequence2(). Maybe
call the latter ResetSequenceData() and have the former call it internally.
* src/backend/commands/subscriptioncmds.c
Also lots of duplication between tables and sequences in this file.
* src/backend/replication/logical/tablesync.c
The comment says it needs sequence support, but there appears to be
support for initial sequence syncing. What exactly is missing here?
* src/test/subscription/t/028_sequences.pl
Change to use done_testing() (see 549ec201d6132b7c7ee11ee90a4e02119259ba5b).