Skip to content

Commit 07cefdf

Browse files
alvherrealvherre
committed
Fix snapshot management, take two.
Partially revert the previous patch I installed and replace it with a more general fix: any time a snapshot is pushed as Active, we need to ensure that it will not be modified in the future. This means that if the same snapshot is used as CurrentSnapshot, it needs to be copied separately. This affects serializable transactions only, because CurrentSnapshot has already been copied by RegisterSnapshot and so PushActiveSnapshot does not think it needs another copy. However, CommandCounterIncrement would modify CurrentSnapshot, whereas ActiveSnapshots must not have their command counters incremented. I say "partially" because the regression test I added for the previous bug has been kept. (This restores 8.3 behavior, because before snapmgr.c existed, any snapshot set as Active was copied.) Per bug report from Stuart Bishop in 6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com
1 parent 603e72b commit 07cefdf

File tree

5 files changed

+80
-19
lines changed

5 files changed

+80
-19
lines changed

‎src/backend/commands/portalcmds.c

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
*
1515
*
1616
* IDENTIFICATION
17-
* $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.80 2009/10/02 17:57:29 alvherre Exp $
17+
* $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.81 2009/10/07 16:27:18 alvherre Exp $
1818
*
1919
*-------------------------------------------------------------------------
2020
*/
@@ -47,7 +47,6 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
4747
DeclareCursorStmt *cstmt = (DeclareCursorStmt *) stmt->utilityStmt;
4848
Portal portal;
4949
MemoryContext oldContext;
50-
Snapshot snapshot;
5150

5251
if (cstmt == NULL || !IsA(cstmt, DeclareCursorStmt))
5352
elog(ERROR, "PerformCursorOpen called for non-cursor query");
@@ -119,18 +118,10 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
119118
portal->cursorOptions |= CURSOR_OPT_NO_SCROLL;
120119
}
121120

122-
/*
123-
* Set up snapshot for portal. Note that we need a fresh, independent copy
124-
* of the snapshot because we don't want it to be modified by future
125-
* CommandCounterIncrement calls. We do not register it, because
126-
* portalmem.c will take care of that internally.
127-
*/
128-
snapshot = CopySnapshot(GetActiveSnapshot());
129-
130121
/*
131122
* Start execution, inserting parameters if any.
132123
*/
133-
PortalStart(portal, params, snapshot);
124+
PortalStart(portal, params, GetActiveSnapshot());
134125

135126
Assert(portal->strategy == PORTAL_ONE_SELECT);
136127

‎src/backend/utils/time/snapmgr.c

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
* Portions Copyright (c) 1994, Regents of the University of California
2020
*
2121
* IDENTIFICATION
22-
* $PostgreSQL: pgsql/src/backend/utils/time/snapmgr.c,v 1.11 2009/10/02 17:57:30 alvherre Exp $
22+
* $PostgreSQL: pgsql/src/backend/utils/time/snapmgr.c,v 1.12 2009/10/07 16:27:18 alvherre Exp $
2323
*
2424
*-------------------------------------------------------------------------
2525
*/
@@ -104,6 +104,7 @@ bool FirstSnapshotSet = false;
104104
static bool registered_serializable = false;
105105

106106

107+
static Snapshot CopySnapshot(Snapshot snapshot);
107108
static void FreeSnapshot(Snapshot snapshot);
108109
static void SnapshotResetXmin(void);
109110

@@ -191,7 +192,7 @@ SnapshotSetCommandId(CommandId curcid)
191192
* The copy is palloc'd in TopTransactionContext and has initial refcounts set
192193
* to 0. The returned snapshot has the copied flag set.
193194
*/
194-
Snapshot
195+
static Snapshot
195196
CopySnapshot(Snapshot snapshot)
196197
{
197198
Snapshot newsnap;
@@ -254,8 +255,9 @@ FreeSnapshot(Snapshot snapshot)
254255
* PushActiveSnapshot
255256
* Set the given snapshot as the current active snapshot
256257
*
257-
* If this is the first use of this snapshot, create a new long-lived copy with
258-
* active refcount=1. Otherwise, only increment the refcount.
258+
* If the passed snapshot is a statically-allocated one, or it is possibly
259+
* subject to a future command counter update, create a new long-lived copy
260+
* with active refcount=1. Otherwise, only increment the refcount.
259261
*/
260262
void
261263
PushActiveSnapshot(Snapshot snap)
@@ -265,8 +267,16 @@ PushActiveSnapshot(Snapshot snap)
265267
Assert(snap != InvalidSnapshot);
266268

267269
newactive = MemoryContextAlloc(TopTransactionContext, sizeof(ActiveSnapshotElt));
268-
/* Static snapshot? Create a persistent copy */
269-
newactive->as_snap = snap->copied ? snap : CopySnapshot(snap);
270+
271+
/*
272+
* Checking SecondarySnapshot is probably useless here, but it seems better
273+
* to be sure.
274+
*/
275+
if (snap == CurrentSnapshot || snap == SecondarySnapshot || !snap->copied)
276+
newactive->as_snap = CopySnapshot(snap);
277+
else
278+
newactive->as_snap = snap;
279+
270280
newactive->as_next = ActiveSnapshot;
271281
newactive->as_level = GetCurrentTransactionNestLevel();
272282

‎src/include/utils/snapmgr.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
9-
* $PostgreSQL: pgsql/src/include/utils/snapmgr.h,v 1.6 2009/10/02 17:57:30 alvherre Exp $
9+
* $PostgreSQL: pgsql/src/include/utils/snapmgr.h,v 1.7 2009/10/07 16:27:18 alvherre Exp $
1010
*
1111
*-------------------------------------------------------------------------
1212
*/
@@ -26,7 +26,6 @@ extern TransactionId RecentGlobalXmin;
2626
extern Snapshot GetTransactionSnapshot(void);
2727
extern Snapshot GetLatestSnapshot(void);
2828
extern void SnapshotSetCommandId(CommandId curcid);
29-
extern Snapshot CopySnapshot(Snapshot snapshot);
3029

3130
extern void PushActiveSnapshot(Snapshot snapshot);
3231
extern void PushUpdatedSnapshot(Snapshot snapshot);

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,37 @@ NOTICE: row 1 not changed
537537
NOTICE: row 2 not changed
538538
DROP TABLE trigger_test;
539539
DROP FUNCTION mytrigger();
540+
-- Test snapshot management in serializable transactions involving triggers
541+
-- per bug report in 6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com
542+
CREATE FUNCTION serializable_update_trig() RETURNS trigger LANGUAGE plpgsql AS
543+
$$
544+
declare
545+
rec record;
546+
begin
547+
new.description = 'updated in trigger';
548+
return new;
549+
end;
550+
$$;
551+
CREATE TABLE serializable_update_tab (
552+
id int,
553+
filler text,
554+
description text
555+
);
556+
CREATE TRIGGER serializable_update_trig BEFORE UPDATE ON serializable_update_tab
557+
FOR EACH ROW EXECUTE PROCEDURE serializable_update_trig();
558+
INSERT INTO serializable_update_tab SELECT a, repeat('xyzxz', 100), 'new'
559+
FROM generate_series(1, 50) a;
560+
BEGIN;
561+
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
562+
UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
563+
COMMIT;
564+
SELECT description FROM serializable_update_tab WHERE id = 1;
565+
description
566+
--------------------
567+
updated in trigger
568+
(1 row)
569+
570+
DROP TABLE serializable_update_tab;
540571
-- minimal update trigger
541572
CREATE TABLE min_updates_test (
542573
f1 text,

‎src/test/regress/sql/triggers.sql

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,36 @@ DROP TABLE trigger_test;
416416

417417
DROP FUNCTION mytrigger();
418418

419+
-- Test snapshot management in serializable transactions involving triggers
420+
-- per bug report in 6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com
421+
CREATE FUNCTION serializable_update_trig() RETURNS trigger LANGUAGE plpgsql AS
422+
$$
423+
declare
424+
rec record;
425+
begin
426+
new.description = 'updated in trigger';
427+
return new;
428+
end;
429+
$$;
430+
431+
CREATE TABLE serializable_update_tab (
432+
id int,
433+
filler text,
434+
description text
435+
);
436+
437+
CREATE TRIGGER serializable_update_trig BEFORE UPDATE ON serializable_update_tab
438+
FOR EACH ROW EXECUTE PROCEDURE serializable_update_trig();
439+
440+
INSERT INTO serializable_update_tab SELECT a, repeat('xyzxz', 100), 'new'
441+
FROM generate_series(1, 50) a;
442+
443+
BEGIN;
444+
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
445+
UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
446+
COMMIT;
447+
SELECT description FROM serializable_update_tab WHERE id = 1;
448+
DROP TABLE serializable_update_tab;
419449

420450
-- minimal update trigger
421451

0 commit comments

Comments
 (0)