Work-in-progress referential action trigger timing patch

Started by Stephan Szaboover 20 years ago11 messages
#1Stephan Szabo
sszabo@megazone.bigpanda.com
1 attachment(s)

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

Attachments:

timing.difftext/plain; charset=US-ASCII; name=timing.diffDownload
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.193
diff -c -r1.193 trigger.c
*** src/backend/commands/trigger.c	23 Aug 2005 22:40:08 -0000	1.193
--- src/backend/commands/trigger.c	24 Aug 2005 03:24:48 -0000
***************
*** 2281,2286 ****
--- 2280,2286 ----
  	TriggerDesc *trigdesc = NULL;
  	FmgrInfo   *finfo = NULL;
  	Instrumentation *instr = NULL;
+ 	bool last_relation_loaded = false;
  
  	/* Make a per-tuple memory context for trigger function calls */
  	per_tuple_context =
***************
*** 2309,2314 ****
--- 2309,2328 ----
  			 */
  			if (rel == NULL || rel->rd_id != event->ate_relid)
  			{
+ 				bool found_relation_in_estate = false;
+ 				if (last_relation_loaded) {
+ 					if (rel)
+ 						heap_close(rel, NoLock);
+ 					if (trigdesc)
+ 						FreeTriggerDesc(trigdesc);
+ 					if (finfo)
+ 						pfree(finfo);
+ 					Assert(instr == NULL);	/* never used in this case */
+ 					rel = NULL;
+ 					trigdesc = NULL;
+ 					finfo = NULL;
+ 				}
+ 
  				if (estate)
  				{
  					/* Find target relation among estate's result rels */
***************
*** 2324,2348 ****
  						rInfo++;
  						nr--;
  					}
! 					if (nr <= 0)				/* should not happen */
! 						elog(ERROR, "could not find relation %u among query result relations",
! 							 event->ate_relid);
! 					rel = rInfo->ri_RelationDesc;
! 					trigdesc = rInfo->ri_TrigDesc;
! 					finfo = rInfo->ri_TrigFunctions;
! 					instr = rInfo->ri_TrigInstrument;
  				}
- 				else
- 				{
- 					/* Hard way: we manage the resources for ourselves */
- 					if (rel)
- 						heap_close(rel, NoLock);
- 					if (trigdesc)
- 						FreeTriggerDesc(trigdesc);
- 					if (finfo)
- 						pfree(finfo);
- 					Assert(instr == NULL);	/* never used in this case */
  
  					/*
  					 * We assume that an appropriate lock is still held by
  					 * the executor, so grab no new lock here.
--- 2338,2355 ----
  						rInfo++;
  						nr--;
  					}
! 					if (nr > 0) 
! 					{
! 						rel = rInfo->ri_RelationDesc;
! 						trigdesc = rInfo->ri_TrigDesc;
! 						finfo = rInfo->ri_TrigFunctions;
! 						instr = rInfo->ri_TrigInstrument;
! 						found_relation_in_estate = true;
! 					}
  				}
  
+ 				if (!found_relation_in_estate) 
+ 				{
  					/*
  					 * We assume that an appropriate lock is still held by
  					 * the executor, so grab no new lock here.
***************
*** 2367,2372 ****
--- 2374,2386 ----
  						palloc0(trigdesc->numtriggers * sizeof(FmgrInfo));
  
  					/* Never any EXPLAIN info in this case */
+ 					instr = NULL;
+ 
+ 					last_relation_loaded = true;
+ 				}
+ 				else 
+ 				{
+ 					last_relation_loaded = false;
  				}
  			}
  
***************
*** 2417,2423 ****
  	events->tail = prev_event;
  
  	/* Release working resources */
! 	if (!estate)
  	{
  		if (rel)
  			heap_close(rel, NoLock);
--- 2431,2437 ----
  	events->tail = prev_event;
  
  	/* Release working resources */
! 	if (last_relation_loaded)
  	{
  		if (rel)
  			heap_close(rel, NoLock);
***************
*** 2543,2551 ****
  	 * will be available for it to fire.
  	 *
  	 * If we find no firable events, we don't have to increment firing_counter.
  	 */
  	events = &afterTriggers->query_stack[afterTriggers->query_depth];
! 	if (afterTriggerMarkEvents(events, &afterTriggers->events, true))
  	{
  		CommandId		firing_id = afterTriggers->firing_counter++;
  
--- 2557,2569 ----
  	 * will be available for it to fire.
  	 *
  	 * If we find no firable events, we don't have to increment firing_counter.
+ 	 *
+ 	 * If we do find events, we continue to mark events and invoking them
+ 	 * until there are no events remaining because a referential action may
+ 	 * be adding not yet marked events to the end of our queue.
  	 */
  	events = &afterTriggers->query_stack[afterTriggers->query_depth];
! 	while (afterTriggerMarkEvents(events, &afterTriggers->events, true))
  	{
  		CommandId		firing_id = afterTriggers->firing_counter++;
  
Index: src/backend/executor/spi.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/executor/spi.c,v
retrieving revision 1.141
diff -c -r1.141 spi.c
*** src/backend/executor/spi.c	9 Jun 2005 21:25:22 -0000	1.141
--- src/backend/executor/spi.c	24 Aug 2005 03:24:50 -0000
***************
*** 40,48 ****
  static int _SPI_execute_plan(_SPI_plan *plan,
  							 Datum *Values, const char *Nulls,
  							 Snapshot snapshot, Snapshot crosscheck_snapshot,
! 							 bool read_only, long tcount);
  
! static int _SPI_pquery(QueryDesc *queryDesc, long tcount);
  
  static void _SPI_error_callback(void *arg);
  
--- 40,48 ----
  static int _SPI_execute_plan(_SPI_plan *plan,
  							 Datum *Values, const char *Nulls,
  							 Snapshot snapshot, Snapshot crosscheck_snapshot,
! 							 bool read_only, long tcount, bool nest_triggers);
  
! static int _SPI_pquery(QueryDesc *queryDesc, long tcount, bool nest_triggers);
  
  static void _SPI_error_callback(void *arg);
  
***************
*** 302,308 ****
  
  	res = _SPI_execute_plan(&plan, NULL, NULL,
  							InvalidSnapshot, InvalidSnapshot,
! 							read_only, tcount);
  
  	_SPI_end_call(true);
  	return res;
--- 302,308 ----
  
  	res = _SPI_execute_plan(&plan, NULL, NULL,
  							InvalidSnapshot, InvalidSnapshot,
! 							read_only, tcount, true);
  
  	_SPI_end_call(true);
  	return res;
***************
*** 335,341 ****
  	res = _SPI_execute_plan((_SPI_plan *) plan,
  							Values, Nulls,
  							InvalidSnapshot, InvalidSnapshot,
! 							read_only, tcount);
  
  	_SPI_end_call(true);
  	return res;
--- 335,341 ----
  	res = _SPI_execute_plan((_SPI_plan *) plan,
  							Values, Nulls,
  							InvalidSnapshot, InvalidSnapshot,
! 							read_only, tcount, true);
  
  	_SPI_end_call(true);
  	return res;
***************
*** 361,367 ****
  SPI_execute_snapshot(void *plan,
  					 Datum *Values, const char *Nulls,
  					 Snapshot snapshot, Snapshot crosscheck_snapshot,
! 					 bool read_only, long tcount)
  {
  	int			res;
  
--- 361,367 ----
  SPI_execute_snapshot(void *plan,
  					 Datum *Values, const char *Nulls,
  					 Snapshot snapshot, Snapshot crosscheck_snapshot,
! 					 bool read_only, long tcount, bool nest_triggers)
  {
  	int			res;
  
***************
*** 378,384 ****
  	res = _SPI_execute_plan((_SPI_plan *) plan,
  							Values, Nulls,
  							snapshot, crosscheck_snapshot,
! 							read_only, tcount);
  
  	_SPI_end_call(true);
  	return res;
--- 378,384 ----
  	res = _SPI_execute_plan((_SPI_plan *) plan,
  							Values, Nulls,
  							snapshot, crosscheck_snapshot,
! 							read_only, tcount, nest_triggers);
  
  	_SPI_end_call(true);
  	return res;
***************
*** 1306,1316 ****
   * crosscheck_snapshot: for RI use, all others pass InvalidSnapshot
   * read_only: TRUE for read-only execution (no CommandCounterIncrement)
   * tcount: execution tuple-count limit, or 0 for none
   */
  static int
  _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
  				  Snapshot snapshot, Snapshot crosscheck_snapshot,
! 				  bool read_only, long tcount)
  {
  	volatile int res = 0;
  	Snapshot	saveActiveSnapshot;
--- 1306,1319 ----
   * crosscheck_snapshot: for RI use, all others pass InvalidSnapshot
   * read_only: TRUE for read-only execution (no CommandCounterIncrement)
   * tcount: execution tuple-count limit, or 0 for none
+  * nest_triggers: TRUE to treat as a separate query for after triggers,
+  *		FALSE to place after triggers on the outer statement's 
+  *		queue.
   */
  static int
  _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
  				  Snapshot snapshot, Snapshot crosscheck_snapshot,
! 				  bool read_only, long tcount, bool nest_triggers)
  {
  	volatile int res = 0;
  	Snapshot	saveActiveSnapshot;
***************
*** 1462,1468 ****
  											dest,
  											paramLI, false);
  					res = _SPI_pquery(qdesc,
! 									  queryTree->canSetTag ? tcount : 0);
  					FreeQueryDesc(qdesc);
  				}
  				FreeSnapshot(ActiveSnapshot);
--- 1465,1471 ----
  											dest,
  											paramLI, false);
  					res = _SPI_pquery(qdesc,
! 									  queryTree->canSetTag ? tcount : 0, nest_triggers);
  					FreeQueryDesc(qdesc);
  				}
  				FreeSnapshot(ActiveSnapshot);
***************
*** 1494,1500 ****
  }
  
  static int
! _SPI_pquery(QueryDesc *queryDesc, long tcount)
  {
  	int			operation = queryDesc->operation;
  	CommandDest	origDest = queryDesc->dest->mydest;
--- 1497,1503 ----
  }
  
  static int
! _SPI_pquery(QueryDesc *queryDesc, long tcount, bool nest_triggers)
  {
  	int			operation = queryDesc->operation;
  	CommandDest	origDest = queryDesc->dest->mydest;
***************
*** 1529,1535 ****
  		ResetUsage();
  #endif
  
! 	AfterTriggerBeginQuery();
  
  	ExecutorStart(queryDesc, false);
  
--- 1532,1539 ----
  		ResetUsage();
  #endif
  
! 	if (nest_triggers)
! 		AfterTriggerBeginQuery();
  
  	ExecutorStart(queryDesc, false);
  
***************
*** 1544,1551 ****
  			elog(ERROR, "consistency check on SPI tuple count failed");
  	}
  
! 	/* Take care of any queued AFTER triggers */
! 	AfterTriggerEndQuery(queryDesc->estate);
  
  	ExecutorEnd(queryDesc);
  
--- 1548,1556 ----
  			elog(ERROR, "consistency check on SPI tuple count failed");
  	}
  
! 	/* Take care of any queued AFTER triggers if we are nesting triggers */
! 	if (nest_triggers)
! 		AfterTriggerEndQuery(queryDesc->estate);
  
  	ExecutorEnd(queryDesc);
  
Index: src/backend/utils/adt/ri_triggers.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/ri_triggers.c,v
retrieving revision 1.80
diff -c -r1.80 ri_triggers.c
*** src/backend/utils/adt/ri_triggers.c	28 Jun 2005 05:09:00 -0000	1.80
--- src/backend/utils/adt/ri_triggers.c	24 Aug 2005 03:24:56 -0000
***************
*** 2383,2389 ****
  									 qkey.keypair[i][RI_KEYPAIR_PK_IDX]);
  				}
  				strcat(querystr, qualstr);
- 
  				/* Prepare the plan, don't save it */
  				qplan = ri_PlanCheck(querystr, qkey.nkeypairs, queryoids,
  									 &qkey, fk_rel, pk_rel, false);
--- 2383,2388 ----
***************
*** 2745,2751 ****
  									  NULL, NULL,
  									  CopySnapshot(GetLatestSnapshot()),
  									  InvalidSnapshot,
! 									  true, 1);
  
  	/* Check result */
  	if (spi_result != SPI_OK_SELECT)
--- 2744,2750 ----
  									  NULL, NULL,
  									  CopySnapshot(GetLatestSnapshot()),
  									  InvalidSnapshot,
