Re: logical decoding and replication of sequences

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.