Re: logical decoding and replication of sequences, take 2

On Thu, Jul 13, 2023 at 9:47 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:

>
> >>
> >> 6) simplified protocol versioning
> >
> > I had tested the cross-version logical replication with older set of
> > patches. Didn't see any unexpected behaviour then. I will test again.
> >>
>
> I think the question is what's the expected behavior. What behavior did
> you expect/observe?

Let me run my test again and respond.

>
> IIRC with the previous version of the patch, if you connected an old
> subscriber (without sequence replication), it just ignored/skipped the
> sequence increments and replicated the other changes.

I liked that.

>
> The new patch detects that, and triggers ERROR on the publisher. And I
> think that's the correct thing to do.

With this behaviour users will never be able to setup logical
replication between old and new servers considering almost every setup
has sequences.

>
> There was a lengthy discussion about making this more flexible (by not
> tying this to "linear" protocol version) and/or permissive. I tried
> doing that by doing similar thing to decoding of 2PC, which allows
> choosing when creating a subscription.
>
> But ultimately that just chooses where to throw an error - whether on
> the publisher (in the output plugin callback) or on apply side (when
> trying to apply change to non-existent sequence).

I had some comments on throwing error in [1], esp. towards the end.

>
> I still think it might be useful to have these "capabilities" orthogonal
> to the protocol version, but it's a matter for a separate patch. It's
> enough not to fail with "unknown message" on the subscriber.

Yes, We should avoid breaking replication with "unknown message".

I also agree that improving things in this area can be done in a
separate patch, but as far as possible in this release itself.

> > If the DDL replication takes care of replicating and applying sequence
> > changes, I think we don't need the changes tracking "transactional"
> > sequence changes in this patch-set. That also makes a case for not
> > adding a new field to WAL which may not be used.
> >
>
> Maybe, but the DDL replication patch is not there yet, and I'm not sure
> it's a good idea to make this patch wait for a much larger/complex
> patch. If the DDL replication patch gets committed, it may ditch this
> part (assuming it happens in the same development cycle).
>
> However, my impression was DDL replication would be optional. In which
> case we still need to handle the transactional case, to support sequence
> replication without DDL replication enabled.

As I said before, I don't think this patchset needs to wait for DDL
replication patch. Let's hope that the later lands in the same release
and straightens protocol instead of carrying it forever.

[1] https://www.postgresql.org/message-id/CAExHW5vScYKKb0RZoiNEPfbaQ60hihfuWeLuZF4JKrwPJXPcUw%40mail.gmail.com

--
Best Wishes,
Ashutosh Bapat