Date: Tue, 27 Oct 2020 12:43:18 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org,
	Amit Langote <amitlangote09@gmail.com>
Subject: Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for
 inherited row triggers
Message-ID: <20201027174318.GM9241@telsasoft.com>
References: <20201021115453.GL9241@telsasoft.com>
 <20201021170237.GA17401@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MET8MpPxp2u2c48q"
Content-Disposition: inline
In-Reply-To: <20201021170237.GA17401@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--MET8MpPxp2u2c48q
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Oct 21, 2020 at 02:02:37PM -0300, Alvaro Herrera wrote:
> On 2020-Oct-21, Justin Pryzby wrote:
> 
> > I came up with this, which probably needs more than a little finesse.
> 
> Hmm, there are two important changes needed on this: 1) it must not emit
> CREATE lines for the child triggers; only the ALTER TABLE ONLY
> <partition> lines to set disable state on the partition are needed.  2)
> tgparentid does not exist prior to pg13, so you need some additional
> trick to cover that case.

Thanks for looking.

> Also, I think the multipartition case is broken: if grandparent has
> trigger enabled, parent has trigger disabled and child trigger set to
> always, is that dumped correctly?  I think the right way to do this is
> change only the partitions that differ from the topmost partitioned
> table -- not their immediate parents; and use ONLY to ensure they don't
> affect downstream children.

I think either way could be ok - if you assume that the trigger was disabled
with ONLY, then it'd make sense to restore it with ONLY, but I think it's at
least as common to ALTER TABLE [*].  It might look weird to the user if they
used ALTER TABLE ONLY and the pg_dump output uses ALTER TABLE for that table,
and then again for all its children (or vice versa).  But it's fine as long as
the state is correctly restored.

There are serveral issues:
 - fail to preserve childs' tgenabled in CREATE TABLE PARTITION OF;
 - fail to preserve childs' tgenabled in pg_dump;
 - fail to preserve childs' comments in pg_dump;

I'm going step away from this patch at least for awhile, so I'm attaching my
latest in case it's useful.

-- 
Justin

--MET8MpPxp2u2c48q
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-pg_dump-output-DISABLE-ENABLE-for-child-triggers.patch"

From 15cb95df4e3771f0bb3fe2f1fd8dfb94fe3f7a50 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 20 Oct 2020 23:19:07 -0500
Subject: [PATCH v2] pg_dump: output DISABLE/ENABLE for child triggers ..

..if their state does not match their parent
---
 src/bin/pg_dump/pg_dump.c        | 62 +++++++++++++++++++++++++++-----
 src/bin/pg_dump/pg_dump.h        |  1 +
 src/bin/pg_dump/t/002_pg_dump.pl | 26 +++++++++++---
 3 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ff45e3fb8c..40844fa1e7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7811,6 +7811,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
 				i_tgconstrrelid,
 				i_tgconstrrelname,
 				i_tgenabled,
+				i_tgisinternal,
 				i_tgdeferrable,
 				i_tginitdeferred,
 				i_tgdef;
@@ -7829,21 +7830,46 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
 					tbinfo->dobj.name);
 
 		resetPQExpBuffer(query);
-		if (fout->remoteVersion >= 90000)
+		if (fout->remoteVersion >= 13000)
 		{
 			/*
 			 * NB: think not to use pretty=true in pg_get_triggerdef.  It
 			 * could result in non-forward-compatible dumps of WHEN clauses
 			 * due to under-parenthesization.
+			 * Use tgparentid, which is available since v13.
 			 */
 			appendPQExpBuffer(query,
-							  "SELECT tgname, "
-							  "tgfoid::pg_catalog.regproc AS tgfname, "
-							  "pg_catalog.pg_get_triggerdef(oid, false) AS tgdef, "
-							  "tgenabled, tableoid, oid "
+							  "SELECT t.tgname, "
+							  "t.tgfoid::pg_catalog.regproc AS tgfname, "
+							  "pg_catalog.pg_get_triggerdef(t.oid, false) AS tgdef, "
+							  "t.tgenabled, t.tableoid, t.oid, t.tgisinternal "
 							  "FROM pg_catalog.pg_trigger t "
-							  "WHERE tgrelid = '%u'::pg_catalog.oid "
-							  "AND NOT tgisinternal",
+							  "LEFT JOIN pg_catalog.pg_trigger u ON u.oid = t.tgparentid "
+							  "WHERE t.tgrelid = '%u'::pg_catalog.oid "
+							  "AND (NOT t.tgisinternal OR t.tgenabled != u.tgenabled)",
+							  tbinfo->dobj.catId.oid);
+		}
+		else if (fout->remoteVersion >= 90000)
+		{
+			/*
+			 * NB: think not to use pretty=true in pg_get_triggerdef.  It
+			 * could result in non-forward-compatible dumps of WHEN clauses
+			 * due to under-parenthesization.
+			 * The pg_depend joins handle v11-v12, which allow inherited row
+			 * triggers, but did not have tgparentid.  And do nothing between v9-v10.
+			 */
+			appendPQExpBuffer(query,
+							  "SELECT t.tgname, "
+							  "t.tgfoid::pg_catalog.regproc AS tgfname, "
+							  "pg_catalog.pg_get_triggerdef(t.oid, false) AS tgdef, "
+							  "t.tgenabled, t.tableoid, t.oid, t.tgisinternal "
+							  "FROM pg_catalog.pg_trigger t "
+							  "LEFT JOIN pg_catalog.pg_depend AS d ON "
+							  " d.classid = 'pg_trigger'::pg_catalog.regclass AND "
+							  " d.refclassid = 'pg_trigger'::pg_catalog.regclass AND d.objid = t.oid "
+							  "LEFT JOIN pg_catalog.pg_trigger AS u ON u.oid = refobjid "
+							  "WHERE t.tgrelid = '%u'::pg_catalog.oid "
+							  "AND (NOT t.tgisinternal OR t.tgenabled != u.tgenabled)",
 							  tbinfo->dobj.catId.oid);
 		}
 		else if (fout->remoteVersion >= 80300)
