Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit81ee435

Browse files
committed
Fix subtransaction cleanup after an outer-subtransaction portal fails.
Formerly, we treated only portals created in the current subtransaction ashaving failed during subtransaction abort. However, if the error occurredwhile running a portal created in an outer subtransaction (ie, a cursordeclared before the last savepoint), that has to be considered broken too.To allow reliable detection of which ones those are, add a bookkeepingfield to struct Portal that tracks the innermost subtransaction in whicheach portal has actually been executed. (Without this, we'd end upfailing portals containing functions that had called the subtransaction,thereby breaking plpgsql exception blocks completely.)In addition, when we fail an outer-subtransaction Portal, transfer itsresources into the subtransaction's resource owner, so that they'rereleased early in cleanup of the subxact. This fixes a problem reported byJim Nasby in which a function executed in an outer-subtransaction cursorcould cause an Assert failure or crash by referencing a relation createdwithin the inner subtransaction.The proximate cause of the Assert failure is that AtEOSubXact_RelationCacheassumed it could blow away a relcache entry without first checking that theentry had zero refcount. That was a bad idea on its own terms, so add sucha check there, and to the similar coding in AtEOXact_RelationCache. Thisprovides an independent safety measure in case there are still ways toprovoke the situation despite the Portal-level changes.This has been broken since subtransactions were invented, so back-patchto all supported branches.Tom Lane and Michael Paquier
1 parentb5a22d8 commit81ee435

File tree

8 files changed

+192
-14
lines changed

8 files changed

+192
-14
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4101,6 +4101,7 @@ AbortSubTransaction(void)
41014101
AfterTriggerEndSubXact(false);
41024102
AtSubAbort_Portals(s->subTransactionId,
41034103
s->parent->subTransactionId,
4104+
s->curTransactionOwner,
41044105
s->parent->curTransactionOwner);
41054106
AtEOSubXact_LargeObject(false,s->subTransactionId,
41064107
s->parent->subTransactionId);

‎src/backend/commands/portalcmds.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,7 @@ PersistHoldablePortal(Portal portal)
329329
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
330330
errmsg("portal \"%s\" cannot be run",portal->name)));
331331
portal->status=PORTAL_ACTIVE;
332+
portal->activeSubid=GetCurrentSubTransactionId();
332333

333334
/*
334335
* Set up global portal context pointers.

‎src/backend/tcop/pquery.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,7 @@ PortalRun(Portal portal, long count, bool isTopLevel,
748748
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
749749
errmsg("portal \"%s\" cannot be run",portal->name)));
750750
portal->status=PORTAL_ACTIVE;
751+
portal->activeSubid=GetCurrentSubTransactionId();
751752

752753
/*
753754
* Set up global portal context pointers.
@@ -1372,6 +1373,7 @@ PortalRunFetch(Portal portal,
13721373
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
13731374
errmsg("portal \"%s\" cannot be run",portal->name)));
13741375
portal->status=PORTAL_ACTIVE;
1376+
portal->activeSubid=GetCurrentSubTransactionId();
13751377

13761378
/*
13771379
* Set up global portal context pointers.

‎src/backend/utils/cache/relcache.c

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1855,7 +1855,9 @@ RelationClearRelation(Relation relation, bool rebuild)
18551855
{
18561856
/*
18571857
* As per notes above, a rel to be rebuilt MUST have refcnt > 0; while of
1858-
* course it would be a bad idea to blow away one with nonzero refcnt.
1858+
* course it would be an equally bad idea to blow away one with nonzero
1859+
* refcnt, since that would leave someone somewhere with a dangling
1860+
* pointer. All callers are expected to have verified that this holds.
18591861
*/
18601862
Assert(rebuild ?
18611863
!RelationHasReferenceCountZero(relation) :
@@ -2345,11 +2347,25 @@ AtEOXact_RelationCache(bool isCommit)
23452347
{
23462348
if (isCommit)
23472349
relation->rd_createSubid=InvalidSubTransactionId;
2348-
else
2350+
elseif (RelationHasReferenceCountZero(relation))
23492351
{
23502352
RelationClearRelation(relation, false);
23512353
continue;
23522354
}
2355+
else
2356+
{
2357+
/*
2358+
* Hmm, somewhere there's a (leaked?) reference to the
2359+
* relation. We daren't remove the entry for fear of
2360+
* dereferencing a dangling pointer later. Bleat, and mark it
2361+
* as not belonging to the current transaction. Hopefully
2362+
* it'll get cleaned up eventually. This must be just a
2363+
* WARNING to avoid error-during-error-recovery loops.
2364+
*/
2365+
relation->rd_createSubid=InvalidSubTransactionId;
2366+
elog(WARNING,"cannot remove relcache entry for \"%s\" because it has nonzero refcount",
2367+
RelationGetRelationName(relation));
2368+
}
23532369
}
23542370

23552371
/*
@@ -2410,11 +2426,25 @@ AtEOSubXact_RelationCache(bool isCommit, SubTransactionId mySubid,
24102426
{
24112427
if (isCommit)
24122428
relation->rd_createSubid=parentSubid;
2413-
else
2429+
elseif (RelationHasReferenceCountZero(relation))
24142430
{
24152431
RelationClearRelation(relation, false);
24162432
continue;
24172433
}
2434+
else
2435+
{
2436+
/*
2437+
* Hmm, somewhere there's a (leaked?) reference to the
2438+
* relation. We daren't remove the entry for fear of
2439+
* dereferencing a dangling pointer later. Bleat, and
2440+
* transfer it to the parent subtransaction so we can try
2441+
* again later. This must be just a WARNING to avoid
2442+
* error-during-error-recovery loops.
2443+
*/
2444+
relation->rd_createSubid=parentSubid;
2445+
elog(WARNING,"cannot remove relcache entry for \"%s\" because it has nonzero refcount",
2446+
RelationGetRelationName(relation));
2447+
}
24182448
}
24192449

