Re: logical decoding and replication of sequences - Mailing list pgsql-hackers
From | Peter Eisentraut |
---|---|
Subject | Re: logical decoding and replication of sequences |
Date | |
Msg-id | f64b0ecd-0723-c70d-8ec5-2b5c0e476a50@enterprisedb.com Whole thread Raw |
In response to | Re: logical decoding and replication of sequences (Tomas Vondra <tomas.vondra@enterprisedb.com>) |
Responses |
Re: logical decoding and replication of sequences
|
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.
Attachment
pgsql-hackers by date: