Remove unnecessary PG_TRY overhead for CurrentResourceOwner changes.

resowner/README contained advice to use a PG_TRY block to restore the
old CurrentResourceOwner value anywhere that that variable is transiently
changed.  That advice was only inconsistently followed, however, and
on reflection it seems like unnecessary overhead.  We don't bother
with such a convention for transient CurrentMemoryContext changes,
on the grounds that any (sub)transaction abort will start out by
resetting CurrentMemoryContext to what it wants.  But the same is
true of CurrentResourceOwner, so there seems no need to treat it
differently.

Hence, remove PG_TRY blocks that exist only to restore CurrentResourceOwner
before re-throwing the error.  There are a couple of places that restore
it along with some other actions, and I left those alone; the restore is
probably unnecessary but no noticeable gain will result from removing it.

Discussion: https://postgr.es/m/5236.1507583529@sss.pgh.pa.us

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 93dca7a72af59be806e8d47ad30292a764b8bc0e..8203388fa83e71a8c7f49687b6afc63b3b8ccc50 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -575,18 +575,10 @@ AssignTransactionId(TransactionState s)
         * ResourceOwner.
         */
        currentOwner = CurrentResourceOwner;
-       PG_TRY();
-       {
-               CurrentResourceOwner = s->curTransactionOwner;
-               XactLockTableInsert(s->transactionId);
-       }
-       PG_CATCH();
-       {
-               /* Ensure CurrentResourceOwner is restored on error */
-               CurrentResourceOwner = currentOwner;
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
+       CurrentResourceOwner = s->curTransactionOwner;
+
+       XactLockTableInsert(s->transactionId);
+
        CurrentResourceOwner = currentOwner;
 
        /*

diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index b36473fba44194e8c4638b5b7da8cf2ec33060a7..76d6cf154c66aa3e63a5a83436d4780bff2f3060 100644 (file)
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -294,21 +294,13 @@ PortalCleanup(Portal portal)
 
                        /* We must make the portal's resource owner current */
                        saveResourceOwner = CurrentResourceOwner;
-                       PG_TRY();
-                       {
-                               if (portal->resowner)
-                                       CurrentResourceOwner = portal->resowner;
-                               ExecutorFinish(queryDesc);
-                               ExecutorEnd(queryDesc);
-                               FreeQueryDesc(queryDesc);
-                       }
-                       PG_CATCH();
-                       {
-                               /* Ensure CurrentResourceOwner is restored on error */
-                               CurrentResourceOwner = saveResourceOwner;
-                               PG_RE_THROW();
-                       }
-                       PG_END_TRY();
+                       if (portal->resowner)
+                               CurrentResourceOwner = portal->resowner;
+
+                       ExecutorFinish(queryDesc);
+                       ExecutorEnd(queryDesc);
+                       FreeQueryDesc(queryDesc);
+
                        CurrentResourceOwner = saveResourceOwner;
                }
        }

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 5c2ce789468463e353d0418f5c8ad53fca7eb24e..5e1b0fe289729e65895101b62d3b0f00065efbfd 100644 (file)
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1055,18 +1055,10 @@ lock_and_open_sequence(SeqTable seq)
                ResourceOwner currentOwner;
 
                currentOwner = CurrentResourceOwner;
-               PG_TRY();
-               {
-                       CurrentResourceOwner = TopTransactionResourceOwner;
-                       LockRelationOid(seq->relid, RowExclusiveLock);
-               }
-               PG_CATCH();
-               {
-                       /* Ensure CurrentResourceOwner is restored on error */
-                       CurrentResourceOwner = currentOwner;
-                       PG_RE_THROW();
-               }
-               PG_END_TRY();
+               CurrentResourceOwner = TopTransactionResourceOwner;
+
+               LockRelationOid(seq->relid, RowExclusiveLock);
+
                CurrentResourceOwner = currentOwner;
 
                /* Flag that we have a lock in the current xact */

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index e75a59d29958ef99e323cbb9133d4bc19b1025e9..8d0345cd64e58b4b82ee58b6c8d87c8dfd6026aa 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3639,17 +3639,10 @@ GetCurrentFDWTuplestore(void)
                 */
                oldcxt = MemoryContextSwitchTo(CurTransactionContext);
                saveResourceOwner = CurrentResourceOwner;
-               PG_TRY();
-               {
-                       CurrentResourceOwner = CurTransactionResourceOwner;
-                       ret = tuplestore_begin_heap(false, false, work_mem);
-               }
-               PG_CATCH();
-               {
-                       CurrentResourceOwner = saveResourceOwner;
-                       PG_RE_THROW();
-               }
-               PG_END_TRY();
+               CurrentResourceOwner = CurTransactionResourceOwner;
+
+               ret = tuplestore_begin_heap(false, false, work_mem);
+
                CurrentResourceOwner = saveResourceOwner;
                MemoryContextSwitchTo(oldcxt);
 
@@ -4468,20 +4461,13 @@ MakeTransitionCaptureState(TriggerDesc *trigdesc, Oid relid, CmdType cmdType)
        /* Now create required tuplestore(s), if we don't have them already. */
        oldcxt = MemoryContextSwitchTo(CurTransactionContext);
        saveResourceOwner = CurrentResourceOwner;
