Cannot use all four trigger events at once

Started by Greg Sabino Mullaneover 16 years ago5 messages
#1Greg Sabino Mullane
greg@turnstep.com
1 attachment(s)

This was failing:

CREATE TRIGGER foo
AFTER INSERT OR UPDATE OR DELETE OR TRUNCATE
ON foobar
FOR EACH STATEMENT EXECUTE PROCEDURE baz();

Turns out the parser wasn't set up to handle four different trigger
event types. Patch attached.

--
Greg Sabino Mullane greg@endpoint.com greg@turnstep.com
End Point Corporation 610-983-9073
PGP Key: 0x14964AC8 200906171620
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

Attachments:

trigger_four_events.patchtext/x-patch; name=trigger_four_events.patchDownload
Index: backend/parser/gram.y
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.664
diff -r2.664 gram.y
3174c3174
< 					char *e = palloc(4);
---
> 					char *e = palloc(2);
3180c3180
< 					char *e = palloc(4);
---
> 					char *e = palloc(3);
3189a3190,3195
> 			| TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent
> 				{
> 					char *e = palloc(5);
> 					e[0] = $1; e[1] = $3; e[2] = $5; e[3] = $7; e[4] = '\0';
> 					$$ = e;
> 				}
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Greg Sabino Mullane (#1)
Re: [HACKERS] Cannot use all four trigger events at once

Greg Sabino Mullane <greg@turnstep.com> writes:

This was failing:
CREATE TRIGGER foo
AFTER INSERT OR UPDATE OR DELETE OR TRUNCATE
ON foobar
FOR EACH STATEMENT EXECUTE PROCEDURE baz();

Turns out the parser wasn't set up to handle four different trigger
event types.

Hmm, that's a problem ...

Patch attached.

... but this patch doesn't come close to fixing it. struct CreateTrigStmt
needs changes. I kinda think the restriction to 4 (or whatever)
elements ought to go away altogether.

regards, tom lane

#3Greg Sabino Mullane
greg@turnstep.com
In reply to: Tom Lane (#2)
Re: [HACKERS] Cannot use all four trigger events at once

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160
NotDashEscaped: You need GnuPG to verify this message

... but this patch doesn't come close to fixing it. struct CreateTrigStmt
needs changes. I kinda think the restriction to 4 (or whatever)
elements ought to go away altogether.

Thanks. I'll leave the [5] restriction call to someone else, seems it
does no harm to me though (not as if we'd change it that often anyway).
Revised patch below. I looked through other places that might be affected,
but did not see anything else except ecpg/preproc/preproc.y, which
looks as though it's already been changed to handle four events.

Index: src/backend/parser/gram.y
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v      
retrieving revision 2.664                                          
diff -c -r2.664 gram.y                                             
*** src/backend/parser/gram.y   27 May 2009 20:42:29 -0000      2.664
--- src/backend/parser/gram.y   18 Jun 2009 00:44:15 -0000           
***************                                                      
*** 3133,3139 ****                                                   
                                        n->args = $13;               
                                        n->before = $4;              
                                        n->row = $8;                 
!                                       memcpy(n->actions, $5, 4);   
                                        n->isconstraint  = FALSE;    
                                        n->deferrable    = FALSE;    
                                        n->initdeferred  = FALSE;    
--- 3133,3139 ----                                                   
                                        n->args = $13;               
                                        n->before = $4;              
                                        n->row = $8;                 
!                                       memcpy(n->actions, $5, 5);   
                                        n->isconstraint  = FALSE;    
                                        n->deferrable    = FALSE;    
                                        n->initdeferred  = FALSE;    
***************                                                      
*** 3153,3159 ****                                                   
                                        n->args = $18;               
                                        n->before = FALSE;           
                                        n->row = TRUE;               
!                                       memcpy(n->actions, $6, 4);   
                                        n->isconstraint  = TRUE;     
                                        n->deferrable = ($10 & 1) != 0;
                                        n->initdeferred = ($10 & 2) != 0;
--- 3153,3159 ----                                                       
                                        n->args = $18;                   
                                        n->before = FALSE;               
                                        n->row = TRUE;                   
!                                       memcpy(n->actions, $6, 5);       
                                        n->isconstraint  = TRUE;         
                                        n->deferrable = ($10 & 1) != 0;  
                                        n->initdeferred = ($10 & 2) != 0;
***************                                                          
*** 3171,3192 ****                                                       
  TriggerEvents:                                                         
                        TriggerOneEvent                                  
                                {                                        
!                                       char *e = palloc(4);             
                                        e[0] = $1; e[1] = '\0';          
                                        $$ = e;                          
                                }                                        
                        | TriggerOneEvent OR TriggerOneEvent             
                                {                                        
!                                       char *e = palloc(4);             
                                        e[0] = $1; e[1] = $3; e[2] = '\0';
                                        $$ = e;                           
                                }                                         
                        | TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent
                                {                                              
!                                       char *e = palloc(4);                   
                                        e[0] = $1; e[1] = $3; e[2] = $5; e[3] = '\0';
                                        $$ = e;                                      
                                }                                                    
                ;                                                                    
  TriggerOneEvent:                                                                   
--- 3171,3198 ----                                                                   
  TriggerEvents:                                                                     
                        TriggerOneEvent                                              
                                {                                                    
!                                       char *e = palloc(5);                         
                                        e[0] = $1; e[1] = '\0';                      
                                        $$ = e;                                      
                                }                                                    
                        | TriggerOneEvent OR TriggerOneEvent                         
                                {                                                    
!                                       char *e = palloc(5);                         
                                        e[0] = $1; e[1] = $3; e[2] = '\0';           
                                        $$ = e;                                      
                                }                                                    
                        | TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent      
                                {                                                    
!                                       char *e = palloc(5);                         
                                        e[0] = $1; e[1] = $3; e[2] = $5; e[3] = '\0';
                                        $$ = e;                                      
                                }                                                    
+                       | TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent
+                               {                                                                 
+                                       char *e = palloc(5);                                      
+                                       e[0] = $1; e[1] = $3; e[2] = $5; e[3] = $7; e[4] = '\0';  
+                                       $$ = e;                                                   
+                               }                                                                 
                ;                                                                                 
  TriggerOneEvent:                                                                                
Index: src/include/nodes/parsenodes.h                                                             
===================================================================                               
RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v                                
retrieving revision 1.394                                                                         
diff -c -r1.394 parsenodes.h                                                                      
*** src/include/nodes/parsenodes.h      11 Jun 2009 14:49:11 -0000      1.394                     
--- src/include/nodes/parsenodes.h      18 Jun 2009 00:44:15 -0000                                
***************                                                                                   
*** 1551,1557 ****                                                                                
        List       *args;                       /* list of (T_String) Values or NIL */
        bool            before;                 /* BEFORE/AFTER */
        bool            row;                    /* ROW/STATEMENT */
!       char            actions[4];             /* 1 to 3 of 'i', 'u', 'd', + trailing \0 */
        /* The following are used for referential */
        /* integrity constraint triggers */
--- 1551,1557 ----
        List       *args;                       /* list of (T_String) Values or NIL */
        bool            before;                 /* BEFORE/AFTER */
        bool            row;                    /* ROW/STATEMENT */
!       char            actions[5];             /* 1 to 4 of 'i', 'u', 'd', or 't', + trailing \0 */

/* The following are used for referential */
/* integrity constraint triggers */

--
Greg Sabino Mullane greg@turnstep.com
End Point Corporation
PGP Key: 0x14964AC8 200906172045
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAko5jioACgkQvJuQZxSWSsgqBACfVMeTyQkT9wqJTPi9oTLHuipK
8sgAn0vowAiCNhnoYBnsZT310UFC1UV7
=LpeM
-----END PGP SIGNATURE-----

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Greg Sabino Mullane (#3)
Re: [HACKERS] Cannot use all four trigger events at once

"Greg Sabino Mullane" <greg@turnstep.com> writes:

Thanks. I'll leave the [5] restriction call to someone else, seems it
does no harm to me though (not as if we'd change it that often anyway).
Revised patch below. I looked through other places that might be affected,
but did not see anything else except ecpg/preproc/preproc.y, which
looks as though it's already been changed to handle four events.

preproc.y is a derived file nowadays, so if you looked at it after
having fixed gram.y it would've looked fixed. However, I felt we
should fix this code properly instead of propagating its existing
klugy approach still further. It gets a whole lot simpler and shorter
if you make the grammar produce the bitmap representation that trigger.c
actually wants; as per attached applied patch.

regards, tom lane

Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.287
diff -c -r1.287 tablecmds.c
*** src/backend/commands/tablecmds.c	11 Jun 2009 20:46:11 -0000	1.287
--- src/backend/commands/tablecmds.c	18 Jun 2009 01:13:04 -0000
***************
*** 5206,5219 ****
  	if (on_insert)
  	{
  		fk_trigger->funcname = SystemFuncName("RI_FKey_check_ins");
! 		fk_trigger->actions[0] = 'i';
  	}
  	else
  	{
  		fk_trigger->funcname = SystemFuncName("RI_FKey_check_upd");
! 		fk_trigger->actions[0] = 'u';
  	}
- 	fk_trigger->actions[1] = '\0';
  	fk_trigger->isconstraint = true;
  	fk_trigger->deferrable = fkconstraint->deferrable;
--- 5206,5218 ----
  	if (on_insert)
  	{
  		fk_trigger->funcname = SystemFuncName("RI_FKey_check_ins");
! 		fk_trigger->events = TRIGGER_TYPE_INSERT;
  	}
  	else
  	{
  		fk_trigger->funcname = SystemFuncName("RI_FKey_check_upd");
! 		fk_trigger->events = TRIGGER_TYPE_UPDATE;
  	}
  	fk_trigger->isconstraint = true;
  	fk_trigger->deferrable = fkconstraint->deferrable;
***************
*** 5263,5271 ****
  	fk_trigger->relation = fkconstraint->pktable;
  	fk_trigger->before = false;
  	fk_trigger->row = true;
! 	fk_trigger->actions[0] = 'd';
! 	fk_trigger->actions[1] = '\0';
! 
  	fk_trigger->isconstraint = true;
  	fk_trigger->constrrel = myRel;
  	switch (fkconstraint->fk_del_action)
--- 5262,5268 ----
  	fk_trigger->relation = fkconstraint->pktable;
  	fk_trigger->before = false;
  	fk_trigger->row = true;
! 	fk_trigger->events = TRIGGER_TYPE_DELETE;
  	fk_trigger->isconstraint = true;
  	fk_trigger->constrrel = myRel;
  	switch (fkconstraint->fk_del_action)
***************
*** 5316,5323 ****
  	fk_trigger->relation = fkconstraint->pktable;
  	fk_trigger->before = false;
  	fk_trigger->row = true;
! 	fk_trigger->actions[0] = 'u';
! 	fk_trigger->actions[1] = '\0';
  	fk_trigger->isconstraint = true;
  	fk_trigger->constrrel = myRel;
  	switch (fkconstraint->fk_upd_action)
--- 5313,5319 ----
  	fk_trigger->relation = fkconstraint->pktable;
  	fk_trigger->before = false;
  	fk_trigger->row = true;
! 	fk_trigger->events = TRIGGER_TYPE_UPDATE;
  	fk_trigger->isconstraint = true;
  	fk_trigger->constrrel = myRel;
  	switch (fkconstraint->fk_upd_action)
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.247
diff -c -r1.247 trigger.c
*** src/backend/commands/trigger.c	11 Jun 2009 14:48:56 -0000	1.247
--- src/backend/commands/trigger.c	18 Jun 2009 01:13:04 -0000
***************
*** 100,106 ****
  	Oid			funcoid;
  	Oid			funcrettype;
  	Oid			trigoid;
- 	int			i;
  	char		constrtrigname[NAMEDATALEN];
  	char	   *trigname;
  	char	   *constrname;
--- 100,105 ----
***************
*** 150,199 ****
  		TRIGGER_SETT_BEFORE(tgtype);
  	if (stmt->row)
  		TRIGGER_SETT_ROW(tgtype);

! for (i = 0; stmt->actions[i]; i++)
! {
! switch (stmt->actions[i])
! {
! case 'i':
! if (TRIGGER_FOR_INSERT(tgtype))
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("multiple INSERT events specified")));
! TRIGGER_SETT_INSERT(tgtype);
! break;
! case 'd':
! if (TRIGGER_FOR_DELETE(tgtype))
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("multiple DELETE events specified")));
! TRIGGER_SETT_DELETE(tgtype);
! break;
! case 'u':
! if (TRIGGER_FOR_UPDATE(tgtype))
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("multiple UPDATE events specified")));
! TRIGGER_SETT_UPDATE(tgtype);
! break;
! case 't':
! if (TRIGGER_FOR_TRUNCATE(tgtype))
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("multiple TRUNCATE events specified")));
! TRIGGER_SETT_TRUNCATE(tgtype);
! /* Disallow ROW-level TRUNCATE triggers */
! if (stmt->row)
! ereport(ERROR,
! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("TRUNCATE FOR EACH ROW triggers are not supported")));
! break;
! default:
! elog(ERROR, "unrecognized trigger event: %d",
! (int) stmt->actions[i]);
! break;
! }
! }

  	/*
  	 * Find and validate the trigger function.
--- 149,161 ----
  		TRIGGER_SETT_BEFORE(tgtype);
  	if (stmt->row)
  		TRIGGER_SETT_ROW(tgtype);
+ 	tgtype |= stmt->events;

! /* Disallow ROW-level TRUNCATE triggers */
! if (TRIGGER_FOR_ROW(tgtype) && TRIGGER_FOR_TRUNCATE(tgtype))
! ereport(ERROR,
! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("TRUNCATE FOR EACH ROW triggers are not supported")));

  	/*
  	 * Find and validate the trigger function.
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.431
diff -c -r1.431 copyfuncs.c
*** src/backend/nodes/copyfuncs.c	11 Jun 2009 14:48:58 -0000	1.431
--- src/backend/nodes/copyfuncs.c	18 Jun 2009 01:13:04 -0000
***************
*** 3081,3087 ****
  	COPY_NODE_FIELD(args);
  	COPY_SCALAR_FIELD(before);
  	COPY_SCALAR_FIELD(row);
! 	strcpy(newnode->actions, from->actions);	/* in-line string field */
  	COPY_SCALAR_FIELD(isconstraint);
  	COPY_SCALAR_FIELD(deferrable);
  	COPY_SCALAR_FIELD(initdeferred);
