Re: logical decoding and replication of sequences

On Sat, Mar 26, 2022 at 3:26 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> On 3/26/22 08:28, Amit Kapila wrote:
> >>
> >> 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
>

We already call CommitTransactionCommand after end_replication_step at
a few places in that file so as there is no explicit commit in
non-transactional case, we can probably call CommitTransactionCommand
for it.

> and the current code seems a bit complex. And I'm not sure it's quite
> correct. So this place needs more work.
>

Agreed.

>
> >> 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.
>

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. 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.

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 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.

> > 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.

--
With Regards,
Amit Kapila.