Message-ID: <494bb2d9-0c1c-b429-e6bb-231f00f2c36f@enterprisedb.com>
Date: Sat, 26 Mar 2022 10:56:23 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: logical decoding and replication of sequences
Content-Language: en-US
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Petr Jelinek <petr.jelinek@enterprisedb.com>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <d045f3c2-6cfb-06d3-5540-e63c320df8bc@enterprisedb.com>
 <f64b0ecd-0723-c70d-8ec5-2b5c0e476a50@enterprisedb.com>
 <6b21233e-c880-e4ad-5df2-7832f769296c@enterprisedb.com>
 <9da788c7-01eb-f11d-aa08-c5f61fe9f33a@enterprisedb.com>
 <CAA4eK1Jro8hGX_hzXCznBr++suoDM7avEhFtiphPeDLPBOc=Ow@mail.gmail.com>
 <456A528F-8F19-41C7-9857-927024D32396@enterprisedb.com>
 <CAA4eK1Jn-DttQ=4Pdh9YCe1w+zGbgC+0uR0sfg2RtkjiAPmB9g@mail.gmail.com>
 <BDDB28EE-84D3-4BA8-A5BD-DFF56CE584BC@enterprisedb.com>
 <aeb2ba8d-e6f4-5486-cc4c-0d4982c291cb@enterprisedb.com>
 <74729cc5-960b-9295-912a-a7c37356a0c4@enterprisedb.com>
 <CAA4eK1JxDNKGBSNTyN-Xj2JRjzFo+ziSqJbjH==vuO0YF_CQrg@mail.gmail.com>
 <9d7635f8-2402-e038-64da-cbc3732ce8cf@enterprisedb.com>
 <CAA4eK1KG2hj-=5y6Ufc67q8O2P=OgRA584DahK7Sz6UuQvf0qA@mail.gmail.com>
 <c0d61b5f-c122-a057-8063-b7303fb8bdbd@enterprisedb.com>
 <83c682bf-7164-5976-608c-385cc394bb6c@enterprisedb.com>
 <CAA4eK1JrCVD8fQeNQmYo1YnPay_6Mh9fsTYMrAHkNo89Pxr4LA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAA4eK1JrCVD8fQeNQmYo1YnPay_6Mh9fsTYMrAHkNo89Pxr4LA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/494bb2d9-0c1c-b429-e6bb-231f00f2c36f%40enterprisedb.com>
Precedence: bulk


On 3/26/22 08:28, Amit Kapila wrote:
> On Fri, Mar 25, 2022 at 10:20 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> Hmm, so fixing this might be a bit trickier than I expected.
>>
>> Firstly, currently we only send nspname/relname in the sequence message,
>> not the remote OID or schema. The idea was that for sequences we don't
>> really need schema info, so this seemed OK.
>>
>> But should_apply_changes_for_rel() needs LogicalRepRelMapEntry, and to
>> create/maintain that those records we need to send the schema.
>>
>> Attached is a WIP patch does that.
>>
>> Two places need more work, I think:
>>
>> 1) maybe_send_schema needs ReorderBufferChange, but we don't have that
>> for sequences, we only have TXN. I created a simple wrapper, but maybe
>> we should just tweak maybe_send_schema to use TXN.
>>
>> 2) The transaction handling in is a bit confusing. The non-transactional
>> increments won't have any explicit commit later, so we can't just rely
>> on begin_replication_step/end_replication_step. But I want to try
>> spending a bit more time on this.
>>
> 
> I didn't understand what you want to say in point (2).
> 

My point is that handle_apply_sequence() either needs to use the same
transaction handling as other apply methods, or start (and commit) a
separate transaction for the "transactional" case.

Which means we can't use the begin_replication_step/end_replication_step
and the current code seems a bit complex. And I'm not sure it's quite
correct. So this place needs more work.

>>
>> But there's a more serious issue, I think. So far, we allowed this:
>>
>>   BEGIN;
>>   CREATE SEQUENCE s2;
>>   ALTER PUBLICATION p ADD SEQUENCE s2;
>>   INSERT INTO seq_test SELECT nextval('s2') FROM generate_series(1,100);
>>   COMMIT;
>>
>> and the behavior was that we replicated the changes. But with the patch
>> applied, that no longer happens, because should_apply_changes_for_rel
>> says the change should not be applied.
>>
>> And after thinking about this, I think that's correct - we can't apply
>> changes until ALTER SUBSCRIPTION ... REFRESH PUBLICATION gets executed,
>> and we can't do that until the transaction commits.
>>
>> So I guess that's correct, and the current behavior is a bug.
>>
> 
> Yes, I also think that is a bug.
> 

OK

>> For a while I was thinking that maybe this means we don't need the
>> transactional behavior at all, but I think we do - we have to handle
>> ALTER SEQUENCE cases that are transactional.
>>
> 
> I need some time to think about this.

Understood.

> At all places, it is mentioned
> as creating a sequence for transactional cases which at the very least
> need some tweak.
> 

Which places?


regards

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