! 									  true, 1, false);
  
  	/* Check result */
  	if (spi_result != SPI_OK_SELECT)
***************
*** 3175,3181 ****
  	spi_result = SPI_execute_snapshot(qplan,
  									  vals, nulls,
  									  test_snapshot, crosscheck_snapshot,
! 									  false, limit);
  
  	/* Restore UID */
  	SetUserId(save_uid);
--- 3174,3180 ----
  	spi_result = SPI_execute_snapshot(qplan,
  									  vals, nulls,
  									  test_snapshot, crosscheck_snapshot,
! 									  false, limit, false);
  
  	/* Restore UID */
  	SetUserId(save_uid);
Index: src/include/executor/spi.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/executor/spi.h,v
retrieving revision 1.52
diff -c -r1.52 spi.h
*** src/include/executor/spi.h	2 May 2005 00:37:06 -0000	1.52
--- src/include/executor/spi.h	24 Aug 2005 03:24:58 -0000
***************
*** 92,98 ****
  								 Datum *Values, const char *Nulls,
  								 Snapshot snapshot,
  								 Snapshot crosscheck_snapshot,
! 								 bool read_only, long tcount);
  extern void *SPI_prepare(const char *src, int nargs, Oid *argtypes);
  extern void *SPI_saveplan(void *plan);
  extern int	SPI_freeplan(void *plan);
--- 92,98 ----
  								 Datum *Values, const char *Nulls,
  								 Snapshot snapshot,
  								 Snapshot crosscheck_snapshot,
! 								 bool read_only, long tcount, bool nest_triggers);
  extern void *SPI_prepare(const char *src, int nargs, Oid *argtypes);
  extern void *SPI_saveplan(void *plan);
  extern int	SPI_freeplan(void *plan);
Index: src/test/regress/expected/foreign_key.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/foreign_key.out,v
retrieving revision 1.39
diff -c -r1.39 foreign_key.out
*** src/test/regress/expected/foreign_key.out	30 May 2005 07:20:59 -0000	1.39
--- src/test/regress/expected/foreign_key.out	24 Aug 2005 03:25:03 -0000
***************
*** 646,652 ****
  UPDATE PKTABLE set ptest2=5 where ptest2=2;
  ERROR:  insert or update on table "fktable" violates foreign key constraint "constrname3"
  DETAIL:  Key (ftest1,ftest2,ftest3)=(1,-1,3) is not present in table "pktable".
- CONTEXT:  SQL statement "UPDATE ONLY "public"."fktable" SET "ftest2" = DEFAULT WHERE "ftest1" = $1 AND "ftest2" = $2 AND "ftest3" = $3"
  -- Try to update something that will set default
  UPDATE PKTABLE set ptest1=0, ptest2=5, ptest3=10 where ptest2=2;
  UPDATE PKTABLE set ptest2=10 where ptest2=4;
--- 646,651 ----
***************
*** 1172,1174 ****
--- 1171,1187 ----
  COMMIT;
  ERROR:  insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey"
  DETAIL:  Key (fk)=(20) is not present in table "pktable".
