Re: logical decoding and replication of sequences - Mailing list pgsql-hackers

Further review (based on 20220310 patch):

  doc/src/sgml/ref/create_publication.sgml       |   3 +

For the clauses added to the synopsis, descriptions should be added
below.  See attached patch for a start.

  src/backend/commands/sequence.c                |  79 ++

There is quite a bit of overlap between ResetSequence() and
ResetSequence2(), but I couldn't see a good way to combine them that
genuinely saves code and complexity.  So maybe it's ok.

Actually, ResetSequence2() is not really "reset", it's just "set".
Maybe pick a different function name.

  src/backend/commands/subscriptioncmds.c        | 272 +++++++

The code added in AlterSubscription_refresh() seems to be entirely
copy-and-paste from the tables case.  I think this could be combined
by concatenating the lists from fetch_table_list() and
fetch_sequence_list() and looping over it once.  The same also applies
to CreateSubscription(), although the code duplication is smaller
there.

This in turn means that fetch_table_list() and fetch_sequence_list()
can be combined, so that you don't actually need any extensive new
code in CreateSubscription() and AlterSubscription_refresh() for
sequences.  This could go on, you can combine more of the underlying
code, like pg_publication_tables and pg_publication_sequences and so
on.

  src/backend/replication/logical/proto.c        |  52 ++

The documentation of the added protocol message needs to be added to
the documentation.  See attached patch for a start.

The sequence message does not contain the sequence Oid, unlike the
relation message.  Would that be good to add?

  src/backend/replication/logical/worker.c       |  56 ++

Maybe the Asserts in apply_handle_sequence() should be elogs.  These
are checking what is sent over the network, so we don't want a
bad/evil peer able to trigger asserts.  And in non-assert builds these
conditions would be unchecked.

  src/backend/replication/pgoutput/pgoutput.c    |  82 +-

I find the the in get_rel_sync_entry() confusing.  You add a section for

if (!publish && is_sequence)

but then shouldn't the code below that be something like

if (!publish && !is_sequence)

  src/bin/pg_dump/t/002_pg_dump.pl               |  38 +-

This adds a new publication "pub4", but the tests already contain a
"pub4".  I'm not sure why this even works, but perhaps the new one
shold be "pub5", unless there is a deeper meaning.

  src/include/catalog/pg_publication_namespace.h |   3 +-

I don't like how the distinction between table and sequence is done
using a bool field.  That affects also the APIs in pg_publication.c
and publicationcmds.c especially.  There is a lot of unadorned "true"
and "false" being passed around that isn't very clear, and it all
appears to originate at this catalog.  I think we could use a char
field here that uses the relkind constants.  That would also make the
code in pg_publication.c etc. slightly clearer.


See attached patch for more small tweaks.

Your patch still contains a number of XXX and FIXME comments, which in 
my assessment are all more or less correct, so I didn't comment on those 
separately.

Other than that, this seems pretty good.

Earlier in the thread I commented on some aspects of the new grammar 
(e.g., do we need FOR ALL SEQUENCES?).  I think this would be useful to 
review again after all the new logical replication patches are in.  I 
don't want to hold up this patch for that at this point.