Re: logical decoding and replication of sequences

On 2/15/22 10:00, Peter Eisentraut wrote:
> 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.
>

So you're suggesting not to track owned sequences in pg_publication_rel
explicitly, and handle them dynamically in output plugin? So when
calling get_rel_sync_entry on the sequence, we'd check if it's owned by
a table that is replicated.

We'd want a way to enable/disable this for each publication, but that
makes the lookups more complicated. Also, we'd probably need the same
logic elsewhere (e.g. in psql, when listing sequences in a publication).

I'm not sure we want this complexity, maybe we should simply deal with
this in the ALTER PUBLICATION and track all sequences (owned or not) in
pg_publication_rel.

> pg_dump support is missing.
>

Will fix.

> Some psql \dxxx support should probably be there.  Check where existing
> publication-table relationships are displayed.
>

Yeah, I noticed this while adding regression tests. Currently, \dRp+
shows something like this:

Publication testpub_fortbl
Owner | All tables | Inserts | Updates ...
--------------------------+------------+---------+--------- ...
regress_publication_user | f | t | t ...
Tables:
"pub_test.testpub_nopk"
"public.testpub_tbl1"

or

Publication testpub5_forschema
Owner | All tables | Inserts | Updates | ...
--------------------------+------------+---------+---------+- ...
regress_publication_user | f | t | t | ...
Tables from schemas:
"CURRENT_SCHEMA"
"public"

I wonder if we should copy the same block for sequences, so

Publication testpub_fortbl
Owner | All tables | Inserts | Updates ...
--------------------------+------------+---------+--------- ...
regress_publication_user | f | t | t ...
Tables:
"pub_test.testpub_nopk"
"public.testpub_tbl1"
Sequences:
"pub_test.sequence_s1"
"public.sequence_s2"

And same for "tables from schemas" etc.

> * 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.)
>

Will fix.

> * 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.
>

Yes, I noticed that too, and I'll review this later, after making sure
the behavior is correct.

> * src/backend/commands/sequence.c
>
> Could use some refactoring of ResetSequence()/ResetSequence2().  Maybe
> call the latter ResetSequenceData() and have the former call it internally.
>

Will check.

> * src/backend/commands/subscriptioncmds.c
>
> Also lots of duplication between tables and sequences in this file.
>

Same as the case above.

> * 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?
>

I think that's just obsolete comment.

> * src/test/subscription/t/028_sequences.pl
>
> Change to use done_testing() (see
> 549ec201d6132b7c7ee11ee90a4e02119259ba5b).

Will fix.

Do we need to handle both FOR ALL TABLES and FOR ALL SEQUENCES at the
same time? That seems like a reasonable thing people might want.

The patch probably also needs to modify pg_publication_namespace to
track whether the schema is FOR TABLES IN SCHEMA or FOR SEQUENCES.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company