+ -- Check to make sure that forcing checks like the above doesn't allow
+ -- us to see intermediate states caused by multiple referential actions
+ -- on the same row.
+ DROP TABLE pktable, fktable CASCADE;
+ NOTICE:  drop cascades to constraint fktable_fk_fkey on table fktable
+ CREATE TEMP TABLE pktable(id int PRIMARY KEY);
+ NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
+ CREATE TEMP TABLE fktable(id1 int REFERENCES pktable ON UPDATE CASCADE,
+                           id2 int REFERENCES pktable ON UPDATE CASCADE,
+                           id3 int REFERENCES pktable ON UPDATE CASCADE);
+ INSERT INTO pktable VALUES (1);
+ INSERT INTO fktable VALUES (1,1,1);
+ -- This should succeed
+ UPDATE pktable SET id = 2;
Index: src/test/regress/sql/foreign_key.sql
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/foreign_key.sql,v
retrieving revision 1.16
diff -c -r1.16 foreign_key.sql
*** src/test/regress/sql/foreign_key.sql	30 May 2005 07:20:59 -0000	1.16
--- src/test/regress/sql/foreign_key.sql	24 Aug 2005 03:25:04 -0000
***************
*** 808,810 ****
--- 808,827 ----
  
  -- should catch error from initial INSERT
  COMMIT;
