Re: logical decoding and replication of sequences

On 7/20/21 5:30 PM, Peter Eisentraut wrote:
> On 08.06.21 00:28, Tomas Vondra wrote:
>>
>> new "sequence" publication action
>> ---------------------------------
>>
>> The publications now have a new "sequence" publication action, which is
>> enabled by default. This determines whether the publication decodes
>> sequences or what.
>>
>>
>> FOR ALL SEQUENCES
>> -----------------
>>
>> It should be possible to create FOR ALL SEQUENCES publications, just
>> like we have FOR ALL TABLES. But this produces shift/reduce conflicts
>> in the grammar, and I didn't bother dealing with that. So for now it's
>> required to do ALTER PUBLICATION ... [ADD | DROP] SEQUENCE ...
> 
> I have been thinking about these DDL-level issues a bit.  The most
> common use case will be to have a bunch of tables with implicit
> sequences, and you just want to replicate them from here to there
> without too much effort.  So ideally an implicit sequence should be
> replicated by default if the table is part of a publication (unless
> sequences are turned off by the publication option).
> 

Agreed, that seems like a reasonable approach.

> We already have support for things like that in
> GetPublicationRelations(), where a partitioned table is expanded to
> include the actual partitions.  I think that logic could be reused.  So
> in general I would have GetPublicationRelations() include sequences and
> don't have GetPublicationSequenceRelations() at all.  Then sequences
> could also be sent by pg_publication_tables(), maybe add a relkind
> column.  And then you also don't need so much duplicate DDL code, if you
> just consider everything as a relation.  For example, there doesn't seem
> to be an actual need to have fetch_sequence_list() and subsequent
> processing on the subscriber side.  It does the same thing as
> fetch_table_list(), so it might as well just all be one thing.
> 

Not sure. I agree with replicating implicit sequences by default,
without having to add them to the publication. But I think we should
allow adding other sequences too, and I think some of this code and
differentiation from tables will be needed.

FWIW I'm not claiming there are no duplicate parts - I've mostly
copy-pasted the table-handling code for sequences, and I'll look into
reusing some of it.

> We do, however, probably need some checking that we don't replicate
> tables to sequences or vice versa.
> 

True. I haven't tried doing such silly things yet.

> We probably also don't need a separate FOR ALL SEQUENCES option.  What
> users really want is a "for everything" option.  We could think about
> renaming or alternative syntax, but in principle I think FOR ALL TABLES
> should include sequences by default.
> 
> Tests under src/test/subscription/ are needed.
> 

Yeah, true. At the moment there are just tests in test_decoding, mostly
because the previous patch versions did not add support for sequences to
built-in replication. Will fix.

> I'm not sure why test_decoding needs a skip-sequences option.  The
> source code says it's for backward compatibility, but I don't see why we
> need that.

Hmmm, I'm a bit baffled by skip-sequences true. I think Cary added it to
limit chances in test_decoding tests, while the misleading comment about
backwards compatibility comes from me.


regards

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