Re: logical decoding and replication of sequences, take 2

On Thu, Jul 20, 2023 at 8:22 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:

> >
> > PFA such edits in 0002 and 0006 patches. Let me know if those look
> > correct. I think we
> > need similar changes to the documentation and comments in other places.
> >
>
> OK, I merged the changes into the patches, with some minor changes to
> the wording etc.

Thanks.

>
> > In sequence_decode() we skip sequence changes when fast forwarding.
> > Given that smgr_decode() is only to supplement sequence_decode(), I
> > think it's correct to do the same in smgr_decode() as well. Simillarly
> > skipping when we don't have full snapshot.
> >
>
> I don't follow, smgr_decode already checks ctx->fast_forward.

In your earlier email you seemed to expressed some doubts about the
change skipping code in smgr_decode(). To that, I gave my own
perspective of why the change skipping code in smgr_decode() is
correct. I think smgr_decode is doing the right thing, IMO. No change
required there.

>
> > Some minor comments on 0006 patch
> >
> > + /* make sure the relfilenode creation is associated with the XID */
> > + if (XLogLogicalInfoActive())
> > + GetCurrentTransactionId();
> >
> > I think this change is correct and is inline with similar changes in 0002. But
> > I looked at other places from where DefineRelation() is called. For regular
> > tables it is called from ProcessUtilitySlow() which in turn does not call
> > GetCurrentTransactionId(). I am wondering whether we are just discovering a
> > class of bugs caused by not associating an xid with a newly created
> > relfilenode.
> >
>
> Not sure. Why would it be a bug?

This discussion is unrelated to sequence decoding but let me add it
here. If we don't know the transaction ID that created a relfilenode,
we wouldn't know whether to roll back that creation if the transaction
gets rolled back during recovery. But maybe that doesn't matter since
the relfilenode is not visible in any of the catalogs, so it just lies
there unused.

>
> > +void
> > +ReorderBufferAddRelFileLocator(ReorderBuffer *rb, TransactionId xid,
> > + RelFileLocator rlocator)
> > +{
> > ... snip ...
> > +
> > + /* sequence changes require a transaction */
> > + if (xid == InvalidTransactionId)
> > + return;
> >
> > IIUC, with your changes in DefineSequence() in this patch, this should not
> > happen. So this condition will never be true. But in case it happens, this code
> > will not add the relfilelocation to the hash table and we will deem the
> > sequence change as non-transactional. Isn't it better to just throw an error
> > and stop replication if that (ever) happens?
> >
>
> It can't happen for sequence, but it may happen when creating a
> non-sequence relfilenode. In a way, it's a way to skip (some)
> unnecessary relfilenodes.

Ah! The comment is correct but cryptic. I didn't read it to mean this.

> > + /*
> > + * To support logical decoding of sequences, we require the sequence
> > + * callback. We decide it here, but only check it later in the wrappers.
> > + *
> > + * XXX Isn't it wrong to define only one of those callbacks? Say we
> > + * only define the stream_sequence_cb() - that may get strange results
> > + * depending on what gets streamed. Either none or both?
> > + *
> > + * XXX Shouldn't sequence be defined at slot creation time, similar
> > + * to two_phase? Probably not.
> > + */
> >
> > Do you intend to keep these XXX's as is? My previous comments on this comment
> > block are in [1].

This comment remains unanswered.

> >
> > In fact, given that whether or not sequences are replicated is decided by the
> > protocol version, do we really need LogicalDecodingContext::sequences? Drawing
> > parallel with WAL messages, I don't think it's needed.
> >
>
> Right. We do that for two_phase because you can override that when
> creating the subscription - sequences allowed that too initially, but
> then we ditched that. So I don't think we need this.

Then we should just remove that member and its references.

--
Best Wishes,
Ashutosh Bapat