Re: logical decoding and replication of sequences, take 2

On Tue, Feb 20, 2024 at 1:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> You might be interested in more detail [1] where I first reported this
> problem and also [2] where we concluded why this is not creating a
> real problem.
>
> [1] https://www.postgresql.org/message-id/CAFiTN-vAx-Y%2B19ROKOcWnGf7ix2VOTUebpzteaGw9XQyCAeK6g%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CAFiTN-sYpyUBabxopJysqH3DAp4OZUCTi6m_qtgt8d32vDcWSA%40mail.gmail.com

Thanks. Dilip and I just spent a lot of time talking this through on a
call. One of the key bits of logic is here:

+ /* Skip the change if already processed (per the snapshot). */
+ if (transactional &&
+ !SnapBuildProcessChange(builder, xid, buf->origptr))
+ return;
+ else if (!transactional &&
+ (SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT ||
+   SnapBuildXactNeedsSkip(builder, buf->origptr)))
+ return;

As a stylistic note, I think this would be mode clear if it were
written if (transactional) { if (!SnapBuildProcessChange()) return; }
else { if (something else) return; }.

Now, on to correctness. It's possible for us to identify a
transactional change as non-transactional if smgr_decode() was called
for the relfilenode before SNAPBUILD_FULL_SNAPSHOT was reached. In
that case, if !SnapBuildProcessChange() would have been true, then we
need SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT ||
SnapBuildXactNeedsSkip(builder, buf->origptr) to also be true.
Otherwise, we'll process this change when we wouldn't have otherwise.
But Dilip made an argument to me about this which seems correct to me.
snapbuild.h says that SNAPBUILD_CONSISTENT is reached only when we
find a point where any transaction that was running at the time we
reached SNAPBUILD_FULL_SNAPSHOT have finished. So if this transaction
is one for which we incorrectly identified the sequence change as
non-transactional, then we cannot be in the SNAPBUILD_CONSISTENT state
yet, so SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT will be
true and hence whole "or" condition we'll be true and we'll return. So
far, so good.

I think, anyway. I haven't comprehensively verified that the comment
in snapbuild.h accurately reflects what the code actually does. But if
it does, then presumably we shouldn't see a record for which we might
have mistakenly identified a change as non-transactional after
reaching SNAPBUILD_CONSISTENT, which seems to be good enough to
guarantee that the mistake won't matter.

However, the logic in smgr_decode() doesn't only care about the
snapshot state. It also cares about the fast-forward flag:

+ if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
+ ctx->fast_forward)
+ return;

Let's say fast_forward is true. Then smgr_decode() is going to skip
recording anything about the relfilenode, so we'll identify all
sequence changes as non-transactional. But look at how this case is
handled in seq_decode():

+ if (ctx->fast_forward)
+ {
+ /*
+ * We need to set processing_required flag to notify the sequence
+ * change existence to the caller. Usually, the flag is set when
+ * either the COMMIT or ABORT records are decoded, but this must be
+ * turned on here because the non-transactional logical message is
+ * decoded without waiting for these records.
+ */
+ if (!transactional)
+ ctx->processing_required = true;
+
+ return;
+ }

This seems suspicious. Why are we testing the transactional flag here
if it's guaranteed to be false? My guess is that the person who wrote
this code thought that the flag would be accurate even in this case,
but that doesn't seem to be true. So this case probably needs some
more thought.

It's definitely not great that this logic is so complicated; it's
really hard to verify that all the tests match up well enough to keep
us out of trouble.

--
Robert Haas
EDB: http://www.enterprisedb.com