Avoid unexpected changes of CurrentResourceOwner and CurrentMemoryContext

Users of logical decoding can encounter an unexpected change of
CurrentResourceOwner and CurrentMemoryContext.  The problem is that,
unlike other call sites of RollbackAndReleaseCurrentSubTransaction(), in
reorderbuffer.c we fail to restore the original values of these global
variables after being clobbered by subtransaction abort.  This patch
saves the values prior to the call and restores them eventually.

In addition, logical.c and logicalfuncs.c had a hack to restore resource
owner, presumably because of lack of this restore.  Remove that.
Instead, because the test coverage here is not very consistent, add an
Assert() to ensure that the resowner is kept identical; this would make
it easy to detect other cases of bugs were we fail to restore resowner
properly.  This could be removed later.

This is arguably an old bug, but there appears to be no reason to
backpatch it and it's risky to do so, so refrain for now.

Author: Antonin Houska <ah@cybertec.at>
Reported-by: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Reviewed-by: Euler Taveira <euler@eulerto.com>
Discussion: https://postgr.es/m/119497.1756892972@localhost

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 7e363a7c05b4f1890a5db2b9c2411252e152de77..c68c0481f427ac77591a82fac2aa66ca2faa2607 100644 (file)
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -2082,7 +2082,7 @@ LogicalSlotAdvanceAndCheckSnapState(XLogRecPtr moveto,
                                    bool *found_consistent_snapshot)
 {
    LogicalDecodingContext *ctx;
-   ResourceOwner old_resowner = CurrentResourceOwner;
+   ResourceOwner old_resowner PG_USED_FOR_ASSERTS_ONLY = CurrentResourceOwner;
    XLogRecPtr  retlsn;
 
    Assert(moveto != InvalidXLogRecPtr);
@@ -2141,21 +2141,24 @@ LogicalSlotAdvanceAndCheckSnapState(XLogRecPtr moveto,
             * might still have critical updates to do.
             */
            if (record)
+           {
                LogicalDecodingProcessRecord(ctx, ctx->reader);
 
+               /*
+                * We used to have bugs where logical decoding would fail to
+                * preserve the resource owner.  That's important here, so
+                * verify that that doesn't happen anymore.  XXX this could be
+                * removed once it's been battle-tested.
+                */
+               Assert(CurrentResourceOwner == old_resowner);
+           }
+
            CHECK_FOR_INTERRUPTS();
        }
 
        if (found_consistent_snapshot && DecodingContextReady(ctx))
            *found_consistent_snapshot = true;
 
-       /*
-        * Logical decoding could have clobbered CurrentResourceOwner during
-        * transaction management, so restore the executor's value.  (This is
-        * a kluge, but it's not worth cleaning up right now.)
-        */
-       CurrentResourceOwner = old_resowner;
-
        if (ctx->reader->EndRecPtr != InvalidXLogRecPtr)
        {
            LogicalConfirmReceivedLocation(moveto);

diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index ca53caac2f2f5cb0766774631c2666bc4d56a1ac..25f890ddeedacea3ec4b4d767611bd42f8b7d82f 100644 (file)
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -107,7 +107,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
    XLogRecPtr  end_of_wal;
    XLogRecPtr  wait_for_wal_lsn;
    LogicalDecodingContext *ctx;
-   ResourceOwner old_resowner = CurrentResourceOwner;
+   ResourceOwner old_resowner PG_USED_FOR_ASSERTS_ONLY = CurrentResourceOwner;
    ArrayType  *arr;
    Size        ndim;
    List       *options = NIL;
@@ -263,8 +263,18 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
             * store the description into our tuplestore.
             */
            if (record != NULL)
+           {
                LogicalDecodingProcessRecord(ctx, ctx->reader);
 
+               /*
+                * We used to have bugs where logical decoding would fail to
+                * preserve the resource owner.  Verify that that doesn't
+                * happen anymore.  XXX this could be removed once it's been
+                * battle-tested.
+                */
+               Assert(CurrentResourceOwner == old_resowner);
+           }
+
            /* check limits */
            if (upto_lsn != InvalidXLogRecPtr &&
                upto_lsn <= ctx->reader->EndRecPtr)
@@ -275,13 +285,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
            CHECK_FOR_INTERRUPTS();
        }
 
-       /*
-        * Logical decoding could have clobbered CurrentResourceOwner during
-        * transaction management, so restore the executor's value.  (This is
-        * a kluge, but it's not worth cleaning up right now.)
-        */
-       CurrentResourceOwner = old_resowner;
-
        /*
         * Next time, start where we left off. (Hunting things, the family
         * business..)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 34cf05668ae84bb753904b06ba5c0e4bb903e440..4736f993c374373e992fdcf395cfd68975371461 100644 (file)
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2215,6 +2215,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
 {
    bool        using_subtxn;
    MemoryContext ccxt = CurrentMemoryContext;
+   ResourceOwner cowner = CurrentResourceOwner;
    ReorderBufferIterTXNState *volatile iterstate = NULL;
    volatile XLogRecPtr prev_lsn = InvalidXLogRecPtr;
    ReorderBufferChange *volatile specinsert = NULL;
@@ -2692,7 +2693,11 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
        }
 
        if (using_subtxn)
+       {
            RollbackAndReleaseCurrentSubTransaction();
+           MemoryContextSwitchTo(ccxt);
+           CurrentResourceOwner = cowner;
+       }
 
        /*
         * We are here due to one of the four reasons: 1. Decoding an
@@ -2751,7 +2756,11 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
        }
 
        if (using_subtxn)
+       {
            RollbackAndReleaseCurrentSubTransaction();
+           MemoryContextSwitchTo(ccxt);
+           CurrentResourceOwner = cowner;
+       }
 
        /*
         * The error code ERRCODE_TRANSACTION_ROLLBACK indicates a concurrent
@@ -3244,6 +3253,8 @@ ReorderBufferImmediateInvalidation(ReorderBuffer *rb, uint32 ninvalidations,
                                   SharedInvalidationMessage *invalidations)
 {
    bool        use_subtxn = IsTransactionOrTransactionBlock();
+   MemoryContext ccxt = CurrentMemoryContext;
+   ResourceOwner cowner = CurrentResourceOwner;
    int         i;
 
    if (use_subtxn)
@@ -3262,7 +3273,11 @@ ReorderBufferImmediateInvalidation(ReorderBuffer *rb, uint32 ninvalidations,
        LocalExecuteInvalidationMessage(&invalidations[i]);
 
    if (use_subtxn)
+   {
        RollbackAndReleaseCurrentSubTransaction();
+       MemoryContextSwitchTo(ccxt);
+       CurrentResourceOwner = cowner;
+   }
 }
 
 /*