Subject: Re: logical decoding and replication of sequences
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Petr Jelinek <petr.jelinek@enterprisedb.com>
References: <d045f3c2-6cfb-06d3-5540-e63c320df8bc@enterprisedb.com>
 <359bf6d0-413d-292a-4305-e99eeafead39@enterprisedb.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <2dfe62e0-b600-9096-aa5c-7bd7cbc01378@enterprisedb.com>
Date: Tue, 20 Jul 2021 22:27:09 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <359bf6d0-413d-292a-4305-e99eeafead39@enterprisedb.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/2dfe62e0-b600-9096-aa5c-7bd7cbc01378%40enterprisedb.com>
Precedence: bulk


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