--- 3081,3087 ----
  	COPY_NODE_FIELD(args);
  	COPY_SCALAR_FIELD(before);
  	COPY_SCALAR_FIELD(row);
! 	COPY_SCALAR_FIELD(events);
  	COPY_SCALAR_FIELD(isconstraint);
  	COPY_SCALAR_FIELD(deferrable);
  	COPY_SCALAR_FIELD(initdeferred);
Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.354
diff -c -r1.354 equalfuncs.c
*** src/backend/nodes/equalfuncs.c	11 Jun 2009 14:48:58 -0000	1.354
--- src/backend/nodes/equalfuncs.c	18 Jun 2009 01:13:04 -0000
***************
*** 1639,1646 ****
  	COMPARE_NODE_FIELD(args);
  	COMPARE_SCALAR_FIELD(before);
  	COMPARE_SCALAR_FIELD(row);
! 	if (strcmp(a->actions, b->actions) != 0)	/* in-line string field */
! 		return false;
  	COMPARE_SCALAR_FIELD(isconstraint);
  	COMPARE_SCALAR_FIELD(deferrable);
  	COMPARE_SCALAR_FIELD(initdeferred);
--- 1639,1645 ----
  	COMPARE_NODE_FIELD(args);
  	COMPARE_SCALAR_FIELD(before);
  	COMPARE_SCALAR_FIELD(row);
! 	COMPARE_SCALAR_FIELD(events);
  	COMPARE_SCALAR_FIELD(isconstraint);
  	COMPARE_SCALAR_FIELD(deferrable);
  	COMPARE_SCALAR_FIELD(initdeferred);
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.664
diff -c -r2.664 gram.y
*** src/backend/parser/gram.y	27 May 2009 20:42:29 -0000	2.664
--- src/backend/parser/gram.y	18 Jun 2009 01:13:04 -0000
***************
*** 53,58 ****
--- 53,59 ----

#include "catalog/index.h"
#include "catalog/namespace.h"
+ #include "catalog/pg_trigger.h"
#include "commands/defrem.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
***************
*** 244,250 ****
%type <boolean> TriggerActionTime TriggerForSpec opt_trusted opt_restart_seqs
%type <str> opt_lancompiler

! %type <str> TriggerEvents
%type <value> TriggerFuncArg

  %type <str>		relation_name copy_file_name
--- 245,251 ----
  %type <boolean> TriggerActionTime TriggerForSpec opt_trusted opt_restart_seqs
  %type <str>		opt_lancompiler

! %type <ival> TriggerEvents TriggerOneEvent
%type <value> TriggerFuncArg

%type <str> relation_name copy_file_name
***************
*** 266,272 ****
%type <privtarget> privilege_target
%type <funwithargs> function_with_argtypes
%type <list> function_with_argtypes_list
- %type <chr> TriggerOneEvent

  %type <list>	stmtblock stmtmulti
  				OptTableElementList TableElementList OptInherit definition
--- 267,272 ----
***************
*** 3133,3139 ****
  					n->args = $13;
  					n->before = $4;
  					n->row = $8;
! 					memcpy(n->actions, $5, 4);
  					n->isconstraint  = FALSE;
  					n->deferrable	 = FALSE;
  					n->initdeferred  = FALSE;
--- 3133,3139 ----
  					n->args = $13;
  					n->before = $4;
  					n->row = $8;
! 					n->events = $5;
  					n->isconstraint  = FALSE;
  					n->deferrable	 = FALSE;
  					n->initdeferred  = FALSE;
***************
*** 3153,3163 ****
  					n->args = $18;
  					n->before = FALSE;
  					n->row = TRUE;
! 					memcpy(n->actions, $6, 4);
  					n->isconstraint  = TRUE;
  					n->deferrable = ($10 & 1) != 0;
  					n->initdeferred = ($10 & 2) != 0;
- 
  					n->constrrel = $9;
  					$$ = (Node *)n;
  				}
--- 3153,3162 ----
  					n->args = $18;
  					n->before = FALSE;
  					n->row = TRUE;
! 					n->events = $6;
  					n->isconstraint  = TRUE;
  					n->deferrable = ($10 & 1) != 0;
  					n->initdeferred = ($10 & 2) != 0;
  					n->constrrel = $9;
  					$$ = (Node *)n;
  				}
***************
*** 3170,3199 ****

TriggerEvents:
TriggerOneEvent
{
! char *e = palloc(4);
! e[0] = $1; e[1] = '\0';
! $$ = e;
! }
! | TriggerOneEvent OR TriggerOneEvent
! {
! char *e = palloc(4);
! e[0] = $1; e[1] = $3; e[2] = '\0';
! $$ = e;
! }
! | TriggerOneEvent OR TriggerOneEvent OR TriggerOneEvent
! {
! char *e = palloc(4);
! e[0] = $1; e[1] = $3; e[2] = $5; e[3] = '\0';
! $$ = e;
}
;

TriggerOneEvent:
! INSERT { $$ = 'i'; }
! | DELETE_P { $$ = 'd'; }
! | UPDATE { $$ = 'u'; }
! | TRUNCATE { $$ = 't'; }
;

  TriggerForSpec:
--- 3169,3188 ----

TriggerEvents:
TriggerOneEvent
+ { $$ = $1; }
+ | TriggerEvents OR TriggerOneEvent
{
! if ($1 & $3)
! yyerror("duplicate trigger events specified");
! $$ = $1 | $3;
}
;

TriggerOneEvent:
! INSERT { $$ = TRIGGER_TYPE_INSERT; }
! | DELETE_P { $$ = TRIGGER_TYPE_DELETE; }
! | UPDATE { $$ = TRIGGER_TYPE_UPDATE; }
! | TRUNCATE { $$ = TRIGGER_TYPE_TRUNCATE; }
;

  TriggerForSpec:
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.394
diff -c -r1.394 parsenodes.h
*** src/include/nodes/parsenodes.h	11 Jun 2009 14:49:11 -0000	1.394
--- src/include/nodes/parsenodes.h	18 Jun 2009 01:13:05 -0000
***************
*** 1551,1557 ****
  	List	   *args;			/* list of (T_String) Values or NIL */
  	bool		before;			/* BEFORE/AFTER */
  	bool		row;			/* ROW/STATEMENT */
! 	char		actions[4];		/* 1 to 3 of 'i', 'u', 'd', + trailing \0 */
  	/* The following are used for referential */
  	/* integrity constraint triggers */
--- 1551,1558 ----
  	List	   *args;			/* list of (T_String) Values or NIL */
  	bool		before;			/* BEFORE/AFTER */
  	bool		row;			/* ROW/STATEMENT */
! 	/* events uses the TRIGGER_TYPE bits defined in catalog/pg_trigger.h */
! 	int16		events;			/* INSERT/UPDATE/DELETE/TRUNCATE */

/* The following are used for referential */
/* integrity constraint triggers */

#5Simon Riggs
simon@2ndQuadrant.com
In reply to: Greg Sabino Mullane (#1)
Re: [HACKERS] Cannot use all four trigger events at once

On Wed, 2009-06-17 at 16:22 -0400, Greg Sabino Mullane wrote:

This was failing:

CREATE TRIGGER foo
AFTER INSERT OR UPDATE OR DELETE OR TRUNCATE
ON foobar
FOR EACH STATEMENT EXECUTE PROCEDURE baz();

Turns out the parser wasn't set up to handle four different trigger
event types. Patch attached.

Thanks for fixing Greg.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support