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 or update or delete on trigtest + for each row execute
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)
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 (alvherre[a]alvh.no-ip.org)
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 [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)
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)
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;
+
+ 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.);
+
+
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 (alvherre[a]alvh.no-ip.org) 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)
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)
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);
+
+ nkeys = 2;
+ changed = 0;
+ if ( strcmp(tgname, *)==0 )
+ {
+ if ( !superuser() )
+ elog(ERROR,
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)
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],
+ Anum_pg_trigger_tgrelid,
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)
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);
+
+ ScanKeyInit(keys[0],
+ Anum_pg_trigger_tgrelid,
+ BTEqualStrategyNumber,
[PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
Hi all,
Here is a first patch to allow these commands.
ALTER TABLE table ENABLE TRIGGER trigname
ALTER TABLE table DISABLE TRIGGER trigname
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 pgman@candle.pha.pa.us
Date: 2005/06/29 20:49
Subject: Re: [HACKERS] Open items
To: Satoshi Nagayasu [EMAIL PROTECTED]
Cc: PostgreSQL-development pgsql-hackers@postgresql.org
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),
+
Re: [PATCHES] enable/disable trigger (Re: Fwd: [HACKERS] Open items)
ALTER TABLE table ENABLE TRIGGER trigname ALTER TABLE table DISABLE TRIGGER trigname 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
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 table ENABLE TRIGGER trigname ALTER TABLE table DISABLE TRIGGER trigname 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)
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)
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)
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)
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,
+
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 (HeapTupleIsValid(tuple =
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 table ENABLE TRIGGER trigname
ALTER TABLE table DISABLE TRIGGER trigname
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)
On Fri, 1 Jul 2005, Satoshi Nagayasu wrote: Hi all, Here is a first patch to allow these commands. ALTER TABLE table ENABLE TRIGGER trigname ALTER TABLE table DISABLE TRIGGER trigname 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 table DISABLE TRIGGERS and the following to disable one: ALTER TRIGGER 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)
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, 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)
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)
Satoshi Nagayasu wrote: Hi all, Here is a first patch to allow these commands. ALTER TABLE table ENABLE TRIGGER trigname ALTER TABLE table DISABLE TRIGGER trigname 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
