[PATCH] Fix trigger argument propagation to child partitions

Started by Patrick McHardyover 6 years ago3 messages
#1Patrick McHardy
kaber@trash.net
1 attachment(s)

The following patch fixes propagation of arguments to the trigger
function to child partitions both when initially creating the trigger
and when adding new partitions to a partitioned table.

The included regression test should demonstrate the problem, for clarity
repeated in slightly more readable form here:

bb=> create table parted_trig (a int) partition by list (a);
CREATE TABLE
bb=> create table parted_trig1 partition of parted_trig for values in (1);
CREATE TABLE
bb=> create or replace function trigger_notice() returns trigger as $$
bb$> declare
bb$> arg1 text = TG_ARGV[0];
bb$> arg2 integer = TG_ARGV[1];
bb$> begin
bb$> raise notice 'trigger % on % % % for % args % %', TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, arg1, arg2;
bb$> return null;
bb$> end;
bb$> $$ language plpgsql;
CREATE FUNCTION
bb=> create trigger aaa after insert on parted_trig for each row execute procedure trigger_notice('text', 1);
CREATE TRIGGER
bb=> \d parted_trig
Tabelle �public.parted_trig�
Spalte | Typ | Sortierfolge | NULL erlaubt? | Vorgabewert
--------+---------+--------------+---------------+-------------
a | integer | | |
Partitionsschl�ssel: LIST (a)
Trigger:
aaa AFTER INSERT ON parted_trig FOR EACH ROW EXECUTE PROCEDURE trigger_notice('text', '1')
Anzahl Partitionen: 1 (Mit \d+ alle anzeigen.)

bb=> \d parted_trig1
Tabelle �public.parted_trig1�
Spalte | Typ | Sortierfolge | NULL erlaubt? | Vorgabewert
--------+---------+--------------+---------------+-------------
a | integer | | |
Partition von: parted_trig FOR VALUES IN (1)
Trigger:
aaa AFTER INSERT ON parted_trig1 FOR EACH ROW EXECUTE PROCEDURE trigger_notice()

Fixed:

bb=> \d parted_trig1
Tabelle �public.parted_trig1�
Spalte | Typ | Sortierfolge | NULL erlaubt? | Vorgabewert
--------+---------+--------------+---------------+-------------
a | integer | | |
Partition von: parted_trig FOR VALUES IN (1)
Trigger:
aaa AFTER INSERT ON parted_trig1 FOR EACH ROW EXECUTE PROCEDURE trigger_notice('text', '1')

Patch is against 11.4, but applies to master with minor offset.

All regression test pass.

Attachments:

v1.patchtext/x-diff; charset=us-asciiDownload
commit eb33c2cc0047bee6fb6ae20fd11aec438c3463da
Author: Patrick McHardy <kaber@trash.net>
Date:   Tue Jul 9 14:41:21 2019 +0200

    Fix trigger argument propagation to child partitions
    
    When creating a trigger on a partitioned table, the trigger is
    propagated to child partitions. However arguments specified for the
    trigger function are not propagated, leading to the trigger function
    being called without any arguments.
    
    Similarly, when attaching a new partition to a partitioned table, the
    trigger is copied without arguments.
    
    Fix this by:
    
    - Passing the original argument list to CreateTrigger when creating
      triggers for existing partitions.
    
    - Reconstructing the original argument list from the heap and passing it
      to CreateTrigger when creating triggers for newly attached partitions.
    
    Additionally add a simple regression test for both cases.
    
    Introduced in 86f575948c7.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5c278463c6..655d39ef71 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15145,6 +15145,7 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
 		Datum		value;
 		bool		isnull;
 		List	   *cols = NIL;
+		List	   *args = NIL;
 		MemoryContext oldcxt;
 
 		/*
@@ -15209,11 +15210,28 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
 			}
 		}
 
+		/* Reconstruct trigger arguments list. */
+		if (trigForm->tgnargs > 0)
+		{
+			char 	   *p;
+			int			i;
+
+			value = heap_getattr(tuple, Anum_pg_trigger_tgargs,
+								 RelationGetDescr(pg_trigger), &isnull);
+			p = (char *) VARDATA_ANY(DatumGetByteaPP(value));
+
+			for (i = 0; i < trigForm->tgnargs; i++)
+			{
+				args = lappend(args, makeString(pstrdup(p)));
+				p += strlen(p) + 1;
+			}
+		}
+
 		trigStmt = makeNode(CreateTrigStmt);
 		trigStmt->trigname = NameStr(trigForm->tgname);
 		trigStmt->relation = NULL;
 		trigStmt->funcname = NULL;	/* passed separately */
-		trigStmt->args = NULL;	/* passed separately */
+		trigStmt->args = args;
 		trigStmt->row = true;
 		trigStmt->timing = trigForm->tgtype & TRIGGER_TYPE_TIMING_MASK;
 		trigStmt->events = trigForm->tgtype & TRIGGER_TYPE_EVENT_MASK;
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index f6c7a3fefc..a716827c77 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1153,7 +1153,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 			 */
 			childStmt = (CreateTrigStmt *) copyObject(stmt);
 			childStmt->funcname = NIL;
-			childStmt->args = NIL;
 			childStmt->whenClause = NULL;
 
 			/* If there is a WHEN clause, create a modified copy of it */
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 31dbc9bcfc..a7661807c0 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2092,6 +2092,26 @@ NOTICE:  trigger zzz on parted_trig_1_1 AFTER INSERT for ROW
 NOTICE:  trigger bbb on parted_trig_2 AFTER INSERT for ROW
 NOTICE:  trigger zzz on parted_trig_2 AFTER INSERT for ROW
 drop table parted_trig;
+-- Verify propagation of trigger arguments to partitions
+create table parted_trig (a int) partition by list (a);
+create table parted_trig1 partition of parted_trig for values in (1);
+create or replace function trigger_notice() returns trigger as $$
+  declare
+    arg1 text = TG_ARGV[0];
+    arg2 integer = TG_ARGV[1];
+  begin
+    raise notice 'trigger % on % % % for % args % %', TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, arg1, arg2;
+    return null;
+  end;
+  $$ language plpgsql;
+create trigger aaa after insert on parted_trig for each row execute procedure trigger_notice('text', 1);
+insert into parted_trig values (1);
+NOTICE:  trigger aaa on parted_trig1 AFTER INSERT for ROW args text 1
+-- Verify propagation of trigger arguments to partitions attached after creating trigger
+create table parted_trig2 partition of parted_trig for values in (2);
+insert into parted_trig values (2);
+NOTICE:  trigger aaa on parted_trig2 AFTER INSERT for ROW args text 1
+drop table parted_trig;
 -- test irregular partitions (i.e., different column definitions),
 -- including that the WHEN clause works
 create function bark(text) returns bool language plpgsql immutable
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 60b3e22c5c..909d403c62 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1465,6 +1465,26 @@ create trigger qqq after insert on parted_trig_1_1 for each row execute procedur
 insert into parted_trig values (50), (1500);
 drop table parted_trig;
 
+-- Verify propagation of trigger arguments to partitions
+create table parted_trig (a int) partition by list (a);
+create table parted_trig1 partition of parted_trig for values in (1);
+create or replace function trigger_notice() returns trigger as $$
+  declare
+    arg1 text = TG_ARGV[0];
+    arg2 integer = TG_ARGV[1];
+  begin
+    raise notice 'trigger % on % % % for % args % %', TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, arg1, arg2;
+    return null;
+  end;
+  $$ language plpgsql;
+create trigger aaa after insert on parted_trig for each row execute procedure trigger_notice('text', 1);
+insert into parted_trig values (1);
+
+-- Verify propagation of trigger arguments to partitions attached after creating trigger
+create table parted_trig2 partition of parted_trig for values in (2);
+insert into parted_trig values (2);
+drop table parted_trig;
+
 -- test irregular partitions (i.e., different column definitions),
 -- including that the WHEN clause works
 create function bark(text) returns bool language plpgsql immutable
#2Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Patrick McHardy (#1)
Re: [PATCH] Fix trigger argument propagation to child partitions

On Tue, Jul 09, 2019 at 03:00:27PM +0200, Patrick McHardy wrote:

The following patch fixes propagation of arguments to the trigger
function to child partitions both when initially creating the trigger
and when adding new partitions to a partitioned table.

Thanks for the report and bugfix. It seeem the parameters in row triggers
on partitioned tables never worked :-( For a moment I was wondering why it
shows on 11 and not 10 (based on the assumption you'd send a patch against
10 if it was affected), but 10 actually did not support row triggers on
partitioned tables.

The fix seems OK to me, although I see we're parsing tgargs in ruleutils.c
and that version (around line ~1050) uses fastgetattr instead of
heap_getattr, and checks the isnull parameter after the call. I guess we
should do the same thing here.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tomas Vondra (#2)
1 attachment(s)
Re: [PATCH] Fix trigger argument propagation to child partitions

On 2019-Jul-09, Tomas Vondra wrote:

On Tue, Jul 09, 2019 at 03:00:27PM +0200, Patrick McHardy wrote:

The following patch fixes propagation of arguments to the trigger
function to child partitions both when initially creating the trigger
and when adding new partitions to a partitioned table.

Thanks for the report and bugfix. It seeem the parameters in row triggers
on partitioned tables never worked :-( For a moment I was wondering why it
shows on 11 and not 10 (based on the assumption you'd send a patch against
10 if it was affected), but 10 actually did not support row triggers on
partitioned tables.

Right ...

The fix seems OK to me, although I see we're parsing tgargs in ruleutils.c
and that version (around line ~1050) uses fastgetattr instead of
heap_getattr, and checks the isnull parameter after the call. I guess we
should do the same thing here.

Yeah, absolutely. The attached v2 is basically Patrick's patch with
very minor style changes. I'll get this pushed as soon as the tests
finish running.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v2-0001-first.patchtext/x-diff; charset=us-asciiDownload
From e7d3738739ece9fa7a48192c7ebc481b27a4a682 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 9 Jul 2019 16:46:16 -0400
Subject: [PATCH v2] first

---
 src/backend/commands/tablecmds.c       | 23 ++++++++++++++++++++++-
 src/backend/commands/trigger.c         |  1 -
 src/test/regress/expected/triggers.out | 24 ++++++++++++++++++++++++
 src/test/regress/sql/triggers.sql      | 23 +++++++++++++++++++++++
 4 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c62922a112..5fbe7242c4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15202,6 +15202,7 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
 		Datum		value;
 		bool		isnull;
 		List	   *cols = NIL;
+		List	   *trigargs = NIL;
 		MemoryContext oldcxt;
 
 		/*
@@ -15266,11 +15267,31 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
 			}
 		}
 
+		/* Reconstruct trigger arguments list. */
+		if (trigForm->tgnargs > 0)
+		{
+			char	   *p;
+
+			value = heap_getattr(tuple, Anum_pg_trigger_tgargs,
+								 RelationGetDescr(pg_trigger), &isnull);
+			if (isnull)
+				elog(ERROR, "tgargs is null for trigger \"%s\" in partition \"%s\"",
+					 NameStr(trigForm->tgname), RelationGetRelationName(partition));
+
+			p = (char *) VARDATA_ANY(DatumGetByteaPP(value));
+
+			for (int i = 0; i < trigForm->tgnargs; i++)
+			{
+				trigargs = lappend(trigargs, makeString(pstrdup(p)));
+				p += strlen(p) + 1;
+			}
+		}
+
 		trigStmt = makeNode(CreateTrigStmt);
 		trigStmt->trigname = NameStr(trigForm->tgname);
 		trigStmt->relation = NULL;
 		trigStmt->funcname = NULL;	/* passed separately */
-		trigStmt->args = NULL;	/* passed separately */
+		trigStmt->args = trigargs;
 		trigStmt->row = true;
 		trigStmt->timing = trigForm->tgtype & TRIGGER_TYPE_TIMING_MASK;
 		trigStmt->events = trigForm->tgtype & TRIGGER_TYPE_EVENT_MASK;
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index f6c7a3fefc..a716827c77 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1153,7 +1153,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 			 */
 			childStmt = (CreateTrigStmt *) copyObject(stmt);
 			childStmt->funcname = NIL;
-			childStmt->args = NIL;
 			childStmt->whenClause = NULL;
 
 			/* If there is a WHEN clause, create a modified copy of it */
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 31dbc9bcfc..5be0009f00 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2092,6 +2092,30 @@ NOTICE:  trigger zzz on parted_trig_1_1 AFTER INSERT for ROW
 NOTICE:  trigger bbb on parted_trig_2 AFTER INSERT for ROW
 NOTICE:  trigger zzz on parted_trig_2 AFTER INSERT for ROW
 drop table parted_trig;
+-- Verify propagation of trigger arguments to partitions
+create table parted_trig (a int) partition by list (a);
+create table parted_trig1 partition of parted_trig for values in (1);
+create or replace function trigger_notice() returns trigger as $$
+  declare
+    arg1 text = TG_ARGV[0];
+    arg2 integer = TG_ARGV[1];
+  begin
+    raise notice 'trigger % on % % % for % args % %',
+		TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, arg1, arg2;
+    return null;
+  end;
+  $$ language plpgsql;
+create trigger aaa after insert on parted_trig
+   for each row execute procedure trigger_notice('quirky', 1);
+-- Verify propagation of trigger arguments to partitions attached after creating trigger
+create table parted_trig2 partition of parted_trig for values in (2);
+create table parted_trig3 (like parted_trig);
+alter table parted_trig attach partition parted_trig3 for values in (3);
+insert into parted_trig values (1), (2), (3);
+NOTICE:  trigger aaa on parted_trig1 AFTER INSERT for ROW args quirky 1
+NOTICE:  trigger aaa on parted_trig2 AFTER INSERT for ROW args quirky 1
+NOTICE:  trigger aaa on parted_trig3 AFTER INSERT for ROW args quirky 1
+drop table parted_trig;
 -- test irregular partitions (i.e., different column definitions),
 -- including that the WHEN clause works
 create function bark(text) returns bool language plpgsql immutable
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 60b3e22c5c..6376046c5d 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1465,6 +1465,29 @@ create trigger qqq after insert on parted_trig_1_1 for each row execute procedur
 insert into parted_trig values (50), (1500);
 drop table parted_trig;
 
+-- Verify propagation of trigger arguments to partitions
+create table parted_trig (a int) partition by list (a);
+create table parted_trig1 partition of parted_trig for values in (1);
+create or replace function trigger_notice() returns trigger as $$
+  declare
+    arg1 text = TG_ARGV[0];
+    arg2 integer = TG_ARGV[1];
+  begin
+    raise notice 'trigger % on % % % for % args % %',
+		TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, arg1, arg2;
+    return null;
+  end;
+  $$ language plpgsql;
+create trigger aaa after insert on parted_trig
+   for each row execute procedure trigger_notice('quirky', 1);
+
+-- Verify propagation of trigger arguments to partitions attached after creating trigger
+create table parted_trig2 partition of parted_trig for values in (2);
+create table parted_trig3 (like parted_trig);
+alter table parted_trig attach partition parted_trig3 for values in (3);
+insert into parted_trig values (1), (2), (3);
+drop table parted_trig;
+
 -- test irregular partitions (i.e., different column definitions),
 -- including that the WHEN clause works
 create function bark(text) returns bool language plpgsql immutable
-- 
2.17.1