@@ -7903,6 +7929,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
 		i_tgconstrrelid = PQfnumber(res, "tgconstrrelid");
 		i_tgconstrrelname = PQfnumber(res, "tgconstrrelname");
 		i_tgenabled = PQfnumber(res, "tgenabled");
+		i_tgisinternal = PQfnumber(res, "tgisinternal");
 		i_tgdeferrable = PQfnumber(res, "tgdeferrable");
 		i_tginitdeferred = PQfnumber(res, "tginitdeferred");
 		i_tgdef = PQfnumber(res, "tgdef");
@@ -7922,6 +7949,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
 			tginfo[j].dobj.namespace = tbinfo->dobj.namespace;
 			tginfo[j].tgtable = tbinfo;
 			tginfo[j].tgenabled = *(PQgetvalue(res, j, i_tgenabled));
+			tginfo[j].tgisinternal = *(PQgetvalue(res, j, i_tgisinternal)) == 't';
 			if (i_tgdef >= 0)
 			{
 				tginfo[j].tgdef = pg_strdup(PQgetvalue(res, j, i_tgdef));
@@ -17406,7 +17434,25 @@ dumpTrigger(Archive *fout, TriggerInfo *tginfo)
 								"pg_catalog.pg_trigger", "TRIGGER",
 								trigidentity->data);
 
-	if (tginfo->tgenabled != 't' && tginfo->tgenabled != 'O')
+	if (tginfo->tgisinternal)
+	{
+		/* We're dumping a child trigger with opposite tgenabled as its parent, so needs to be ALTERed */
+		appendPQExpBuffer(query, "\nALTER %sTABLE %s ",
+						  tbinfo->relkind == RELKIND_FOREIGN_TABLE ? "FOREIGN " : "",
+						  fmtQualifiedDumpable(tbinfo));
+		switch (tginfo->tgenabled)
+		{
+			case 'D':
+				appendPQExpBufferStr(query, "DISABLE");
+				break;
+			default:
+				appendPQExpBufferStr(query, "ENABLE");
+				break;
+		}
+		appendPQExpBuffer(query, " TRIGGER %s;\n",
+						  fmtId(tginfo->dobj.name));
+	}
+	else if (tginfo->tgenabled != 't' && tginfo->tgenabled != 'O')
 	{
 		appendPQExpBuffer(query, "\nALTER %sTABLE %s ",
 						  tbinfo->relkind == RELKIND_FOREIGN_TABLE ? "FOREIGN " : "",
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index e0b42e8391..883f83acc6 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -413,6 +413,7 @@ typedef struct _triggerInfo
 	Oid			tgconstrrelid;
 	char	   *tgconstrrelname;
 	char		tgenabled;
+	bool		tgisinternal;
 	bool		tgdeferrable;
 	bool		tginitdeferred;
 	char	   *tgdef;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ec63662060..3f8645fdaa 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2404,10 +2404,28 @@ my %tests = (
 		},
 	},
 
-	# this shouldn't ever get emitted
+	'Partition measurement_y2006m3 creation - disabled child trigger' => {
+		create_order => 93,
+		create_sql =>
+		  'CREATE TABLE dump_test_second_schema.measurement_y2006m3
+						PARTITION OF dump_test.measurement
+						FOR VALUES FROM (\'2006-03-01\') TO (\'2006-04-01\');
+						ALTER TABLE dump_test_second_schema.measurement_y2006m3 DISABLE TRIGGER test_trigger',
+		regexp => qr/^
+			\QALTER TABLE dump_test_second_schema.measurement_y2006m3 DISABLE TRIGGER test_trigger;\E
+			/xm,
+		like => {
+			%full_runs,
+			section_post_data => 1,
+			role             => 1,
+			binary_upgrade   => 1,
+		},
+	},
+
+	# this shouldn't get emitted except for triggers with altered disabled/enabled
 	'Creation of row-level trigger in partition' => {
 		regexp => qr/^
-			\QCREATE TRIGGER test_trigger AFTER INSERT ON dump_test_second_schema.measurement\E
+			\QCREATE TRIGGER test_trigger AFTER INSERT ON dump_test_second_schema.measurement_y2006m2\E
 			/xm,
 		like => {},
 	},
@@ -3002,9 +3020,9 @@ my %tests = (
 	},
 
 	'GRANT SELECT ON TABLE measurement_y2006m2' => {
-		create_order => 92,
+		create_order => 94,
 		create_sql   => 'GRANT SELECT ON
-						   TABLE dump_test_second_schema.measurement_y2006m2
+						   TABLE dump_test_second_schema.measurement_y2006m2, dump_test_second_schema.measurement_y2006m3
 						   TO regress_dump_test_role;',
 		regexp =>
 		  qr/^\QGRANT SELECT ON TABLE dump_test_second_schema.measurement_y2006m2 TO regress_dump_test_role;\E/m,
-- 
2.17.0


--MET8MpPxp2u2c48q--