24202450
/*

‎src/backend/utils/mmgr/portalmem.c

Lines changed: 63 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ CreatePortal(const char *name, bool allowDup, bool dupSilent)
231231
portal->status=PORTAL_NEW;
232232
portal->cleanup=PortalCleanup;
233233
portal->createSubid=GetCurrentSubTransactionId();
234+
portal->activeSubid=portal->createSubid;
234235
portal->strategy=PORTAL_MULTI_QUERY;
235236
portal->cursorOptions=CURSOR_OPT_NO_SCROLL;
236237
portal->atStart= true;
@@ -581,6 +582,7 @@ CommitHoldablePortals(void)
581582
* not belonging to this transaction.
582583
*/
583584
portal->createSubid=InvalidSubTransactionId;
585+
portal->activeSubid=InvalidSubTransactionId;
584586

585587
result= true;
586588
}
@@ -793,8 +795,8 @@ AtCleanup_Portals(void)
793795
/*
794796
* Pre-subcommit processing for portals.
795797
*
796-
* Reassigntheportals created in the current subtransaction to the parent
797-
* subtransaction.
798+
* Reassign portals createdor usedin the current subtransaction to the
799+
*parentsubtransaction.
798800
*/
799801
void
800802
AtSubCommit_Portals(SubTransactionIdmySubid,
@@ -816,21 +818,24 @@ AtSubCommit_Portals(SubTransactionId mySubid,
816818
if (portal->resowner)
817819
ResourceOwnerNewParent(portal->resowner,parentXactOwner);
818820
}
821+
if (portal->activeSubid==mySubid)
822+
portal->activeSubid=parentSubid;
819823
}
820824
}
821825

