Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Patch applied. Thanks. --- Gavin Sherry wrote: > Attached is a patch adding regression tests for this code. > > Thanks, > > Gavin > > On Tue, 23 Aug 2005, Bruce Momjian wrote: > > > > > Thanks, modified patch applied by Tom, with the addition of a USER > > triggers only mode. > > > > --- > > > > Satoshi Nagayasu wrote: > > > The message format for elog() report is cleaned up. > > > > > > -- > > > NAGAYASU Satoshi <[EMAIL PROTECTED]> > > > > > diff -cr pgsql.orig/src/backend/commands/tablecmds.c > > > pgsql/src/backend/commands/tablecmds.c > > > *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 > > > 14:08:54.0 +0900 > > > --- pgsql/src/backend/commands/tablecmds.c2005-08-08 > > > 13:46:44.0 +0900 > > > *** > > > *** 236,241 > > > --- 236,243 > > -- > > 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 > > > > ---(end of broadcast)--- > > TIP 4: Have you searched our list archives? > > > >http://archives.postgresql.org > > Content-Description: [ Attachment, skipping... ] -- 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches It will be applied as soon as one of the PostgreSQL committers reviews and approves it. --- Gavin Sherry wrote: > Attached is a patch adding regression tests for this code. > > Thanks, > > Gavin > > On Tue, 23 Aug 2005, Bruce Momjian wrote: > > > > > Thanks, modified patch applied by Tom, with the addition of a USER > > triggers only mode. > > > > --- > > > > Satoshi Nagayasu wrote: > > > The message format for elog() report is cleaned up. > > > > > > -- > > > NAGAYASU Satoshi <[EMAIL PROTECTED]> > > > > > diff -cr pgsql.orig/src/backend/commands/tablecmds.c > > > pgsql/src/backend/commands/tablecmds.c > > > *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 > > > 14:08:54.0 +0900 > > > --- pgsql/src/backend/commands/tablecmds.c2005-08-08 > > > 13:46:44.0 +0900 > > > *** > > > *** 236,241 > > > --- 236,243 > > -- > > 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 > > > > ---(end of broadcast)--- > > TIP 4: Have you searched our list archives? > > > >http://archives.postgresql.org > > Content-Description: [ Attachment, skipping... ] -- 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Attached is a patch adding regression tests for this code. Thanks, Gavin On Tue, 23 Aug 2005, Bruce Momjian wrote: > > Thanks, modified patch applied by Tom, with the addition of a USER > triggers only mode. > > --- > > Satoshi Nagayasu wrote: > > The message format for elog() report is cleaned up. > > > > -- > > NAGAYASU Satoshi <[EMAIL PROTECTED]> > > > diff -cr pgsql.orig/src/backend/commands/tablecmds.c > > pgsql/src/backend/commands/tablecmds.c > > *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 > > 14:08:54.0 +0900 > > --- pgsql/src/backend/commands/tablecmds.c 2005-08-08 13:46:44.0 > > +0900 > > *** > > *** 236,241 > > --- 236,243 > -- > 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 > > ---(end of broadcast)--- > TIP 4: Have you searched our list archives? > >http://archives.postgresql.org >Index: src/test/regress/expected/triggers.out === RCS file: /usr/local/cvsroot/pgsql/src/test/regress/expected/triggers.out,v retrieving revision 1.18 diff -c -p -r1.18 triggers.out *** src/test/regress/expected/triggers.out 13 Oct 2004 01:22:31 - 1.18 --- src/test/regress/expected/triggers.out 25 Aug 2005 01:07:08 - *** SELECT * FROM main_table ORDER BY a, b; *** 322,324 --- 322,388 | (8 rows) + -- Test enable/disable triggers + create table trigtest (i serial primary key); + NOTICE: CREATE TABLE will create implicit sequence "trigtest_i_seq" for serial column "trigtest.i" + NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "trigtest_pkey" for table "trigtest" + -- test that disabling RI triggers works + create table trigtest2 (i int references trigtest(i) on delete cascade); + create function trigtest() returns trigger as $$ + begin + raise notice '% % % %', TG_RELNAME, TG_OP, TG_WHEN, TG_LEVEL; + return new; + end;$$ language plpgsql; + create trigger trigtest_b_row_tg before insert or update or delete on trigtest + for each row execute procedure trigtest(); + create trigger trigtest_a_row_tg after insert or update or delete on trigtest + for each row execute procedure trigtest(); + create trigger trigtest_b_stmt_tg before insert or update or delete on trigtest + for each statement execute procedure trigtest(); + create trigger trigtest_a_stmt_tg after insert or update or delete on trigtest + for each statement execute procedure trigtest(); + insert into trigtest default values; + NOTICE: trigtest INSERT BEFORE STATEMENT + NOTICE: trigtest INSERT BEFORE ROW + NOTICE: trigtest INSERT AFTER ROW + NOTICE: trigtest INSERT AFTER STATEMENT + alter table trigtest disable trigger trigtest_b_row_tg; + insert into trigtest default values; + NOTICE: trigtest INSERT BEFORE STATEMENT + NOTICE: trigtest INSERT AFTER ROW + NOTICE: trigtest INSERT AFTER STATEMENT + alter table trigtest disable trigger user; + insert into trigtest default values; + alter table trigtest enable trigger trigtest_a_stmt_tg; + insert into trigtest default values; + NOTICE: trigtest INSERT AFTER STATEMENT + insert into trigtest2 values(1); + insert into trigtest2 values(2); + delete from trigtest where i=2; + NOTICE: trigtest DELETE AFTER STATEMENT + select * from trigtest2; + i + --- + 1 + (1 row) + + alter table trigtest disable trigger all; + delete from trigtest where i=1; + select * from trigtest2; + i + --- + 1 + (1 row) + + -- ensure we still insert, even when all triggers are disabled + insert into trigtest default values; + select * from trigtest; + i + --- + 3 + 4 + 5 + (3 rows) + + drop table trigtest2; + drop table trigtest; Index: src/test/regress/sql/triggers.sql === RCS file: /usr/local/cvsroot/pgsql/src/test/regress/sql/triggers.sql,v retrieving revision 1.8 diff -c -p -r1.8 triggers.sql *** src/test/regress/sql/triggers.sql 21 Nov 2003 22:32:49 - 1.8 --- src/test/regress/sql/triggers.sql 25 Aug 2005 01:04:38 - *** COPY main_table (a, b) FROM stdin; *** 253,255 --- 253,296 \. SELECT * FROM main_table ORDER BY a, b; + + -- Test enable/disable triggers + + create table trigtest (i serial primary key); + -- test that disabling RI triggers works + create table trigtest2 (i int references trigtest(i) on delete cascade); + + create function trigtest() returns trigger as $$ + begin + raise notice '% % % %', TG_RELNAME, TG_OP, TG_WHEN, TG_LEVEL; + return new; + end;$$ language plpgsql; + + create trigger trigtest_b_row_tg before insert o
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Thanks, modified patch applied by Tom, with the addition of a USER triggers only mode. --- Satoshi Nagayasu wrote: > The message format for elog() report is cleaned up. > > -- > NAGAYASU Satoshi <[EMAIL PROTECTED]> > diff -cr pgsql.orig/src/backend/commands/tablecmds.c > pgsql/src/backend/commands/tablecmds.c > *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 > 14:08:54.0 +0900 > --- pgsql/src/backend/commands/tablecmds.c2005-08-08 13:46:44.0 > +0900 > *** > *** 236,241 > --- 236,243 -- 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
The message format for elog() report is cleaned up. -- NAGAYASU Satoshi <[EMAIL PROTECTED]> diff -cr pgsql.orig/src/backend/commands/tablecmds.c pgsql/src/backend/commands/tablecmds.c *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 14:08:54.0 +0900 --- pgsql/src/backend/commands/tablecmds.c 2005-08-08 13:46:44.0 +0900 *** *** 236,241 --- 236,243 Oid newOwnerId); static void ATExecClusterOn(Relation rel, const char *indexName); static void ATExecDropCluster(Relation rel); + static void ATExecEnableDisableTrigger(Relation rel, char *trigname, + bool enable); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); *** *** 1993,1998 --- 1995,2005 } pass = AT_PASS_DROP; break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATSimplePermissions(rel, false); + pass = AT_PASS_MISC; + break; case AT_SetTableSpace: /* SET TABLESPACE */ /* This command never recurses */ ATPrepSetTableSpace(tab, rel, cmd->name); *** *** 2155,2160 --- 2162,2173 * Nothing to do here; Phase 3 does the work */ break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, true); + break; + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, false); + break; default:/* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); *** *** 5465,5470 --- 5478,5492 } /* + * ALTER TABLE ENABLE/DISABLE TRIGGER + */ + static void + ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) + { + EnableDisableTrigger(rel, trigname, enable); + } + + /* * ALTER TABLE SET TABLESPACE */ static void diff -cr pgsql.orig/src/backend/commands/trigger.c pgsql/src/backend/commands/trigger.c *** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 +0900 --- pgsql/src/backend/commands/trigger.c2005-08-16 11:23:47.0 +0900 *** *** 3063,3065 --- 3063,3174 afterTriggerAddEvent(new_event); } } + + /* -- + * EnableDisableTrigger() + * + *Called by ALTER TABLE ENABLE/DISABLE TRIGGER + * to change 'tgenabled' flag in the pg_trigger. + * -- + */ + void + EnableDisableTrigger(Relation rel, const char *tgname, bool enable) + { + Relation tgrel; + SysScanDesc tgscan; + ScanKeyData keys[2]; + HeapTuple tuple; + int nkeys; + int changed; + + /* Permissions checks */ + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, + RelationGetRelationName(rel)); + + if (!allowSystemTableMods && IsSystemRelation(rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("permission denied: \"%s\" is a system catalog", + RelationGetRelationName(rel; + + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); + + nkeys = 2; + changed = 0; + if ( strcmp(tgname, "*")==0 ) + { + if ( !superuser() ) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("ENABLE/DISABLE TRIGGER ALL is allowed superuser only."))); + + nkeys = 1; + } + + ScanKeyInit(&keys[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + ScanKeyInit(&keys[1], + Anum_pg_trigger_tgname, + BTEqualStrategyNumber, F_NAMEEQ, + CStringGetDatum(tgname)); + + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, +
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
> This isn't really a gain in localizability because it assumes that there > are only singular and plural forms. I do agree that plugging words like > "enabled" or "disabled" into a string is not good style. Please read > the message style guidelines at > http://developer.postgresql.org/docs/postgres/error-style-guide.html > particularly the section about writing localization-friendly messages > http://developer.postgresql.org/docs/postgres/nls-programmer.html#NLS-GUIDELINES I did not know about this. I'll check my code for this style. Thanks. -- NAGAYASU Satoshi <[EMAIL PROTECTED]> ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Alvaro Herrera <[EMAIL PROTECTED]> writes: >> What does "really be two messages" mean? > I mean you should do this: > if (enabled) > { > if (changed == 1) > ereport("One trigger on %s enabled") > else > ereport("%d triggers on %d enabled") > } This isn't really a gain in localizability because it assumes that there are only singular and plural forms. I do agree that plugging words like "enabled" or "disabled" into a string is not good style. Please read the message style guidelines at http://developer.postgresql.org/docs/postgres/error-style-guide.html particularly the section about writing localization-friendly messages http://developer.postgresql.org/docs/postgres/nls-programmer.html#NLS-GUIDELINES regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
On Mon, Aug 08, 2005 at 02:13:28PM +0900, Satoshi Nagayasu wrote: > Alvaro Herrera wrote: > >>+ elog(NOTICE, "%d trigger(s) on %s %s.", > >>+changed, > >>+NameStr(rel->rd_rel->relname), > >>+enable ? "enabled" : "disabled"); > > > > > > should really be two messages (Maybe even four: disabled-plural, > > disabled-singular, enabled-plural, enabled-singular) > > What does "really be two messages" mean? I mean you should do this: if (enabled) { if (changed == 1) ereport("One trigger on %s enabled") else ereport("%d triggers on %d enabled") } etc. -- Alvaro Herrera () "I personally became interested in Linux while I was dating an English major who wouldn't know an operating system if it walked up and bit him." (Val Henson) ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Alvaro Herrera wrote: > There are a few elog() calls that should really be ereport(). Also > this message I've fixed to call ereport() on permission error. >>+ elog(NOTICE, "%d trigger(s) on %s %s.", >>+ changed, >>+ NameStr(rel->rd_rel->relname), >>+ enable ? "enabled" : "disabled"); > > > should really be two messages (Maybe even four: disabled-plural, > disabled-singular, enabled-plural, enabled-singular) What does "really be two messages" mean? > There's a SQL typo here: > >>+ appendPQExpBuffer(query, "ALTER TABLE %s DIABLE TRIGGER %s;\n", Fixed. -- NAGAYASU Satoshi <[EMAIL PROTECTED]> ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
On Mon, Aug 08, 2005 at 08:07:05AM +0900, Satoshi Nagayasu wrote: > Here is new patch with pg_dump modification. There are a few elog() calls that should really be ereport(). Also this message > + elog(NOTICE, "%d trigger(s) on %s %s.", > + changed, > + NameStr(rel->rd_rel->relname), > + enable ? "enabled" : "disabled"); should really be two messages (Maybe even four: disabled-plural, disabled-singular, enabled-plural, enabled-singular) There's a SQL typo here: > + appendPQExpBuffer(query, "ALTER TABLE %s DIABLE TRIGGER %s;\n", -- Alvaro Herrera () "La conclusiĆ³n que podemos sacar de esos estudios es que no podemos sacar ninguna conclusiĆ³n de ellos" (Tanenbaum) ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Here is new patch with pg_dump modification. Bruce Momjian wrote: > I am waiting for pg_dump support for this patch. > > --- > > Satoshi Nagayasu wrote: > >>Bruce Momjian wrote: >> >>>I am not sure what to do with this patch. It is missing dump >>>capability, there is no clause to disable all triggers on a table, and >>>it uses a table owner check when a super user check is required (because >>>of referential integrity). >>> >>>Satoshi, are you still working on it? If not does someone else want to >>>complete it? I realized you just started on it but the deadline is >>>soon. >> >>I've already implemented 'ENABLE/DISABLE TRIGGER ALL', >>and the superuser check has been added. Please check it. >> >>And, I'm going to have a business trip to Sydney this weekend, >>so I'll complete pg_dump stuffs while my flight. >> >>Thank you. >>-- >>NAGAYASU Satoshi <[EMAIL PROTECTED]> > > >>diff -cr pgsql.orig/src/backend/commands/tablecmds.c >>pgsql/src/backend/commands/tablecmds.c >>*** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 >>14:08:54.0 +0900 >>--- pgsql/src/backend/commands/tablecmds.c2005-07-01 15:50:27.0 >>+0900 >>*** >>*** 236,241 >>--- 236,243 >> >> Oid newOwnerId); >> static void ATExecClusterOn(Relation rel, const char *indexName); >> static void ATExecDropCluster(Relation rel); >>+ static void ATExecEnableDisableTrigger(Relation rel, char *trigname, >>+bool >>enable); >> static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, >> char *tablespacename); >> static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); >>*** >>*** 1993,1998 >>--- 1995,2005 >> } >> pass = AT_PASS_DROP; >> break; >>+ case AT_EnableTrig: /* ENABLE TRIGGER */ >>+ case AT_DisableTrig:/* DISABLE TRIGGER */ >>+ ATSimplePermissions(rel, false); >>+ pass = AT_PASS_MISC; >>+ break; >> case AT_SetTableSpace: /* SET TABLESPACE */ >> /* This command never recurses */ >> ATPrepSetTableSpace(tab, rel, cmd->name); >>*** >>*** 2155,2160 >>--- 2162,2173 >> * Nothing to do here; Phase 3 does the work >> */ >> break; >>+ case AT_EnableTrig: /* ENABLE TRIGGER */ >>+ ATExecEnableDisableTrigger(rel, cmd->name, true); >>+ break; >>+ case AT_DisableTrig:/* DISABLE TRIGGER */ >>+ ATExecEnableDisableTrigger(rel, cmd->name, false); >>+ break; >> default:/* oops */ >> elog(ERROR, "unrecognized alter table type: %d", >> (int) cmd->subtype); >>*** >>*** 5465,5470 >>--- 5478,5492 >> } >> >> /* >>+ * ALTER TABLE ENABLE/DISABLE TRIGGER >>+ */ >>+ static void >>+ ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) >>+ { >>+ EnableDisableTrigger(rel, trigname, enable); >>+ } >>+ >>+ /* >> * ALTER TABLE SET TABLESPACE >> */ >> static void >>diff -cr pgsql.orig/src/backend/commands/trigger.c >>pgsql/src/backend/commands/trigger.c >>*** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 >>+0900 >>--- pgsql/src/backend/commands/trigger.c 2005-07-04 10:40:27.0 >>+0900 >>*** >>*** 3063,3065 >>--- 3063,3158 >> afterTriggerAddEvent(new_event); >> } >> } >>+ >>+ /* -- >>+ * EnableDisableTrigger() >>+ * >>+ * Called by ALTER TABLE ENABLE/DISABLE TRIGGER >>+ * to change 'tgenabled' flag in the pg_trigger. >>+ * -- >>+ */ >>+ void >>+ EnableDisableTrigger(Relation rel, const char *tgname, bool enable) >>+ { >>+ Relation tgrel; >>+ SysScanDesc tgscan; >>+ ScanKeyData keys[2]; >>+ HeapTuple tuple; >>+ int nkeys; >>+ int changed; >>+ >>+ /* Permissions checks */ >>+ if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) >>+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, >>+RelationGetRelationName(rel)); >>+ >>+ if (!allowSystemTableMods && IsSystemRelation(rel)) >>+ ereport(ERROR, >>+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), >>+ errmsg("permission denied: \"%s\" is a system >>catalog", >>+ RelationGetRelationName(rel
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
I am waiting for pg_dump support for this patch. --- Satoshi Nagayasu wrote: > Bruce Momjian wrote: > > I am not sure what to do with this patch. It is missing dump > > capability, there is no clause to disable all triggers on a table, and > > it uses a table owner check when a super user check is required (because > > of referential integrity). > > > > Satoshi, are you still working on it? If not does someone else want to > > complete it? I realized you just started on it but the deadline is > > soon. > > I've already implemented 'ENABLE/DISABLE TRIGGER ALL', > and the superuser check has been added. Please check it. > > And, I'm going to have a business trip to Sydney this weekend, > so I'll complete pg_dump stuffs while my flight. > > Thank you. > -- > NAGAYASU Satoshi <[EMAIL PROTECTED]> > diff -cr pgsql.orig/src/backend/commands/tablecmds.c > pgsql/src/backend/commands/tablecmds.c > *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 > 14:08:54.0 +0900 > --- pgsql/src/backend/commands/tablecmds.c2005-07-01 15:50:27.0 > +0900 > *** > *** 236,241 > --- 236,243 > > Oid newOwnerId); > static void ATExecClusterOn(Relation rel, const char *indexName); > static void ATExecDropCluster(Relation rel); > + static void ATExecEnableDisableTrigger(Relation rel, char *trigname, > +bool > enable); > static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, > char *tablespacename); > static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); > *** > *** 1993,1998 > --- 1995,2005 > } > pass = AT_PASS_DROP; > break; > + case AT_EnableTrig: /* ENABLE TRIGGER */ > + case AT_DisableTrig:/* DISABLE TRIGGER */ > + ATSimplePermissions(rel, false); > + pass = AT_PASS_MISC; > + break; > case AT_SetTableSpace: /* SET TABLESPACE */ > /* This command never recurses */ > ATPrepSetTableSpace(tab, rel, cmd->name); > *** > *** 2155,2160 > --- 2162,2173 >* Nothing to do here; Phase 3 does the work >*/ > break; > + case AT_EnableTrig: /* ENABLE TRIGGER */ > + ATExecEnableDisableTrigger(rel, cmd->name, true); > + break; > + case AT_DisableTrig:/* DISABLE TRIGGER */ > + ATExecEnableDisableTrigger(rel, cmd->name, false); > + break; > default:/* oops */ > elog(ERROR, "unrecognized alter table type: %d", >(int) cmd->subtype); > *** > *** 5465,5470 > --- 5478,5492 > } > > /* > + * ALTER TABLE ENABLE/DISABLE TRIGGER > + */ > + static void > + ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) > + { > + EnableDisableTrigger(rel, trigname, enable); > + } > + > + /* >* ALTER TABLE SET TABLESPACE >*/ > static void > diff -cr pgsql.orig/src/backend/commands/trigger.c > pgsql/src/backend/commands/trigger.c > *** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 > +0900 > --- pgsql/src/backend/commands/trigger.c 2005-07-04 10:40:27.0 > +0900 > *** > *** 3063,3065 > --- 3063,3158 > afterTriggerAddEvent(new_event); > } > } > + > + /* -- > + * EnableDisableTrigger() > + * > + * Called by ALTER TABLE ENABLE/DISABLE TRIGGER > + * to change 'tgenabled' flag in the pg_trigger. > + * -- > + */ > + void > + EnableDisableTrigger(Relation rel, const char *tgname, bool enable) > + { > + Relation tgrel; > + SysScanDesc tgscan; > + ScanKeyData keys[2]; > + HeapTuple tuple; > + int nkeys; > + int changed; > + > + /* Permissions checks */ > + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) > + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, > +RelationGetRelationName(rel)); > + > + if (!allowSystemTableMods && IsSystemRelation(rel)) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > + errmsg("permission denied: \"%s\" is a system > catalog", > + RelationGetRelationName(rel; > + > + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); > + > +
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Satoshi Nagayasu wrote: > Bruce Momjian wrote: > > I am not sure what to do with this patch. It is missing dump > > capability, there is no clause to disable all triggers on a table, and > > it uses a table owner check when a super user check is required (because > > of referential integrity). > > > > Satoshi, are you still working on it? If not does someone else want to > > complete it? I realized you just started on it but the deadline is > > soon. > > I've already implemented 'ENABLE/DISABLE TRIGGER ALL', > and the superuser check has been added. Please check it. > > And, I'm going to have a business trip to Sydney this weekend, > so I'll complete pg_dump stuffs while my flight. OK, but once our patch queue is empty, we will need to process your patch. My guess is that we are several days away from that. -- 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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Bruce Momjian wrote: > I am not sure what to do with this patch. It is missing dump > capability, there is no clause to disable all triggers on a table, and > it uses a table owner check when a super user check is required (because > of referential integrity). > > Satoshi, are you still working on it? If not does someone else want to > complete it? I realized you just started on it but the deadline is > soon. I've already implemented 'ENABLE/DISABLE TRIGGER ALL', and the superuser check has been added. Please check it. And, I'm going to have a business trip to Sydney this weekend, so I'll complete pg_dump stuffs while my flight. Thank you. -- NAGAYASU Satoshi <[EMAIL PROTECTED]> diff -cr pgsql.orig/src/backend/commands/tablecmds.c pgsql/src/backend/commands/tablecmds.c *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 14:08:54.0 +0900 --- pgsql/src/backend/commands/tablecmds.c 2005-07-01 15:50:27.0 +0900 *** *** 236,241 --- 236,243 Oid newOwnerId); static void ATExecClusterOn(Relation rel, const char *indexName); static void ATExecDropCluster(Relation rel); + static void ATExecEnableDisableTrigger(Relation rel, char *trigname, + bool enable); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); *** *** 1993,1998 --- 1995,2005 } pass = AT_PASS_DROP; break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATSimplePermissions(rel, false); + pass = AT_PASS_MISC; + break; case AT_SetTableSpace: /* SET TABLESPACE */ /* This command never recurses */ ATPrepSetTableSpace(tab, rel, cmd->name); *** *** 2155,2160 --- 2162,2173 * Nothing to do here; Phase 3 does the work */ break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, true); + break; + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, false); + break; default:/* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); *** *** 5465,5470 --- 5478,5492 } /* + * ALTER TABLE ENABLE/DISABLE TRIGGER + */ + static void + ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) + { + EnableDisableTrigger(rel, trigname, enable); + } + + /* * ALTER TABLE SET TABLESPACE */ static void diff -cr pgsql.orig/src/backend/commands/trigger.c pgsql/src/backend/commands/trigger.c *** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 +0900 --- pgsql/src/backend/commands/trigger.c2005-07-04 10:40:27.0 +0900 *** *** 3063,3065 --- 3063,3158 afterTriggerAddEvent(new_event); } } + + /* -- + * EnableDisableTrigger() + * + *Called by ALTER TABLE ENABLE/DISABLE TRIGGER + * to change 'tgenabled' flag in the pg_trigger. + * -- + */ + void + EnableDisableTrigger(Relation rel, const char *tgname, bool enable) + { + Relation tgrel; + SysScanDesc tgscan; + ScanKeyData keys[2]; + HeapTuple tuple; + int nkeys; + int changed; + + /* Permissions checks */ + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, + RelationGetRelationName(rel)); + + if (!allowSystemTableMods && IsSystemRelation(rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("permission denied: \"%s\" is a system catalog", + RelationGetRelationName(rel; + + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); + + nkeys = 2; + changed = 0; + if ( strcmp(tgname, "*")==0 ) + { + if ( !superuser() ) + elog(ERROR, "ENABLE/DISABLE TRIGGER ALL is allowed superuser only."); + + nkeys = 1; + } + + ScanKeyInit(&keys[0], +
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Bruce Momjian wrote: I am not sure what to do with this patch. It is missing dump capability, there is no clause to disable all triggers on a table, and it uses a table owner check when a super user check is required (because of referential integrity). From a user's view, a trigger implementing RI isn't a trigger but an implementation detail he shouldn't need to care about. So for std triggers, owner check should be sufficient, requiring superuser for RI triggers only. This impacts EN/DISABLE TRIGGER ALL too. To touch RI triggers as well, an additional keyword is needed. Regards, Andreas ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
I am not sure what to do with this patch. It is missing dump capability, there is no clause to disable all triggers on a table, and it uses a table owner check when a super user check is required (because of referential integrity). Satoshi, are you still working on it? If not does someone else want to complete it? I realized you just started on it but the deadline is soon. --- Satoshi Nagayasu wrote: > There is one more fix... > > > + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, > > + SnapshotNow, 1, > > keys); > > '1' was incorrect, must be '2'. > > > + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, > > + SnapshotNow, 2, > > keys); > > -- > NAGAYASU Satoshi <[EMAIL PROTECTED]> > diff -cr pgsql.orig/src/backend/commands/tablecmds.c > pgsql/src/backend/commands/tablecmds.c > *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 > 14:08:54.0 +0900 > --- pgsql/src/backend/commands/tablecmds.c2005-07-01 15:50:27.0 > +0900 > *** > *** 236,241 > --- 236,243 > > Oid newOwnerId); > static void ATExecClusterOn(Relation rel, const char *indexName); > static void ATExecDropCluster(Relation rel); > + static void ATExecEnableDisableTrigger(Relation rel, char *trigname, > +bool > enable); > static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, > char *tablespacename); > static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); > *** > *** 1993,1998 > --- 1995,2005 > } > pass = AT_PASS_DROP; > break; > + case AT_EnableTrig: /* ENABLE TRIGGER */ > + case AT_DisableTrig:/* DISABLE TRIGGER */ > + ATSimplePermissions(rel, false); > + pass = AT_PASS_MISC; > + break; > case AT_SetTableSpace: /* SET TABLESPACE */ > /* This command never recurses */ > ATPrepSetTableSpace(tab, rel, cmd->name); > *** > *** 2155,2160 > --- 2162,2173 >* Nothing to do here; Phase 3 does the work >*/ > break; > + case AT_EnableTrig: /* ENABLE TRIGGER */ > + ATExecEnableDisableTrigger(rel, cmd->name, true); > + break; > + case AT_DisableTrig:/* DISABLE TRIGGER */ > + ATExecEnableDisableTrigger(rel, cmd->name, false); > + break; > default:/* oops */ > elog(ERROR, "unrecognized alter table type: %d", >(int) cmd->subtype); > *** > *** 5465,5470 > --- 5478,5492 > } > > /* > + * ALTER TABLE ENABLE/DISABLE TRIGGER > + */ > + static void > + ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) > + { > + EnableDisableTrigger(rel, trigname, enable); > + } > + > + /* >* ALTER TABLE SET TABLESPACE >*/ > static void > diff -cr pgsql.orig/src/backend/commands/trigger.c > pgsql/src/backend/commands/trigger.c > *** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 > +0900 > --- pgsql/src/backend/commands/trigger.c 2005-07-01 17:21:44.0 > +0900 > *** > *** 3063,3065 > --- 3063,3132 > afterTriggerAddEvent(new_event); > } > } > + > + /* -- > + * EnableDisableTrigger() > + * > + * Called by ALTER TABLE ENABLE/DISABLE TRIGGER > + * to change 'tgenabled' flag in the pg_trigger. > + * -- > + */ > + void > + EnableDisableTrigger(Relation rel, const char *tgname, bool enable) > + { > + Relation tgrel; > + SysScanDesc tgscan; > + ScanKeyData keys[2]; > + HeapTuple tuple; > + > + /* Permissions checks */ > + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) > + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, > +RelationGetRelationName(rel)); > + > + if (!allowSystemTableMods && IsSystemRelation(rel)) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > + errmsg("permission denied: \"%s\" is a system > catalog", > + RelationGetRelationName(rel; > + > + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); > +
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Gavin Sherry wrote: > On Fri, 1 Jul 2005, Satoshi Nagayasu wrote: > > > Hi all, > > > > Here is a first patch to allow these commands. > > > > > ALTER TABLE ENABLE TRIGGER > > > ALTER TABLE DISABLE TRIGGER > > > > Hmmm.. just thinking about it for a second. I wonder if we should also > support: > > ALTER TABLE DISABLE TRIGGERS > > which would disable all triggers on the table. We would have a > complimentary ENABLE TRIGGERS as well, obviously. The reason I say this is > that the common case will be that people are doing a bulk load and want to > disable all triggers. However, this will be very useful for debugging > interactions between triggers on a table so a user might want to disable > only one of many triggers -- as your current grammar does. > > Perhaps a way of making the grammar a little less ambiguous would be to > have the following to disable all triggers: > > ALTER TABLE DISABLE TRIGGERS > > and the following to disable one: > > ALTER TRIGGER DISABLE The proposed syntax in TODO.detail/trigger is: ALTER TABLE foo {DISABLE|ENABLE} TRIGGER { ALL | trigger_name [ ,... ] }; which would cleanly allow all triggers of a table to be disabled. -- 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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Satoshi Nagayasu wrote: >Hi all, > >Here is a first patch to allow these commands. > > > >>ALTER TABLE ENABLE TRIGGER >>ALTER TABLE DISABLE TRIGGER >> >> > >Bruce said to allow them only super-user, >but currently this patch allows also the table owner. > > > It would be convenient if all triggers could be disabled with a single command. More precise: option 1: All triggers except for RI triggers (EN/DISABLE TRIGGER ALL) option 2: really all triggers including RI triggers (superuser only) Regards, Andreas ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Christopher Kings-Lynne <[EMAIL PROTECTED]> writes: > The table owner can drop and create triggers - so why shouldn't they be > able to enable and disable them? Not triggers belonging to RI constraints on other tables. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
On Fri, 1 Jul 2005, Satoshi Nagayasu wrote: > Hi, > > Gavin Sherry wrote: > > Hmmm.. just thinking about it for a second. I wonder if we should also > > support: > > > > ALTER TABLE DISABLE TRIGGERS > > I found some RDBMSes are supporting 'DISABLE TRIGGER ALL TRIGGERS' command. > > Does anyone know about the SQL99 spec? The spec says nothing about disabling triggers. Gavin ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Hi, Gavin Sherry wrote: > Hmmm.. just thinking about it for a second. I wonder if we should also > support: > > ALTER TABLE DISABLE TRIGGERS I found some RDBMSes are supporting 'DISABLE TRIGGER ALL TRIGGERS' command. Does anyone know about the SQL99 spec? -- NAGAYASU Satoshi <[EMAIL PROTECTED]> ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
On Fri, 1 Jul 2005, Satoshi Nagayasu wrote: > Hi all, > > Here is a first patch to allow these commands. > > > ALTER TABLE ENABLE TRIGGER > > ALTER TABLE DISABLE TRIGGER > Hmmm.. just thinking about it for a second. I wonder if we should also support: ALTER TABLE DISABLE TRIGGERS which would disable all triggers on the table. We would have a complimentary ENABLE TRIGGERS as well, obviously. The reason I say this is that the common case will be that people are doing a bulk load and want to disable all triggers. However, this will be very useful for debugging interactions between triggers on a table so a user might want to disable only one of many triggers -- as your current grammar does. Perhaps a way of making the grammar a little less ambiguous would be to have the following to disable all triggers: ALTER TABLE DISABLE TRIGGERS and the following to disable one: ALTER TRIGGER DISABLE Just an idea. Thanks, Gavin ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
On Fri, 1 Jul 2005, Satoshi Nagayasu wrote: > Hi all, > > Here is a first patch to allow these commands. > > > ALTER TABLE ENABLE TRIGGER > > ALTER TABLE DISABLE TRIGGER There are three other areas which are worth looking at: a) We may defer the execution of some triggers to the end of the transaction. Do we execute those if a they were later disabled? b) There is a bug in how we execute triggers. For example, in ExecDelete(): booldodelete; dodelete = ExecBRDeleteTriggers(estate, resultRelInfo, tupleid, estate->es_snapshot->curcid); if (!dodelete) /* "do nothing" */ return; This means that if a before trigger returned NULL, we short circuit and do not delete the tuple. Consider the following in ExecBRDeleteTriggers() HeapTuple newtuple = NULL; ... for (i = 0; i < ntrigs; i++) { Trigger*trigger = &trigdesc->triggers[tgindx[i]]; if (!trigger->tgenabled) continue; LocTriggerData.tg_trigtuple = trigtuple; LocTriggerData.tg_trigtuplebuf = InvalidBuffer; LocTriggerData.tg_trigger = trigger; newtuple = ExecCallTriggerFunc(&LocTriggerData, tgindx[i], relinfo->ri_TrigFunctions, relinfo->ri_TrigInstrument, GetPerTupleMemoryContext(estate)); if (newtuple == NULL) break; if (newtuple != trigtuple) heap_freetuple(newtuple); } ... return (newtuple == NULL) ? false : true; This means that if all triggers on a table are disabled, we tell the caller that a trigger returned NULL and that we should short circuit. This does not seem to be the case for the other DML statements. c) There has been a push over previous releases to make dumps generated by pg_dump look like ANSI SQL. Now, ALTER TABLE ... DISABLE trigger is useful for pg_dump but not compliant. Others have suggested something like: SET enable_triggers = off This would turn all triggers off in the current session. It has the added benefit that it does not affect other sessions. It does introduce some issues with permissions -- I wouldn't want users turning off data validation before triggers in my database, but that's me. I'm not enamoured of the idea but it is worth discussing, I believe. Also, a final patch will also need documentation and regression tests :-) Thanks, Gavin ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
There is one more fix... > + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, > + SnapshotNow, 1, > keys); '1' was incorrect, must be '2'. > + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, > + SnapshotNow, 2, > keys); -- NAGAYASU Satoshi <[EMAIL PROTECTED]> diff -cr pgsql.orig/src/backend/commands/tablecmds.c pgsql/src/backend/commands/tablecmds.c *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 14:08:54.0 +0900 --- pgsql/src/backend/commands/tablecmds.c 2005-07-01 15:50:27.0 +0900 *** *** 236,241 --- 236,243 Oid newOwnerId); static void ATExecClusterOn(Relation rel, const char *indexName); static void ATExecDropCluster(Relation rel); + static void ATExecEnableDisableTrigger(Relation rel, char *trigname, + bool enable); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); *** *** 1993,1998 --- 1995,2005 } pass = AT_PASS_DROP; break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATSimplePermissions(rel, false); + pass = AT_PASS_MISC; + break; case AT_SetTableSpace: /* SET TABLESPACE */ /* This command never recurses */ ATPrepSetTableSpace(tab, rel, cmd->name); *** *** 2155,2160 --- 2162,2173 * Nothing to do here; Phase 3 does the work */ break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, true); + break; + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, false); + break; default:/* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); *** *** 5465,5470 --- 5478,5492 } /* + * ALTER TABLE ENABLE/DISABLE TRIGGER + */ + static void + ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) + { + EnableDisableTrigger(rel, trigname, enable); + } + + /* * ALTER TABLE SET TABLESPACE */ static void diff -cr pgsql.orig/src/backend/commands/trigger.c pgsql/src/backend/commands/trigger.c *** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 +0900 --- pgsql/src/backend/commands/trigger.c2005-07-01 17:21:44.0 +0900 *** *** 3063,3065 --- 3063,3132 afterTriggerAddEvent(new_event); } } + + /* -- + * EnableDisableTrigger() + * + *Called by ALTER TABLE ENABLE/DISABLE TRIGGER + * to change 'tgenabled' flag in the pg_trigger. + * -- + */ + void + EnableDisableTrigger(Relation rel, const char *tgname, bool enable) + { + Relation tgrel; + SysScanDesc tgscan; + ScanKeyData keys[2]; + HeapTuple tuple; + + /* Permissions checks */ + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, + RelationGetRelationName(rel)); + + if (!allowSystemTableMods && IsSystemRelation(rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("permission denied: \"%s\" is a system catalog", + RelationGetRelationName(rel; + + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); + + ScanKeyInit(&keys[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + ScanKeyInit(&keys[1], + Anum_pg_trigger_tgname, + BTEqualStrategyNumber, F_NAMEEQ, + CStringGetDatum(tgname)); + + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, + SnapshotNow, 2, keys); + + while (HeapTupleIsVa
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Thanks for comments, Neil. Some are fixed. Neil Conway wrote: > Also, you should probably skip the simple_heap_update() if the user > tries to disable an already-disabled trigger, to avoid pointless MVCC > bloat (and same for enabling an already-enabled trigger, of course). Do we need some log/warning message for the user in these cases? > If someone pg_dump's a table with a disabled trigger, should the dump > enable or disable the trigger? I'd be inclined to say pg_dump should > preserve the state of the database it is dumping, and so the trigger > should be disabled. In that case I believe pg_dump needs to be updated. I think so too. > The patch needs to update the documentation and add regression tests. > psql tab completion might also be worth adding. I'll dive into pg_dump and psql this weekend... -- NAGAYASU Satoshi <[EMAIL PROTECTED]> diff -cr pgsql.orig/src/backend/commands/tablecmds.c pgsql/src/backend/commands/tablecmds.c *** pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 14:08:54.0 +0900 --- pgsql/src/backend/commands/tablecmds.c 2005-07-01 15:50:27.0 +0900 *** *** 236,241 --- 236,243 Oid newOwnerId); static void ATExecClusterOn(Relation rel, const char *indexName); static void ATExecDropCluster(Relation rel); + static void ATExecEnableDisableTrigger(Relation rel, char *trigname, + bool enable); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); *** *** 1993,1998 --- 1995,2005 } pass = AT_PASS_DROP; break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATSimplePermissions(rel, false); + pass = AT_PASS_MISC; + break; case AT_SetTableSpace: /* SET TABLESPACE */ /* This command never recurses */ ATPrepSetTableSpace(tab, rel, cmd->name); *** *** 2155,2160 --- 2162,2173 * Nothing to do here; Phase 3 does the work */ break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, true); + break; + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, false); + break; default:/* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); *** *** 5465,5470 --- 5478,5492 } /* + * ALTER TABLE ENABLE/DISABLE TRIGGER + */ + static void + ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) + { + EnableDisableTrigger(rel, trigname, enable); + } + + /* * ALTER TABLE SET TABLESPACE */ static void diff -cr pgsql.orig/src/backend/commands/trigger.c pgsql/src/backend/commands/trigger.c *** pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 +0900 --- pgsql/src/backend/commands/trigger.c2005-07-01 17:21:44.0 +0900 *** *** 3063,3065 --- 3063,3132 afterTriggerAddEvent(new_event); } } + + /* -- + * EnableDisableTrigger() + * + *Called by ALTER TABLE ENABLE/DISABLE TRIGGER + * to change 'tgenabled' flag in the pg_trigger. + * -- + */ + void + EnableDisableTrigger(Relation rel, const char *tgname, bool enable) + { + Relation tgrel; + SysScanDesc tgscan; + ScanKeyData keys[2]; + HeapTuple tuple; + + /* Permissions checks */ + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, + RelationGetRelationName(rel)); + + if (!allowSystemTableMods && IsSystemRelation(rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("permission denied: \"%s\" is a system catalog", + RelationGetRelationName(rel; + + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); + + ScanKeyInit(&keys[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectId
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Hi, BTW, the Postgres convention is to submit patches in context diff format (diff -c). Satoshi Nagayasu wrote: + while (HeapTupleIsValid(tuple = systable_getnext(tgscan))) + { + HeapTuple newtup = heap_copytuple(tuple); + Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(newtup); + + if ( pg_trigger->tgenabled==true && enable==false ) + { + pg_trigger->tgenabled = false; + } + else if ( pg_trigger->tgenabled==false && enable==true ) + { + pg_trigger->tgenabled = true; + } + + simple_heap_update(tgrel, &newtup->t_self, newtup); Wouldn't it just be easier to set pg_trigger->tgenabled = enable ? Also, you should probably skip the simple_heap_update() if the user tries to disable an already-disabled trigger, to avoid pointless MVCC bloat (and same for enabling an already-enabled trigger, of course). --- pgsql.orig/src/backend/parser/gram.y2005-06-30 05:34:13.0 +0900 +++ pgsql/src/backend/parser/gram.y 2005-07-01 14:21:25.0 +0900 @@ -348,9 +348,9 @@ DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS - DESC DISTINCT DO DOMAIN_P DOUBLE_P DROP + DESC DISABLE DISTINCT DO DOMAIN_P DOUBLE_P DROP - EACH ELSE ENCODING ENCRYPTED END_P ESCAPE EXCEPT EXCLUDING + EACH ELSE ENABLE ENCODING ENCRYPTED END_P ESCAPE EXCEPT EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN EXTERNAL EXTRACT You also need to add these to unreserved_keywords (see gram.y:7903). --- pgsql.orig/src/include/commands/trigger.h 2005-05-30 16:20:58.0 +0900 +++ pgsql/src/include/commands/trigger.h2005-07-01 15:50:09.0 +0900 @@ -164,6 +164,7 @@ extern void AfterTriggerSetState(ConstraintsSetStmt *stmt); +void EnableDisableTrigger(Relation rel, const char *tgname, bool enable); The Postgres convention is to use the "extern" keyword in declarations of global functions. If someone pg_dump's a table with a disabled trigger, should the dump enable or disable the trigger? I'd be inclined to say pg_dump should preserve the state of the database it is dumping, and so the trigger should be disabled. In that case I believe pg_dump needs to be updated. The patch needs to update the documentation and add regression tests. psql tab completion might also be worth adding. -Neil ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
> I said why _shouldn't_. I was agreeing with you. Oops. Sorry. I don't know why only super-user shold be able to enable/disable tirggers. I believe the table owner want to enable/disable triggers, because I always need it. Loading huge data set into the table with triggers(or fk) is very painful. Christopher Kings-Lynne wrote: > > Satoshi Nagayasu wrote: > >>>The table owner can drop and create triggers - so why shouldn't they be >>>able to enable and disable them? >> >> >>For convenience or easy operation. >> >>I believe the user doesn't like to create same triggers again and again. > > > I said why _shouldn't_. I was agreeing with you. > > Chris > > -- NAGAYASU Satoshi <[EMAIL PROTECTED]> ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Satoshi Nagayasu wrote: >>The table owner can drop and create triggers - so why shouldn't they be >>able to enable and disable them? > > > For convenience or easy operation. > > I believe the user doesn't like to create same triggers again and again. I said why _shouldn't_. I was agreeing with you. Chris ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
> The table owner can drop and create triggers - so why shouldn't they be > able to enable and disable them? For convenience or easy operation. I believe the user doesn't like to create same triggers again and again. Christopher Kings-Lynne wrote: >>>ALTER TABLE ENABLE TRIGGER >>>ALTER TABLE DISABLE TRIGGER >> >> >>Bruce said to allow them only super-user, >>but currently this patch allows also the table owner. > > > The table owner can drop and create triggers - so why shouldn't they be > able to enable and disable them? > > Chris > > -- NAGAYASU Satoshi <[EMAIL PROTECTED]> ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
>>ALTER TABLE ENABLE TRIGGER >>ALTER TABLE DISABLE TRIGGER > > > Bruce said to allow them only super-user, > but currently this patch allows also the table owner. The table owner can drop and create triggers - so why shouldn't they be able to enable and disable them? Chris ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
[PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Hi all, Here is a first patch to allow these commands. > ALTER TABLE ENABLE TRIGGER > ALTER TABLE DISABLE TRIGGER Bruce said to allow them only super-user, but currently this patch allows also the table owner. If we need to restrict these operations, we have to add more user checks. > From: Bruce Momjian > Date: 2005/06/29 20:49 > Subject: Re: [HACKERS] Open items > To: Satoshi Nagayasu <[EMAIL PROTECTED]> > Cc: PostgreSQL-development > > > Satoshi Nagayasu wrote: > >>How about enable/disable triggers? >> >>>From TODO: >> >>>Allow triggers to be disabled. >> >>http://momjian.postgresql.org/cgi-bin/pgtodo?trigger >> >>I think this is good for COPY performance improvement. >> >>Now I have user functions to enable/disable triggers, not DDL. >>It modifies system tables. >>But I can rewrite this as a DDL. (ALTER TABLE?) > > > Yea, it is a TODO item, and should be pretty straight-forward to code, > so sure, go ahead. > > It has to be something that is super-user-only. > > -- > 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 > > ---(end of broadcast)--- > TIP 4: Don't 'kill -9' the postmaster > -- NAGAYASU Satoshi <[EMAIL PROTECTED]> diff -ru pgsql.orig/src/backend/commands/tablecmds.c pgsql/src/backend/commands/tablecmds.c --- pgsql.orig/src/backend/commands/tablecmds.c 2005-06-28 14:08:54.0 +0900 +++ pgsql/src/backend/commands/tablecmds.c 2005-07-01 15:50:27.0 +0900 @@ -236,6 +236,8 @@ Oid newOwnerId); static void ATExecClusterOn(Relation rel, const char *indexName); static void ATExecDropCluster(Relation rel); +static void ATExecEnableDisableTrigger(Relation rel, char *trigname, + bool enable); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); @@ -1993,6 +1995,11 @@ } pass = AT_PASS_DROP; break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATSimplePermissions(rel, false); + pass = AT_PASS_MISC; + break; case AT_SetTableSpace: /* SET TABLESPACE */ /* This command never recurses */ ATPrepSetTableSpace(tab, rel, cmd->name); @@ -2155,6 +2162,12 @@ * Nothing to do here; Phase 3 does the work */ break; + case AT_EnableTrig: /* ENABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, true); + break; + case AT_DisableTrig:/* DISABLE TRIGGER */ + ATExecEnableDisableTrigger(rel, cmd->name, false); + break; default:/* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -5465,6 +5478,15 @@ } /* + * ALTER TABLE ENABLE/DISABLE TRIGGER + */ +static void +ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable) +{ + EnableDisableTrigger(rel, trigname, enable); +} + +/* * ALTER TABLE SET TABLESPACE */ static void diff -ru pgsql.orig/src/backend/commands/trigger.c pgsql/src/backend/commands/trigger.c --- pgsql.orig/src/backend/commands/trigger.c 2005-05-30 16:20:58.0 +0900 +++ pgsql/src/backend/commands/trigger.c2005-07-01 15:53:45.0 +0900 @@ -3063,3 +3063,74 @@ afterTriggerAddEvent(new_event); } } + +/* -- + * EnableDisableTrigger() + * + * Called by ALTER TABLE ENABLE/DISABLE TRIGGER + * to change 'tgenabled' flag in the pg_trigger. + * -- + */ +void +EnableDisableTrigger(Relation rel, const char *tgname, bool enable) +{ + Relation tgrel; + SysScanDesc tgscan; + ScanKeyData keys[2]; + HeapTuple tuple; + + /* Permissions checks */ + if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, + RelationGetRelationName(rel)); + + if (!allowSystemTableMods && IsSystemRelation(rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("permission denied: