Обсуждение: So, why shouldn't SET CONSTRAINTS set a transaction snapshot?
In connection with fixing http://archives.postgresql.org/pgsql-hackers/2008-12/msg00620.php I decided to insert "Assert(ActiveSnapshotSet())" into pg_plan_query, since any path reaching that function should already have provided a snapshot. I was bemused to find that this resulted in an assert failure in the regression tests, with backtrace #4 0x480b74 in ExceptionalCondition ( conditionName=0x169630 "!(ActiveSnapshotSet())", errorType=0x169648 "FailedAssertion",fileName=0x169564 "postgres.c", lineNumber=689) at assert.c:57 #5 0x3a5a4c in pg_plan_query (querytree=0x4017d288, cursorOptions=1565885, boundParams=0x7b033ec8) at postgres.c:689 #6 0x3a5b98 in pg_plan_queries (querytrees=0x7b011c28, cursorOptions=32, boundParams=0x7f, needSnapshot=-24 '�') at postgres.c:772 #7 0x2eac74 in _SPI_prepare_plan (src=0x1 <Address 0x1 out of bounds>, plan=0x17e4bd, boundParams=0x7b03cf5a) at spi.c:1609 #8 0x2e8654 in SPI_prepare_cursor ( src=0x40172468 "SELECT 1 FROM ONLY \"public\".\"pktable\" x WHERE \"id\" OPERATOR(pg_catalog.=)$1 FOR SHARE OF x", nargs=1, argtypes=0x7b03c300, cursorOptions=0) at spi.c:499 #9 0x2e8590 in SPI_prepare (src=0x7b011c28 "", nargs=2063845116, argtypes=0x7b011cf0) at spi.c:473 #10 0x44a024 in ri_PlanCheck ( querystr=0x40172468 "SELECT 1 FROM ONLY \"public\".\"pktable\" x WHERE \"id\" OPERATOR(pg_catalog.=)$1 FOR SHARE OF x", nargs=1, argtypes=0x7b03c300, qkey=0x7b03be40, fk_rel=0x4014ba78, pk_rel=0x40142b38,cache_plan=1) at ri_triggers.c:3225 #11 0x4456fc in RI_FKey_check (fcinfo=0x7b011c28) at ri_triggers.c:486 #12 0x4457e4 in RI_FKey_check_ins (fcinfo=0x7b011c28) at ri_triggers.c:518 #13 0x2a3f70 in ExecCallTriggerFunc (trigdata=0x7b03b868, tgindx=0, finfo=0x7b03bb30, instr=0x0, per_tuple_context=0x401eaf20) at trigger.c:1591 #14 0x2a5a74 in AfterTriggerExecute (event=0x40174890, rel=0x4014ba78, trigdesc=0x7b011cf0, finfo=0x40127948, instr=0x0, per_tuple_context=0x401eaf20) at trigger.c:2777 #15 0x2a5de0 in afterTriggerInvokeEvents (events=0x40174c70, firing_id=1, estate=0x401276b8, delete_ok=1 '\001') at trigger.c:2956 #16 0x2a6f4c in AfterTriggerSetState (stmt=0x400cd9f8) at trigger.c:3729 #17 0x3aef54 in ProcessUtility (parsetree=0x400cd9f8, queryString=0x400cd1a8 "SET CONSTRAINTS ALL IMMEDIATE;", params=0x0, isTopLevel=1 '\001', dest=0x400cdbe0, completionTag=0x7b03b348 "") at utility.c:1036 #18 0x3ac9f8 in PortalRunUtility (portal=0x4011c438, utilityStmt=0x400cd9f8, isTopLevel=1 '\001', dest=0x400cdbe0, completionTag=0x7b03b348"") at pquery.c:1183 #19 0x3acba8 in PortalRunMulti (portal=0x4011c438, isTopLevel=1 '\001', dest=0x400cdbe0, altdest=0x400cdbe0, completionTag=0x7b03b348"") at pquery.c:1288 #20 0x3ac224 in PortalRun (portal=0x4011c438, count=2147483647, isTopLevel=1 '\001', dest=0x400cdbe0, altdest=0x400cdbe0, completionTag=0x7b03b348 "") at pquery.c:815 #21 0x3a5fe4 in exec_simple_query ( query_string=0x400cd1a8 "SET CONSTRAINTS ALL IMMEDIATE;") at postgres.c:1011 The problem here is that PortalRunUtility() intentionally does not set a snapshot for ConstraintsSetStmt; so if SET CONSTRAINTS IMMEDIATE results in any triggers getting fired, those triggers are run with no snapshot available. Boo hiss. The comment in PortalRunUtility asserts loudly that ConstraintsSetStmt MUST NOT have a transaction snapshot set before it is executed, but I confess that I don't see why not at the moment. We certainly can't have it not set a snap if it has any triggers to fire. Comments? regards, tom lane
I wrote: > The comment in PortalRunUtility asserts loudly that ConstraintsSetStmt > MUST NOT have a transaction snapshot set before it is executed, but > I confess that I don't see why not at the moment. We certainly can't > have it not set a snap if it has any triggers to fire. Comments? I looked into the CVS history, and found that the exclusion of ConstraintsSetStmt appeared in my commit here: http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c.diff?r1=1.304;r2=1.305 pursuant to an earlier proposal here: http://archives.postgresql.org/pgsql-hackers/2002-10/msg00457.php but that proposal doesn't mention ConstraintsSetStmt, so I'm not totally sure why I did that. I have a feeling that I just wanted to have the principle that "SET doesn't cause the transaction snapshot to become established" without distinguishing SET CONSTRAINTS from GUC-variable SET. But we document SET CONSTRAINTS as a different command anyway, so it's not clear that there's any point to treating it the same for this. It clearly is necessary for GUC SET to not set the snapshot, else SET TRANSACTION ISOLATION MODE wouldn't work properly. It's less clear that there's much of a use-case for SET CONSTRAINTS. There seem to be two ways we could fix this: 1. Always set a snapshot for SET CONSTRAINTS. This is a minus-one-liner --- just remove it from the exclusion list in PortalRunUtility. 2. Have it set a snapshot only if it finds pending trigger events to fire. This would only require another half dozen lines of code, but it's certainly more complicated than choice #1. The argument for #2 is mainly that it's certain not to create any compatibility issues, should there be apps out there that are expecting to be able to do SET CONSTRAINTS at the start of a transaction without locking down the transaction snapshot. (The only way there could be pending trigger events is if we already did a DML command in the current xact, so the transaction snap has certainly been set.) While it's not a lot of code, the case we're protecting seems like a mighty narrow corner case that might very well not exist in the field anyplace --- it's definitely not promised to work in the documentation. Any strong opinions one way or the other? regards, tom lane
Tom Lane wrote: > 1. Always set a snapshot for SET CONSTRAINTS. This is a minus-one-liner > --- just remove it from the exclusion list in PortalRunUtility. > > 2. Have it set a snapshot only if it finds pending trigger events to > fire. This would only require another half dozen lines of code, but > it's certainly more complicated than choice #1. It seems to me there is room for a backwards compatibility argument here. How about doing #2 for 8.3 and back, and #1 for 8.4? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> 1. Always set a snapshot for SET CONSTRAINTS. This is a minus-one-liner
>> --- just remove it from the exclusion list in PortalRunUtility.
>>
>> 2. Have it set a snapshot only if it finds pending trigger events to
>> fire. This would only require another half dozen lines of code, but
>> it's certainly more complicated than choice #1.
> It seems to me there is room for a backwards compatibility argument
> here. How about doing #2 for 8.3 and back, and #1 for 8.4?
Well, if you think there's a real backwards compatibility issue, we
should just do #2 and be done with it. It's not like it's enough code
to really matter in the big scheme of things.
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.241
diff -c -r1.241 trigger.c
*** src/backend/commands/trigger.c 21 Nov 2008 20:14:27 -0000 1.241
--- src/backend/commands/trigger.c 12 Dec 2008 23:08:41 -0000
***************
*** 3716,3727 ****
--- 3716,3743 ---- if (!stmt->deferred) { AfterTriggerEventList *events = &afterTriggers->events;
+ bool snapshot_set = false; while (afterTriggerMarkEvents(events, NULL, true)) {
CommandId firing_id = afterTriggers->firing_counter++; /*
+ * Make sure a snapshot has been established in case trigger
+ * functions need one. Note that we avoid setting a snapshot if
+ * we don't find at least one trigger that has to be fired now.
+ * This is so that BEGIN; SET CONSTRAINTS ...; SET TRANSACTION
+ * ISOLATION LEVEL SERIALIZABLE; ... works properly. (If we are
+ * at the start of a transaction it's not possible for any trigger
+ * events to be queued yet.)
+ */
+ if (!snapshot_set)
+ {
+ PushActiveSnapshot(GetTransactionSnapshot());
+ snapshot_set = true;
+ }
+
+ /* * We can delete fired events if we are at top transaction level, * but we'd
betternot if inside a subtransaction, since the * subtransaction could later get rolled back.
***************
*** 3730,3735 ****
--- 3746,3754 ---- !IsSubTransaction())) break; /*
allfired */ }
+
+ if (snapshot_set)
+ PopActiveSnapshot(); } }
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Tom Lane wrote: > >> 1. Always set a snapshot for SET CONSTRAINTS. This is a minus-one-liner > >> --- just remove it from the exclusion list in PortalRunUtility. > >> > >> 2. Have it set a snapshot only if it finds pending trigger events to > >> fire. This would only require another half dozen lines of code, but > >> it's certainly more complicated than choice #1. > > > It seems to me there is room for a backwards compatibility argument > > here. How about doing #2 for 8.3 and back, and #1 for 8.4? > > Well, if you think there's a real backwards compatibility issue, we > should just do #2 and be done with it. It's not like it's enough code > to really matter in the big scheme of things. I don't like it just because it's another kludge in the way we set up ActiveSnapshot. I think it would be better if we were simplifying that code, not adding more kludges. If there's no backwards compatibility argument (and from the looks of your patch, perhaps there wouldn't), then I think we should just do #1. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > I don't like it just because it's another kludge in the way we set up > ActiveSnapshot. I think it would be better if we were simplifying that > code, not adding more kludges. I just saw your commits. It's nice that adding this kludge helped remove a previous one :-) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Well, if you think there's a real backwards compatibility issue, we
>> should just do #2 and be done with it. It's not like it's enough code
>> to really matter in the big scheme of things.
> I don't like it just because it's another kludge in the way we set up
> ActiveSnapshot. I think it would be better if we were simplifying that
> code, not adding more kludges.
> If there's no backwards compatibility argument (and from the looks of
> your patch, perhaps there wouldn't), then I think we should just do #1.
On the whole I think your original instinct was right: there is a
backwards compatibility issue here. Without the kluge added to
trigger.c, this would fail:
BEGIN;SET CONSTRAINTS ALL IMMEDIATE;SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
since a transaction snapshot would be set before reaching the isolation
level change. Since that has worked in the past, it seems there's a
nonnegligible risk of breaking apps. There's no obvious-to-the-user
reason why this ordering shouldn't be okay ...
regards, tom lane