Message-ID: <de8cc7c3-4b61-94d5-87dd-ed317b245655@enterprisedb.com>
Date: Fri, 1 Apr 2022 17:02:14 +0200
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>
 <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>
 <494bb2d9-0c1c-b429-e6bb-231f00f2c36f@enterprisedb.com>
 <CAA4eK1KAFdQEULk+4C=ieWA5UPSUtf_gtqKsFj9J9f2c=8hm4g@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <CAA4eK1KAFdQEULk+4C=ieWA5UPSUtf_gtqKsFj9J9f2c=8hm4g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/de8cc7c3-4b61-94d5-87dd-ed317b245655%40enterprisedb.com>
Precedence: bulk


On 3/28/22 07:29, Amit Kapila wrote:
> ...
>
> While thinking about this, I think I see a problem with the
> non-transactional handling of sequences. It seems that we will skip
> sending non-transactional sequence change if it occurs before the
> decoding has reached a consistent point but the surrounding commit
> occurs after a consistent point is reached. In such cases, the
> corresponding DMLs like inserts will be sent but sequence changes
> won't be sent. For example (this scenario is based on
> twophase_snapshot.spec),
> 
> Initial setup:
> ==============
> Create table t1_seq(c1 int);
> Create Sequence seq1;
> 
> Test Execution via multiple sessions (this test allows insert in
> session-2 to happen before we reach a consistent point and commit
> happens after a consistent point):
> =======================================================================================================
> 
> Session-2:
> Begin;
> SELECT pg_current_xact_id();
> 
> Session-1:
> SELECT 'init' FROM pg_create_logical_replication_slot('test_slot',
> 'test_decoding', false, true);
> 
> Session-3:
> Begin;
> SELECT pg_current_xact_id();
> 
> Session-2:
> Commit;
> Begin;
> INSERT INTO t1_seq SELECT nextval('seq1') FROM generate_series(1,100);
> 
> Session-3:
> Commit;
> 
> Session-2:
> Commit 'foo'
> 
> Session-1:
> SELECT data  FROM pg_logical_slot_get_changes('test_slot', NULL, NULL,
> 'include-xids', 'false', 'skip-empty-xacts', '1');
> 
>                      data
> ----------------------------------------------
>  BEGIN
>  table public.t1_seq: INSERT: c1[integer]:1
>  table public.t1_seq: INSERT: c1[integer]:2
>  table public.t1_seq: INSERT: c1[integer]:3
>  table public.t1_seq: INSERT: c1[integer]:4
>  table public.t1_seq: INSERT: c1[integer]:5
>  table public.t1_seq: INSERT: c1[integer]:6
> 
> 
> Now, if we normally try to decode such an insert, the result would be
> something like:
>                                      data
> ------------------------------------------------------------------------------
>  sequence public.seq1: transactional:0 last_value: 33 log_cnt: 0 is_called:1
>  sequence public.seq1: transactional:0 last_value: 66 log_cnt: 0 is_called:1
>  sequence public.seq1: transactional:0 last_value: 99 log_cnt: 0 is_called:1
>  sequence public.seq1: transactional:0 last_value: 132 log_cnt: 0 is_called:1
>  BEGIN
>  table public.t1_seq: INSERT: c1[integer]:1
>  table public.t1_seq: INSERT: c1[integer]:2
>  table public.t1_seq: INSERT: c1[integer]:3
>  table public.t1_seq: INSERT: c1[integer]:4
>  table public.t1_seq: INSERT: c1[integer]:5
>  table public.t1_seq: INSERT: c1[integer]:6
> 
> This will create an inconsistent replica as sequence changes won't be
> replicated.

Hmm, that's interesting. I wonder if it can actually happen, though.
Have you been able to reproduce that, somehow?

> I thought about changing snapshot dealing of
> non-transactional sequence changes similar to transactional ones but
> that also won't work because it is only at commit we decide whether we
> can send the changes.
> 
I wonder if there's some earlier LSN (similar to the consistent point)
which might be useful for this.

Or maybe we should queue even the non-transactional changes, not
per-transaction but in a global list, and then at each commit either
discard inspect them (at that point we know the lowest LSN for all
transactions and the consistent point). Seems complex, though.

> For the transactional case, as we are considering the create sequence
> operation as transactional, we would unnecessarily queue them even
> though that is not required. Basically, they don't need to be
> considered transactional and we can simply ignore such messages like
> other DDLs. But for that probably we need to distinguish Alter/Create
> case which may or may not be straightforward. Now, queuing them is
> probably harmless unless it causes the transaction to spill/stream.
> 

I'm not sure I follow. Why would we queue them unnecessarily?

Also, there's the bug with decoding changes in transactions that create
the sequence and add it to a publication. I think the agreement was that
this behavior was incorrect, we should not decode changes until the
subscription is refreshed. Doesn't that mean can't be any CREATE case,
just ALTER?

> I still couldn't think completely about cases where a mix of
> transactional and non-transactional changes occur in the same
> transaction as I think it somewhat depends on what we want to do about
> the above cases.
> 

Understood. I need to think about this too.

>>> At all places, it is mentioned
>>> as creating a sequence for transactional cases which at the very least
>>> need some tweak.
>>>
>>
>> Which places?
>>
> 
> In comments like:
> a. When decoding sequences, we differentiate between sequences created
> in a (running) transaction and sequences created in other (already
> committed) transactions.
> b. ... But for new sequences, we need to handle them in a transactional way, ..
> c. ... Change needs to be handled as transactional, because the
> sequence was created in a transaction that is still running ...
> 
> It seems all these places indicate a scenario of creating a sequence
> whereas we want to do transactional stuff mainly for Alter.
> 

Right, I'll think about how to clarify the comments.


regards

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