822826
/*
823827
* Subtransaction abort handling for portals.
824828
*
825-
* Deactivate portals created during the failed subtransaction.
826-
* Note that per AtSubCommit_Portals, this will catch portals created
829+
* Deactivate portals createdor usedduring the failed subtransaction.
830+
* Note that per AtSubCommit_Portals, this will catch portals created/used
827831
* in descendants of the subtransaction too.
828832
*
829833
* We don't destroy any portals here; that's done in AtSubCleanup_Portals.
830834
*/
831835
void
832836
AtSubAbort_Portals(SubTransactionIdmySubid,
833837
SubTransactionIdparentSubid,
838+
ResourceOwnermyXactOwner,
834839
ResourceOwnerparentXactOwner)
835840
{
836841
HASH_SEQ_STATUSstatus;
@@ -842,16 +847,66 @@ AtSubAbort_Portals(SubTransactionId mySubid,
842847
{
843848
Portalportal=hentry->portal;
844849

850+
/* Was it created in this subtransaction? */
845851
if (portal->createSubid!=mySubid)
852+
{
853+
/* No, but maybe it was used in this subtransaction? */
854+
if (portal->activeSubid==mySubid)
855+
{
856+
/* Maintain activeSubid until the portal is removed */
857+
portal->activeSubid=parentSubid;
858+
859+
/*
860+
* Upper-level portals that failed while running in this
861+
* subtransaction must be forced into FAILED state, for the
862+
* same reasons discussed below.
863+
*
864+
* We assume we can get away without forcing upper-level READY
865+
* portals to fail, even if they were run and then suspended.
866+
* In theory a suspended upper-level portal could have
867+
* acquired some references to objects that are about to be
868+
* destroyed, but there should be sufficient defenses against
869+
* such cases: the portal's original query cannot contain such
870+
* references, and any references within, say, cached plans of
871+
* PL/pgSQL functions are not from active queries and should
872+
* be protected by revalidation logic.
873+
*/
874+
if (portal->status==PORTAL_ACTIVE)
875+
{
876+
portal->status=PORTAL_FAILED;
877+
/* let portalcmds.c clean up the state it knows about */
878+
if (PointerIsValid(portal->cleanup))
879+
{
880+
(*portal->cleanup) (portal);
881+
portal->cleanup=NULL;
882+
}
883+
}
884+
885+
/*
886+
* Also, if we failed it during the current subtransaction
887+
* (either just above, or earlier), reattach its resource
888+
* owner to the current subtransaction's resource owner, so
889+
* that any resources it still holds will be released while
890+
* cleaning up this subtransaction. This prevents some corner
891+
* cases wherein we might get Asserts or worse while cleaning
892+
* up objects created during the current subtransaction
893+
* (because they're still referenced within this portal).
894+
*/
895+
if (portal->status==PORTAL_FAILED&&portal->resowner)
896+
{
897+
ResourceOwnerNewParent(portal->resowner,myXactOwner);
898+
portal->resowner=NULL;
899+
}
900+
}
901+
/* Done if it wasn't created in this subtransaction */
846902
continue;
903+
}
847904

848905
/*
849906
* Force any live portals of my own subtransaction into FAILED state.
850907
* We have to do this because they might refer to objects created or
851-
* changed in the failed subtransaction, leading to crashes if
852-
* execution is resumed, or even if we just try to run ExecutorEnd.
853-
* (Note we do NOT do this to upper-level portals, since they cannot
854-
* have such references and hence may be able to continue.)
908+
* changed in the failed subtransaction, leading to crashes within
909+
* ExecutorEnd when portalcmds.c tries to close down the portal.
855910
*/
856911
if (portal->status==PORTAL_READY||
857912
portal->status==PORTAL_ACTIVE)

‎src/include/utils/portal.h

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,12 +112,15 @@ typedef struct PortalData
112112
MemoryContextheap;/* subsidiary memory for portal */
113113
ResourceOwnerresowner;/* resources owned by portal */
114114
void(*cleanup) (Portalportal);/* cleanup hook */
115-
SubTransactionIdcreateSubid;/* the ID of the creating subxact */
116115

117116
/*
118-
* if createSubid is InvalidSubTransactionId, the portal is held over from
119-
* a previous transaction
117+
* State data for remembering which subtransaction(s) the portal was
118+
* created or used in. If the portal is held over from a previous
119+
* transaction, both subxids are InvalidSubTransactionId. Otherwise,
120+
* createSubid is the creating subxact and activeSubid is the last subxact
121+
* in which we ran the portal.
120122
*/
123+
SubTransactionIdcreateSubid;/* the creating subxact */
121124

122125
/* The query or queries the portal will execute */
123126
constchar*sourceText;/* text of query (as of 8.4, never NULL) */
@@ -168,6 +171,13 @@ typedef struct PortalData
168171
/* Presentation data, primarily used by the pg_cursors system view */
169172
TimestampTzcreation_time;/* time at which this portal was defined */
170173
boolvisible;/* include this portal in pg_cursors? */
174+
175+
/*
176+
* This field belongs with createSubid, but in pre-9.5 branches, add it
177+
* at the end to avoid creating an ABI break for extensions that examine
178+
* Portal structs.
179+
*/
180+
SubTransactionIdactiveSubid;/* the last subxact with activity */
171181
}PortalData;
172182

173183
/*
@@ -196,6 +206,7 @@ extern void AtSubCommit_Portals(SubTransactionId mySubid,
196206
ResourceOwnerparentXactOwner);
197207
externvoidAtSubAbort_Portals(SubTransactionIdmySubid,
198208
SubTransactionIdparentSubid,
209+
ResourceOwnermyXactOwner,
199210
ResourceOwnerparentXactOwner);
200211
externvoidAtSubCleanup_Portals(SubTransactionIdmySubid);
201212
externPortalCreatePortal(constchar*name,boolallowDup,booldupSilent);

‎src/test/regress/expected/transactions.out

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,52 @@ fetch from foo;
527527
(1 row)
528528

529529
abort;
530+
-- Test for proper cleanup after a failure in a cursor portal
531+
-- that was created in an outer subtransaction
532+
CREATE FUNCTION invert(x float8) RETURNS float8 LANGUAGE plpgsql AS
533+
$$ begin return 1/x; end $$;
534+
CREATE FUNCTION create_temp_tab() RETURNS text
535+
LANGUAGE plpgsql AS $$
536+
BEGIN
537+
CREATE TEMP TABLE new_table (f1 float8);
538+
-- case of interest is that we fail while holding an open
539+
-- relcache reference to new_table
540+
INSERT INTO new_table SELECT invert(0.0);
541+
RETURN 'foo';
542+
END $$;
543+
BEGIN;
544+
DECLARE ok CURSOR FOR SELECT * FROM int8_tbl;
545+
DECLARE ctt CURSOR FOR SELECT create_temp_tab();
546+
FETCH ok;
547+
q1 | q2
548+
-----+-----
549+
123 | 456
550+
(1 row)
551+
552+
SAVEPOINT s1;
553+
FETCH ok; -- should work
554+
q1 | q2
555+
-----+------------------
556+
123 | 4567890123456789
557+
(1 row)
558+
559+
FETCH ctt; -- error occurs here
560+
ERROR: division by zero
561+
CONTEXT: PL/pgSQL function "invert" line 1 at RETURN
562+
SQL statement "INSERT INTO new_table SELECT invert(0.0)"
563+
PL/pgSQL function "create_temp_tab" line 5 at SQL statement
564+
ROLLBACK TO s1;
565+
FETCH ok; -- should work
566+
q1 | q2
567+
------------------+-----
568+
4567890123456789 | 123
569+
(1 row)
570+
571+
FETCH ctt; -- must be rejected
572+
ERROR: portal "ctt" cannot be run
573+
COMMIT;
574+
DROP FUNCTION create_temp_tab();
575+
DROP FUNCTION invert(x float8);
530576
-- tests for the "tid" type
531577
SELECT '(3, 3)'::tid = '(3, 4)'::tid;
532578
?column?

‎src/test/regress/sql/transactions.sql

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,38 @@ fetch from foo;
326326

327327
abort;
328328

329+
330+
-- Test for proper cleanup after a failure in a cursor portal
331+
-- that was created in an outer subtransaction
332+
CREATEFUNCTIONinvert(x float8) RETURNS float8 LANGUAGE plpgsqlAS
333+
$$begin return1/x; end $$;
334+
335+
CREATEFUNCTIONcreate_temp_tab() RETURNStext
336+
LANGUAGE plpgsqlAS $$
337+
BEGIN
338+
CREATE TEMP TABLE new_table (f1 float8);
339+
-- case of interest is that we fail while holding an open
340+
-- relcache reference to new_table
341+
INSERT INTO new_tableSELECT invert(0.0);
342+
RETURN'foo';
343+
END $$;
344+
345+
BEGIN;
346+
DECLARE ok CURSOR FORSELECT*FROM int8_tbl;
347+
DECLARE ctt CURSOR FORSELECT create_temp_tab();
348+
FETCH ok;
349+
SAVEPOINT s1;
350+
FETCH ok;-- should work
351+
FETCH ctt;-- error occurs here
352+
ROLLBACK TO s1;
353+
FETCH ok;-- should work
354+
FETCH ctt;-- must be rejected
355+
COMMIT;
356+
357+
DROPFUNCTION create_temp_tab();
358+
DROPFUNCTION invert(x float8);
359+
360+
329361
-- tests for the "tid" type
330362
SELECT'(3, 3)'::tid='(3, 4)'::tid;
331363
SELECT'(3, 3)'::tid='(3, 3)'::tid;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp