Re: logical decoding and replication of sequences, take 2

Hi,

On 2022-11-17 22:13:23 +0100, Tomas Vondra wrote:
> On 11/17/22 18:07, Andres Freund wrote:
> > On 2022-11-17 12:39:49 +0100, Tomas Vondra wrote:
> >> On 11/17/22 03:43, Andres Freund wrote:
> >>> I assume that part of the initial sync would have to be a new sequence
> >>> synchronization step that reads all the sequence states on the publisher and
> >>> ensures that the subscriber sequences are at the same point. There's a bit of
> >>> trickiness there, but it seems entirely doable. The logical replication replay
> >>> support for sequences will have to be a bit careful about not decreasing the
> >>> subscriber's sequence values - the standby initially will be ahead of the
> >>> increments we'll see in the WAL. But that seems inevitable given the
> >>> non-transactional nature of sequences.
> >>>
> >>
> >> See fetch_sequence_data / copy_sequence in the patch. The bit about
> >> ensuring the sequence does not go away (say, using page LSN and/or LSN
> >> of the increment) is not there, however isn't that pretty much what I
> >> proposed doing for "reconciling" the sequence state logged at COMMIT?
> >
> > Well, I think the approach of logging all sequence increments at commit is the
> > wrong idea...
> >
>
> But we're not logging all sequence increments, no?

I was imprecise - I meant streaming them out at commit.



> Yeah, I think you're right. I looked at this again, with fresh mind, and
> I came to the same conclusion. Roughly what the attached patch does.

To me it seems a bit nicer to keep the SnapBuildGetOrBuildSnapshot() call in
decode.c instead of moving it to reorderbuffer.c. Perhaps we should add a
snapbuild.c helper similar to SnapBuildProcessChange() for non-transactional
changes that also gets a snapshot?

Could look something like

    Snapshot snapshot = NULL;

    if (message->transactional &&
        !SnapBuildProcessChange(builder, xid, buf->origptr))
        return;
    else if (!SnapBuildProcessStateNonTx(builder, &snapshot))
        return;

    ...

Or perhaps we should just bite the bullet and add an argument to
SnapBuildProcessChange to deal with that?

Greetings,

Andres Freund