Skip to content

Commit 5fa6b0d

Browse files
tglsfdctglsfdc
committed
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
1 parent f676616 commit 5fa6b0d

File tree

7 files changed

+42
-116
lines changed

7 files changed

+42
-116
lines changed

‎src/backend/access/transam/xact.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -575,18 +575,10 @@ AssignTransactionId(TransactionState s)
575575
* ResourceOwner.
576576
*/
577577
currentOwner = CurrentResourceOwner;
578-
PG_TRY();
579-
{
580-
CurrentResourceOwner = s->curTransactionOwner;
581-
XactLockTableInsert(s->transactionId);
582-
}
583-
PG_CATCH();
584-
{
585-
/* Ensure CurrentResourceOwner is restored on error */
586-
CurrentResourceOwner = currentOwner;
587-
PG_RE_THROW();
588-
}
589-
PG_END_TRY();
578+
CurrentResourceOwner = s->curTransactionOwner;
579+
580+
XactLockTableInsert(s->transactionId);
581+
590582
CurrentResourceOwner = currentOwner;
591583

592584
/*

‎src/backend/commands/portalcmds.c

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -294,21 +294,13 @@ PortalCleanup(Portal portal)
294294

295295
/* We must make the portal's resource owner current */
296296
saveResourceOwner = CurrentResourceOwner;
297-
PG_TRY();
298-
{
299-
if (portal->resowner)
300-
CurrentResourceOwner = portal->resowner;
301-
ExecutorFinish(queryDesc);
302-
ExecutorEnd(queryDesc);
303-
FreeQueryDesc(queryDesc);
304-
}
305-
PG_CATCH();
306-
{
307-
/* Ensure CurrentResourceOwner is restored on error */
308-
CurrentResourceOwner = saveResourceOwner;
309-
PG_RE_THROW();
310-
}
311-
PG_END_TRY();
297+
if (portal->resowner)
298+
CurrentResourceOwner = portal->resowner;
299+
300+
ExecutorFinish(queryDesc);
301+
ExecutorEnd(queryDesc);
302+
FreeQueryDesc(queryDesc);
303+
312304
CurrentResourceOwner = saveResourceOwner;
313305
}
314306
}

‎src/backend/commands/sequence.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,18 +1055,10 @@ lock_and_open_sequence(SeqTable seq)
10551055
ResourceOwner currentOwner;
10561056

10571057
currentOwner = CurrentResourceOwner;
1058-
PG_TRY();
1059-
{
1060-
CurrentResourceOwner = TopTransactionResourceOwner;
1061-
LockRelationOid(seq->relid, RowExclusiveLock);
1062-
}
1063-
PG_CATCH();
1064-
{
1065-
/* Ensure CurrentResourceOwner is restored on error */
1066-
CurrentResourceOwner = currentOwner;
1067-
PG_RE_THROW();
1068-
}
1069-
PG_END_TRY();
1058+
CurrentResourceOwner = TopTransactionResourceOwner;
1059+
1060+
LockRelationOid(seq->relid, RowExclusiveLock);
1061+
10701062
CurrentResourceOwner = currentOwner;
10711063

10721064
/* Flag that we have a lock in the current xact */

‎src/backend/commands/trigger.c

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3639,17 +3639,10 @@ GetCurrentFDWTuplestore(void)
36393639
*/
36403640
oldcxt = MemoryContextSwitchTo(CurTransactionContext);
36413641
saveResourceOwner = CurrentResourceOwner;
3642-
PG_TRY();
3643-
{
3644-
CurrentResourceOwner = CurTransactionResourceOwner;
3645-
ret = tuplestore_begin_heap(false, false, work_mem);
3646-
}
3647-
PG_CATCH();
3648-
{
3649-
CurrentResourceOwner = saveResourceOwner;
3650-
PG_RE_THROW();
3651-
}
3652-
PG_END_TRY();
3642+
CurrentResourceOwner = CurTransactionResourceOwner;
3643+
3644+
ret = tuplestore_begin_heap(false, false, work_mem);
3645+
36533646
CurrentResourceOwner = saveResourceOwner;
36543647
MemoryContextSwitchTo(oldcxt);
36553648

@@ -4468,20 +4461,13 @@ MakeTransitionCaptureState(TriggerDesc *trigdesc, Oid relid, CmdType cmdType)
44684461
/* Now create required tuplestore(s), if we don't have them already. */
44694462
oldcxt = MemoryContextSwitchTo(CurTransactionContext);
44704463
saveResourceOwner = CurrentResourceOwner;
4471-
PG_TRY();
4472-
{
4473-
CurrentResourceOwner = CurTransactionResourceOwner;
4474-
if (need_old && table->old_tuplestore == NULL)
4475-
table->old_tuplestore = tuplestore_begin_heap(false, false, work_mem);
4476-
if (need_new && table->new_tuplestore == NULL)
4477-
table->new_tuplestore = tuplestore_begin_heap(false, false, work_mem);
4478-
}
4479-
PG_CATCH();
4480-
{
4481-
CurrentResourceOwner = saveResourceOwner;
4482-
PG_RE_THROW();
4483-
}
4484-
PG_END_TRY();
4464+
CurrentResourceOwner = CurTransactionResourceOwner;
4465+
4466+
if (need_old && table->old_tuplestore == NULL)
4467+
table->old_tuplestore = tuplestore_begin_heap(false, false, work_mem);
4468+
if (need_new && table->new_tuplestore == NULL)
4469+
table->new_tuplestore = tuplestore_begin_heap(false, false, work_mem);
4470+
44854471
CurrentResourceOwner = saveResourceOwner;
44864472
MemoryContextSwitchTo(oldcxt);
44874473

