Re: logical decoding and replication of sequences, take 2

[Ashutosh Bapat]

On Thu, Dec 14, 2023 at 12:37 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> I think you forgot to attach the patch.

Sorry. Here it is.

On Thu, Dec 14, 2023 at 2:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> >
> It looks like the solution works. But this is the only place where we
> process a change before SNAPSHOT reaches FULL. But this is also the
> only record which affects a decision to queue/not a following change.
> So it should be ok. The sequence_hash'es as separate for each
> transaction and they are cleaned when processing COMMIT record.
> >
>
> But it is possible that even commit or abort also happens before the
> snapshot reaches full state in which case the hash table will have
> stale or invalid (for aborts) entries. That will probably be cleaned
> at a later point by running_xact records.

Why would cleaning wait till running_xact records? Won't txn entry
itself be removed when processing commit/abort record? At the same the
sequence hash will be cleaned as well.

> Now, I think in theory, it
> is possible that the same RelFileLocator can again be allocated before
> we clean up the existing entry which can probably confuse the system.

How? The transaction allocating the first time would be cleaned before
it happens the second time. So shouldn't matter.

-- 
Best Wishes,
Ashutosh Bapat

diff --git a/src/backend/replication/logical/decode.c 
b/src/backend/replication/logical/decode.c
index 73e38cafd8..8e2ebbd8e0 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -1543,10 +1543,15 @@ smgr_decode(LogicalDecodingContext *ctx, 
XLogRecordBuffer *buf)
        elog(LOG, "XXX: smgr_decode. snapshot is %s", 
SnapBuildIdentify(builder));
 
        /*
-        * If we don't have snapshot or we are just fast-forwarding, there is no
-        * point in decoding relfilenode information.
+        * If we are not building snapshot yet or we are just fast-forwarding, 
there
+        * is no point in decoding relfilenode information.  If the sequence
+        * associated with relfilenode here is changed in the same transaction 
but
+        * after snapshot was built, the relfilenode needs to be present in the
+        * sequence hash table so that the change can be deemed as 
transactional.
+        * Otherwise it will not be queued. Hence we process this change even 
before
+        * we have built snapshot.
         */
-       if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
+       if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT ||
                ctx->fast_forward)
        {
                elog(LOG, "XXX: skipped");