-       PG_TRY();
-       {
-               CurrentResourceOwner = CurTransactionResourceOwner;
-               if (need_old && table->old_tuplestore == NULL)
-                       table->old_tuplestore = tuplestore_begin_heap(false, false, work_mem);
-               if (need_new && table->new_tuplestore == NULL)
-                       table->new_tuplestore = tuplestore_begin_heap(false, false, work_mem);
-       }
-       PG_CATCH();
-       {
-               CurrentResourceOwner = saveResourceOwner;
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
+       CurrentResourceOwner = CurTransactionResourceOwner;
+
+       if (need_old && table->old_tuplestore == NULL)
+               table->old_tuplestore = tuplestore_begin_heap(false, false, work_mem);
+       if (need_new && table->new_tuplestore == NULL)
+               table->new_tuplestore = tuplestore_begin_heap(false, false, work_mem);
+
        CurrentResourceOwner = saveResourceOwner;
        MemoryContextSwitchTo(oldcxt);
 

diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
index d55d40e6f8184d6b5b23015ab3417682786e2b5a..aa43b46c305f4a9cd54fdedba2b192b42f70a87f 100644 (file)
--- a/src/backend/storage/large_object/inv_api.c
+++ b/src/backend/storage/large_object/inv_api.c
@@ -75,23 +75,14 @@ open_lo_relation(void)
 
        /* Arrange for the top xact to own these relation references */
        currentOwner = CurrentResourceOwner;
-       PG_TRY();
-       {
-               CurrentResourceOwner = TopTransactionResourceOwner;
+       CurrentResourceOwner = TopTransactionResourceOwner;
+
+       /* Use RowExclusiveLock since we might either read or write */
+       if (lo_heap_r == NULL)
+               lo_heap_r = heap_open(LargeObjectRelationId, RowExclusiveLock);
+       if (lo_index_r == NULL)
+               lo_index_r = index_open(LargeObjectLOidPNIndexId, RowExclusiveLock);
 
-               /* Use RowExclusiveLock since we might either read or write */
-               if (lo_heap_r == NULL)
-                       lo_heap_r = heap_open(LargeObjectRelationId, RowExclusiveLock);
-               if (lo_index_r == NULL)
-                       lo_index_r = index_open(LargeObjectLOidPNIndexId, RowExclusiveLock);
-       }
-       PG_CATCH();
-       {
-               /* Ensure CurrentResourceOwner is restored on error */
-               CurrentResourceOwner = currentOwner;
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
        CurrentResourceOwner = currentOwner;
 }
 
@@ -112,22 +103,13 @@ close_lo_relation(bool isCommit)
                        ResourceOwner currentOwner;
 
                        currentOwner = CurrentResourceOwner;
-                       PG_TRY();
-                       {
-                               CurrentResourceOwner = TopTransactionResourceOwner;
+                       CurrentResourceOwner = TopTransactionResourceOwner;
+
+                       if (lo_index_r)
+                               index_close(lo_index_r, NoLock);
+                       if (lo_heap_r)
+                               heap_close(lo_heap_r, NoLock);
 
-                               if (lo_index_r)
-                                       index_close(lo_index_r, NoLock);
-                               if (lo_heap_r)
-                                       heap_close(lo_heap_r, NoLock);
-                       }
-                       PG_CATCH();
-                       {
-                               /* Ensure CurrentResourceOwner is restored on error */
-                               CurrentResourceOwner = currentOwner;
-                               PG_RE_THROW();
-                       }
-                       PG_END_TRY();
                        CurrentResourceOwner = currentOwner;
                }
                lo_heap_r = NULL;

diff --git a/src/backend/utils/resowner/README b/src/backend/utils/resowner/README
index e7b9ddfa59a0b510fb32c59f96cd731f7ee99a1a..2998f6bb362e11edf727969ef0cb5befa3f8580f 100644 (file)
--- a/src/backend/utils/resowner/README
+++ b/src/backend/utils/resowner/README
@@ -80,7 +80,3 @@ CurrentResourceOwner must point to the same resource owner that was current
 when the buffer, lock, or cache reference was acquired.  It would be possible
 to relax this restriction given additional bookkeeping effort, but at present
 there seems no need.
-
-Code that transiently changes CurrentResourceOwner should generally use a
-PG_TRY construct to ensure that the previous value of CurrentResourceOwner
-is restored if control is lost during an error exit.

diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index bd19fad77e952873fa4bb4d24dd8cea1ee25e7b5..4c35ccf65eb8ba61b1c970c10be15607e93da23d 100644 (file)
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -473,21 +473,8 @@ ResourceOwnerRelease(ResourceOwner owner,
                                         bool isCommit,
                                         bool isTopLevel)
 {
-       /* Rather than PG_TRY at every level of recursion, set it up once */
-       ResourceOwner save;
-
-       save = CurrentResourceOwner;
-       PG_TRY();
-       {
-               ResourceOwnerReleaseInternal(owner, phase, isCommit, isTopLevel);
-       }
-       PG_CATCH();
-       {
-               CurrentResourceOwner = save;
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
-       CurrentResourceOwner = save;
+       /* There's not currently any setup needed before recursing */
+       ResourceOwnerReleaseInternal(owner, phase, isCommit, isTopLevel);
 }
 
 static void
@@ -507,8 +494,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
 
        /*
         * Make CurrentResourceOwner point to me, so that ReleaseBuffer etc don't
-        * get confused.  We needn't PG_TRY here because the outermost level will
-        * fix it on error abort.
+        * get confused.
         */
        save = CurrentResourceOwner;
        CurrentResourceOwner = owner;