‎src/backend/storage/large_object/inv_api.c

Lines changed: 13 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -75,23 +75,14 @@ open_lo_relation(void)
7575

7676
/* Arrange for the top xact to own these relation references */
7777
currentOwner = CurrentResourceOwner;
78-
PG_TRY();
79-
{
80-
CurrentResourceOwner = TopTransactionResourceOwner;
78+
CurrentResourceOwner = TopTransactionResourceOwner;
79+
80+
/* Use RowExclusiveLock since we might either read or write */
81+
if (lo_heap_r == NULL)
82+
lo_heap_r = heap_open(LargeObjectRelationId, RowExclusiveLock);
83+
if (lo_index_r == NULL)
84+
lo_index_r = index_open(LargeObjectLOidPNIndexId, RowExclusiveLock);
8185

82-
/* Use RowExclusiveLock since we might either read or write */
83-
if (lo_heap_r == NULL)
84-
lo_heap_r = heap_open(LargeObjectRelationId, RowExclusiveLock);
85-
if (lo_index_r == NULL)
86-
lo_index_r = index_open(LargeObjectLOidPNIndexId, RowExclusiveLock);
87-
}
88-
PG_CATCH();
89-
{
90-
/* Ensure CurrentResourceOwner is restored on error */
91-
CurrentResourceOwner = currentOwner;
92-
PG_RE_THROW();
93-
}
94-
PG_END_TRY();
9586
CurrentResourceOwner = currentOwner;
9687
}
9788

@@ -112,22 +103,13 @@ close_lo_relation(bool isCommit)
112103
ResourceOwner currentOwner;
113104

114105
currentOwner = CurrentResourceOwner;
115-
PG_TRY();
116-
{
117-
CurrentResourceOwner = TopTransactionResourceOwner;
106+
CurrentResourceOwner = TopTransactionResourceOwner;
107+
108+
if (lo_index_r)
109+
index_close(lo_index_r, NoLock);
110+
if (lo_heap_r)
111+
heap_close(lo_heap_r, NoLock);
118112

119-
if (lo_index_r)
120-
index_close(lo_index_r, NoLock);
121-
if (lo_heap_r)
122-
heap_close(lo_heap_r, NoLock);
123-
}
124-
PG_CATCH();
125-
{
126-
/* Ensure CurrentResourceOwner is restored on error */
127-
CurrentResourceOwner = currentOwner;
128-
PG_RE_THROW();
129-
}
130-
PG_END_TRY();
131113
CurrentResourceOwner = currentOwner;
132114
}
133115
lo_heap_r = NULL;

‎src/backend/utils/resowner/README

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,3 @@ CurrentResourceOwner must point to the same resource owner that was current
8080
when the buffer, lock, or cache reference was acquired. It would be possible
8181
to relax this restriction given additional bookkeeping effort, but at present
8282
there seems no need.
83-
84-
Code that transiently changes CurrentResourceOwner should generally use a
85-
PG_TRY construct to ensure that the previous value of CurrentResourceOwner
86-
is restored if control is lost during an error exit.

‎src/backend/utils/resowner/resowner.c

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -473,21 +473,8 @@ ResourceOwnerRelease(ResourceOwner owner,
473473
bool isCommit,
474474
bool isTopLevel)
475475
{
476-
/* Rather than PG_TRY at every level of recursion, set it up once */
477-
ResourceOwner save;
478-
479-
save = CurrentResourceOwner;
480-
PG_TRY();
481-
{
482-
ResourceOwnerReleaseInternal(owner, phase, isCommit, isTopLevel);
483-
}
484-
PG_CATCH();
485-
{
486-
CurrentResourceOwner = save;
487-
PG_RE_THROW();
488-
}
489-
PG_END_TRY();
490-
CurrentResourceOwner = save;
476+
/* There's not currently any setup needed before recursing */
477+
ResourceOwnerReleaseInternal(owner, phase, isCommit, isTopLevel);
491478
}
492479

493480
static void
@@ -507,8 +494,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
507494

508495
/*
509496
* Make CurrentResourceOwner point to me, so that ReleaseBuffer etc don't
510-
* get confused. We needn't PG_TRY here because the outermost level will
511-
* fix it on error abort.
497+
* get confused.
512498
*/
513499
save = CurrentResourceOwner;
514500
CurrentResourceOwner = owner;

0 commit comments

Comments
 (0)