+ 
+ 
+ -- Check to make sure that forcing checks like the above doesn't allow
+ -- us to see intermediate states caused by multiple referential actions
+ -- on the same row if we update it a bunch of times in a single statement.
+ DROP TABLE pktable, fktable CASCADE;
+ CREATE TEMP TABLE pktable(id int PRIMARY KEY);
+ CREATE TEMP TABLE fktable(id1 int REFERENCES pktable ON UPDATE CASCADE, 
+ 			  id2 int REFERENCES pktable ON UPDATE CASCADE,
+ 			  id3 int REFERENCES pktable ON UPDATE CASCADE);
+ 
+ INSERT INTO pktable VALUES (1);
+ INSERT INTO fktable VALUES (1,1,1);
+ 
+ -- This should succeed
+ UPDATE pktable SET id = 2;
+ 
#2Allan Wang
allanvv@gmail.com
In reply to: Stephan Szabo (#1)
Re: Work-in-progress referential action trigger timing patch

On Tue, 2005-08-23 at 20:47 -0700, Stephan Szabo wrote:

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

Thanks, I'll try this patch and get back to you on the results.

Allan Wang

#3Allan Wang
allanvv@gmail.com
In reply to: Stephan Szabo (#1)
Re: Work-in-progress referential action trigger timing patch

On Tue, 2005-08-23 at 20:47 -0700, Stephan Szabo wrote:

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

Yup, this patch fixed my issues.

Allan Wang

#4Stephan Szabo
sszabo@megazone.bigpanda.com
In reply to: Stephan Szabo (#1)
Re: Work-in-progress referential action trigger timing

On Tue, 23 Aug 2005, Stephan Szabo wrote:

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

As Darcy noticed, the patch as given does definately still have problems
with before triggers. I was able to construct a case that violates the
constraint with an update in a before delete trigger. I think this might
be why the spec has the wierd timing rules for before triggers on cascaded
deletes such that the deletions happen before the before triggers.

We have a similar problem for before triggers that update the rows that
are being cascade updated. The following seems to violate the constraint
for me on 8.0.3:

drop table pk cascade;
drop table fk cascade;
drop function fk_move();

create table pk(a int primary key);
create table fk(a int references pk on delete cascade on update cascade, b
int);
create function fk_move() returns trigger as '
begin
raise notice '' about to move for % '', old.b;
update fk set b=b-1 where b > old.b;
return new;
end;' language 'plpgsql';
create trigger fkmovetrig before update on fk for each row execute
procedure fk_move();
insert into pk values(1);
insert into pk values(2);
insert into fk values(1,1);
insert into fk values(1,2);
insert into fk values(2,3);
select * from pk;
select * from fk;
update pk set a = 3 where a = 1;
select * from pk;
select * from fk;

This gives me (3,1), (1,1) and (2,2) as the rows in fk where the (1,1) row
is invalid. This is obviously wrong, but the question is, what is the
correct answer? Should the update in the before trigger trying to change
b on a row that no longer has a reference have errored?

#5Stephan Szabo
sszabo@megazone.bigpanda.com
In reply to: Stephan Szabo (#4)
Re: [PATCHES] Work-in-progress referential action trigger timing

[Hackers now seems more appropriate]

On Thu, 1 Sep 2005, Stephan Szabo wrote:

On Tue, 23 Aug 2005, Stephan Szabo wrote:

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

As Darcy noticed, the patch as given does definately still have problems
with before triggers. I was able to construct a case that violates the
constraint with an update in a before delete trigger. I think this might
be why the spec has the wierd timing rules for before triggers on cascaded
deletes such that the deletions happen before the before triggers.

We have a similar problem for before triggers that update the rows that
are being cascade updated. The following seems to violate the constraint
for me on 8.0.3:

drop table pk cascade;
drop table fk cascade;
drop function fk_move();

create table pk(a int primary key);
create table fk(a int references pk on delete cascade on update cascade, b
int);
create function fk_move() returns trigger as '
begin
raise notice '' about to move for % '', old.b;
update fk set b=b-1 where b > old.b;
return new;
end;' language 'plpgsql';
create trigger fkmovetrig before update on fk for each row execute
procedure fk_move();
insert into pk values(1);
insert into pk values(2);
insert into fk values(1,1);
insert into fk values(1,2);
insert into fk values(2,3);
select * from pk;
select * from fk;
update pk set a = 3 where a = 1;
select * from pk;
select * from fk;

This gives me (3,1), (1,1) and (2,2) as the rows in fk where the (1,1) row
is invalid. This is obviously wrong, but the question is, what is the
correct answer? Should the update in the before trigger trying to change
b on a row that no longer has a reference have errored?

Well, the spec seems to get out of this simply. I read SQL2003's trigger
execution information (specifically 14.27 GR5g*) to say that before
triggers that call data changing statements are invalid.

We can't do that for compatibility reasons, but it would allow us to say
that modifying a row in a before trigger that is also a row selected in
the outer statement is an error for this update case. It'd presumably be
an error for a normal delete as well, although I think it might be
relaxable for cascaded deletes because the spec seems to say that the
before triggers for deletions caused by the cascade are actually run after
the removals. I'm not sure whether we could easily differentiate this case
from any other cases where the row was modified twice either yet.

---
* "If TR is a BEFORE trigger and if, before the completion of the
execution of an <SQL procedure statement> simply contained in TSS, an
attempt is made to execute an SQL-data change statement or an SQL-invoked
routine that possibly modifies SQL-data, then an exception condition is
raised: prohibited statement encountered during trigger execution."

#6Stephan Szabo
sszabo@megazone.bigpanda.com
In reply to: Stephan Szabo (#5)
Re: [PATCHES] Work-in-progress referential action trigger

On Fri, 2 Sep 2005, Stephan Szabo wrote:

[Hackers now seems more appropriate]

On Thu, 1 Sep 2005, Stephan Szabo wrote:

On Tue, 23 Aug 2005, Stephan Szabo wrote:

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

As Darcy noticed, the patch as given does definately still have problems
with before triggers. I was able to construct a case that violates the
constraint with an update in a before delete trigger. I think this might
be why the spec has the wierd timing rules for before triggers on cascaded
deletes such that the deletions happen before the before triggers.

We have a similar problem for before triggers that update the rows that
are being cascade updated. The following seems to violate the constraint
for me on 8.0.3:

drop table pk cascade;
drop table fk cascade;
drop function fk_move();

create table pk(a int primary key);
create table fk(a int references pk on delete cascade on update cascade, b
int);
create function fk_move() returns trigger as '
begin
raise notice '' about to move for % '', old.b;
update fk set b=b-1 where b > old.b;
return new;
end;' language 'plpgsql';
create trigger fkmovetrig before update on fk for each row execute
procedure fk_move();
insert into pk values(1);
insert into pk values(2);
insert into fk values(1,1);
insert into fk values(1,2);
insert into fk values(2,3);
select * from pk;
select * from fk;
update pk set a = 3 where a = 1;
select * from pk;
select * from fk;

This gives me (3,1), (1,1) and (2,2) as the rows in fk where the (1,1) row
is invalid. This is obviously wrong, but the question is, what is the
correct answer? Should the update in the before trigger trying to change
b on a row that no longer has a reference have errored?

We can't do that for compatibility reasons, but it would allow us to say
that modifying a row in a before trigger that is also a row selected in
the outer statement is an error for this update case. It'd presumably be
an error for a normal delete as well, although I think it might be
relaxable for cascaded deletes because the spec seems to say that the
before triggers for deletions caused by the cascade are actually run after
the removals. I'm not sure whether we could easily differentiate this case
from any other cases where the row was modified twice either yet.

Is there a case other than a before trigger updating a row we will want to
act upon later in the statement where we'll get a row with xmax of our
transaction and cmax greater than the current command?

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Stephan Szabo (#6)
Re: [PATCHES] Work-in-progress referential action trigger timing

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:

Is there a case other than a before trigger updating a row we will want to
act upon later in the statement where we'll get a row with xmax of our
transaction and cmax greater than the current command?

The greater-cmax case could occur via any kind of function, not only a
trigger, ie

update tab set x = foo(x) where ...

where foo() is a volatile function that internally updates the tab
table.

I suppose you could say that this is horrible programming practice and
anyone who tries it deserves whatever weird behavior ensues ... but
it's not the case that every such situation involves a trigger.

regards, tom lane

#8Stephan Szabo
sszabo@megazone.bigpanda.com
In reply to: Tom Lane (#7)
Re: [PATCHES] Work-in-progress referential action trigger

On Fri, 9 Sep 2005, Tom Lane wrote:

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:

Is there a case other than a before trigger updating a row we will want to
act upon later in the statement where we'll get a row with xmax of our
transaction and cmax greater than the current command?

The greater-cmax case could occur via any kind of function, not only a
trigger, ie

update tab set x = foo(x) where ...

where foo() is a volatile function that internally updates the tab
table.

I *thought* I was missing a case, I just couldn't figure out what.

I suppose you could say that this is horrible programming practice and
anyone who tries it deserves whatever weird behavior ensues ... but
it's not the case that every such situation involves a trigger.

Well, the change I was thinking of would have made it an error if foo(x)
updated a row that was then later selected by the update rather than the
current behavior which I think would have ignored the already updated row,
so that's probably not going to work.

#9Darcy Buskermolen
darcy@wavefire.com
In reply to: Stephan Szabo (#8)
Re: [PATCHES] Work-in-progress referential action trigger

On Friday 09 September 2005 08:46, Stephan Szabo wrote:

On Fri, 9 Sep 2005, Tom Lane wrote:

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:

Is there a case other than a before trigger updating a row we will want
to act upon later in the statement where we'll get a row with xmax of
our transaction and cmax greater than the current command?

The greater-cmax case could occur via any kind of function, not only a
trigger, ie

update tab set x = foo(x) where ...

where foo() is a volatile function that internally updates the tab
table.

I *thought* I was missing a case, I just couldn't figure out what.

I suppose you could say that this is horrible programming practice and
anyone who tries it deserves whatever weird behavior ensues ... but
it's not the case that every such situation involves a trigger.

Well, the change I was thinking of would have made it an error if foo(x)
updated a row that was then later selected by the update rather than the
current behavior which I think would have ignored the already updated row,
so that's probably not going to work.

I see that this still is not addressed fulling in beta 3. Can anybody give a
quick overview of where this is sitting, and if it's likely to make it's way
into 8.1 gold ?

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

--
Darcy Buskermolen
Wavefire Technologies Corp.

http://www.wavefire.com
ph: 250.717.0200
fx: 250.763.1759

#10Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Darcy Buskermolen (#9)
Foreign key trigger timing bug?

I think this is the foreign key trigger timing issue.

---------------------------------------------------------------------------

Darcy Buskermolen wrote:

On Friday 09 September 2005 08:46, Stephan Szabo wrote:

On Fri, 9 Sep 2005, Tom Lane wrote:

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:

Is there a case other than a before trigger updating a row we will want
to act upon later in the statement where we'll get a row with xmax of
our transaction and cmax greater than the current command?

The greater-cmax case could occur via any kind of function, not only a
trigger, ie

update tab set x = foo(x) where ...

where foo() is a volatile function that internally updates the tab
table.

I *thought* I was missing a case, I just couldn't figure out what.

I suppose you could say that this is horrible programming practice and
anyone who tries it deserves whatever weird behavior ensues ... but
it's not the case that every such situation involves a trigger.

Well, the change I was thinking of would have made it an error if foo(x)
updated a row that was then later selected by the update rather than the
current behavior which I think would have ignored the already updated row,
so that's probably not going to work.

I see that this still is not addressed fulling in beta 3. Can anybody give a
quick overview of where this is sitting, and if it's likely to make it's way
into 8.1 gold ?

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

--
Darcy Buskermolen
Wavefire Technologies Corp.

http://www.wavefire.com
ph: 250.717.0200
fx: 250.763.1759

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
#11Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Stephan Szabo (#5)
Re: [PATCHES] Work-in-progress referential action trigger

Added to TODO:

o Fix problem when cascading referential triggers make changes on
cascaded tables, seeing the tables in an intermediate state

http://archives.postgresql.org/pgsql-hackers/2005-09/msg00174.php
http://archives.postgresql.org/pgsql-hackers/2005-09/msg00174.php

---------------------------------------------------------------------------

Stephan Szabo wrote:

[Hackers now seems more appropriate]

On Thu, 1 Sep 2005, Stephan Szabo wrote:

On Tue, 23 Aug 2005, Stephan Szabo wrote:

Here's my current work in progress for 8.1 devel related to fixing the
timing issues with referential actions having their checks run on
intermediate states. I've only put in a simple test that failed against
8.0 in the regression patch and regression still passes for me. There's
still an outstanding question of whether looping gives the correct result
in the presence of explicit inserts and set constraints immediate in
before triggers.

As Darcy noticed, the patch as given does definately still have problems
with before triggers. I was able to construct a case that violates the
constraint with an update in a before delete trigger. I think this might
be why the spec has the wierd timing rules for before triggers on cascaded
deletes such that the deletions happen before the before triggers.

We have a similar problem for before triggers that update the rows that
are being cascade updated. The following seems to violate the constraint
for me on 8.0.3:

drop table pk cascade;
drop table fk cascade;
drop function fk_move();

create table pk(a int primary key);
create table fk(a int references pk on delete cascade on update cascade, b
int);
create function fk_move() returns trigger as '
begin
raise notice '' about to move for % '', old.b;
update fk set b=b-1 where b > old.b;
return new;
end;' language 'plpgsql';
create trigger fkmovetrig before update on fk for each row execute
procedure fk_move();
insert into pk values(1);
insert into pk values(2);
insert into fk values(1,1);
insert into fk values(1,2);
insert into fk values(2,3);
select * from pk;
select * from fk;
update pk set a = 3 where a = 1;
select * from pk;
select * from fk;

This gives me (3,1), (1,1) and (2,2) as the rows in fk where the (1,1) row
is invalid. This is obviously wrong, but the question is, what is the
correct answer? Should the update in the before trigger trying to change
b on a row that no longer has a reference have errored?

Well, the spec seems to get out of this simply. I read SQL2003's trigger
execution information (specifically 14.27 GR5g*) to say that before
triggers that call data changing statements are invalid.

We can't do that for compatibility reasons, but it would allow us to say
that modifying a row in a before trigger that is also a row selected in
the outer statement is an error for this update case. It'd presumably be
an error for a normal delete as well, although I think it might be
relaxable for cascaded deletes because the spec seems to say that the
before triggers for deletions caused by the cascade are actually run after
the removals. I'm not sure whether we could easily differentiate this case
from any other cases where the row was modified twice either yet.

---
* "If TR is a BEFORE trigger and if, before the completion of the
execution of an <SQL procedure statement> simply contained in TSS, an
attempt is made to execute an SQL-data change statement or an SQL-invoked
routine that possibly modifies SQL-data, then an exception condition is
raised: prohibited statement encountered during trigger execution."

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +