On Tue, Oct 20, 2020 at 09:54:53PM -0300, Alvaro Herrera wrote:
> On 2020-Oct-20, Justin Pryzby wrote:
> > On Tue, Oct 20, 2020 at 03:56:30PM -0400, Tom Lane wrote:
> > > Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> > > > Hmm, next question: should we backpatch a fix for this? (This applies
> > > > all the way back to 11.) If we do, then we would change behavior of
> > > > partition creation. It's hard to see that the current behavior is
> > > > desirable ... and I think anybody who would have come across this, would
> > > > wish it behaved the other way. But still -- it would definitely be a
> > > > behavior change.
> > >
> > > The behavior change seems like it'd be an improvement in a vacuum,
> > > but I wonder how it would interact with catalog contents left behind
> > > by the old misbehavior. Also, would we expect pg_dump to try to do
> > > anything to clean up the mess? If so, allowing a back branch to have
> > > had more than one behavior would complicate that greatly.
> >
> > I don't think there's a problem with catalog content ?
> > I think it's fine if there's an enabled child trigger inheriting from a
> > disabled parent? This changes the initial tgenabled for new partitions.
>
> I don't think we'd need to do anything special here ... particularly
> considering the discovery that pg_dump does not preserve the disable
> status of trigger on partitions:
>
> > However...it looks like pg_dump should ALTER the child trigger state if it
> > differ from its parent. Or maybe it needs to CREATE child triggers with the
> > proper state before attaching the child table ?
>
> I guess *something* needs to be done, but I'm not clear on what it is.
> Creating the trigger on partition beforehand does not work: an error is
> raised on attach that the trigger already exists.
>
> The only way I see to do this, is to have pg_dump extract tgenabled for
I came up with this, which probably needs more than a little finesse.
--
Justin
>From 465fba070986774e8bf4ec911986cc97dd211b20 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Tue, 20 Oct 2020 23:19:07 -0500
Subject: [PATCH v1] pg_dump: output DISABLE/ENABLE for child triggers ..
..if their state does not match their parent
---
src/bin/pg_dump/pg_dump.c | 36 +++++++++++++++++++++++++-------
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 26 +++++++++++++++++++----
3 files changed, 52 insertions(+), 11 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ff45e3fb8c..c4b7046cac 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7792,77 +7792,79 @@ getRules(Archive *fout, int *numRules)
* does get entered into the DumpableObject tables.
*/
void
getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
{
int i,
j;
PQExpBuffer query = createPQExpBuffer();
PGresult *res;
TriggerInfo *tginfo;
int i_tableoid,
i_oid,
i_tgname,
i_tgfname,
i_tgtype,
i_tgnargs,
i_tgargs,
i_tgisconstraint,
i_tgconstrname,
i_tgconstrrelid,
i_tgconstrrelname,
i_tgenabled,
+ i_tgisinternal,
i_tgdeferrable,
i_tginitdeferred,
i_tgdef;
int ntups;
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = &tblinfo[i];
if (!tbinfo->hastriggers ||
!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;
pg_log_info("reading triggers for table \"%s.%s\"",
tbinfo->dobj.namespace->dobj.name,
tbinfo->dobj.name);
resetPQExpBuffer(query);
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.
*/
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 >= 80300)
{
/*
* We ignore triggers that are tied to a foreign-key constraint
*/
appendPQExpBuffer(query,
"SELECT tgname, "
"tgfoid::pg_catalog.regproc AS tgfname, "
"tgtype, tgnargs, tgargs, tgenabled, "
"tgisconstraint, tgconstrname, tgdeferrable, "
"tgconstrrelid, tginitdeferred, tableoid, oid, "
"tgconstrrelid::pg_catalog.regclass AS tgconstrrelname "
"FROM pg_catalog.pg_trigger t "
"WHERE tgrelid = '%u'::pg_catalog.oid "
"AND tgconstraint = 0",
tbinfo->dobj.catId.oid);
}
else
{
/*
@@ -7884,63 +7886,65 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
" (SELECT 1 FROM pg_catalog.pg_depend d "
" JOIN pg_catalog.pg_constraint c ON (d.refclassid = c.tableoid AND d.refobjid = c.oid) "
" WHERE d.classid = t.tableoid AND d.objid = t.oid AND d.deptype = 'i' AND c.contype = 'f'))",
tbinfo->dobj.catId.oid);
}
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
ntups = PQntuples(res);
i_tableoid = PQfnumber(res, "tableoid");
i_oid = PQfnumber(res, "oid");
i_tgname = PQfnumber(res, "tgname");
i_tgfname = PQfnumber(res, "tgfname");
i_tgtype = PQfnumber(res, "tgtype");
i_tgnargs = PQfnumber(res, "tgnargs");
i_tgargs = PQfnumber(res, "tgargs");
i_tgisconstraint = PQfnumber(res, "tgisconstraint");
i_tgconstrname = PQfnumber(res, "tgconstrname");
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");
tginfo = (TriggerInfo *) pg_malloc(ntups * sizeof(TriggerInfo));
tbinfo->numTriggers = ntups;
tbinfo->triggers = tginfo;
for (j = 0; j < ntups; j++)
{
tginfo[j].dobj.objType = DO_TRIGGER;
tginfo[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, i_tableoid));
tginfo[j].dobj.catId.oid = atooid(PQgetvalue(res, j, i_oid));
AssignDumpId(&tginfo[j].dobj);
tginfo[j].dobj.name = pg_strdup(PQgetvalue(res, j, i_tgname));
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));
/* remaining fields are not valid if we have tgdef */
tginfo[j].tgfname = NULL;
tginfo[j].tgtype = 0;
tginfo[j].tgnargs = 0;
tginfo[j].tgargs = NULL;
tginfo[j].tgisconstraint = false;
tginfo[j].tgdeferrable = false;
tginfo[j].tginitdeferred = false;
tginfo[j].tgconstrname = NULL;
tginfo[j].tgconstrrelid = InvalidOid;
tginfo[j].tgconstrrelname = NULL;
}
else
{
tginfo[j].tgdef = NULL;
tginfo[j].tgfname = pg_strdup(PQgetvalue(res, j, i_tgfname));
tginfo[j].tgtype = atoi(PQgetvalue(res, j, i_tgtype));
@@ -17387,45 +17391,63 @@ dumpTrigger(Archive *fout, TriggerInfo *tginfo)
/* hm, not found before end of bytea value... */
pg_log_error("invalid argument string (%s) for trigger \"%s\" on table \"%s\"",
tginfo->tgargs,
tginfo->dobj.name,
tbinfo->dobj.name);
exit_nicely(1);
}
if (findx > 0)
appendPQExpBufferStr(query, ", ");
appendStringLiteralAH(query, p, fout);
p += tlen + 1;
}
free(tgargs);
appendPQExpBufferStr(query, ");\n");
}
/* Triggers can depend on extensions */
append_depends_on_extension(fout, query, &tginfo->dobj,
"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 " : "",
fmtQualifiedDumpable(tbinfo));
switch (tginfo->tgenabled)
{
case 'D':
case 'f':
appendPQExpBufferStr(query, "DISABLE");
break;
case 'A':
appendPQExpBufferStr(query, "ENABLE ALWAYS");
break;
case 'R':
appendPQExpBufferStr(query, "ENABLE REPLICA");
break;
default:
appendPQExpBufferStr(query, "ENABLE");
break;
}
appendPQExpBuffer(query, " TRIGGER %s;\n",
fmtId(tginfo->dobj.name));
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
@@ -394,44 +394,45 @@ typedef struct _ruleInfo
DumpableObject dobj;
TableInfo *ruletable; /* link to table the rule is for */
char ev_type;
bool is_instead;
char ev_enabled;
bool separate; /* true if must dump as separate item */
/* separate is always true for non-ON SELECT rules */
} RuleInfo;
typedef struct _triggerInfo
{
DumpableObject dobj;
TableInfo *tgtable; /* link to table the trigger is for */
char *tgfname;
int tgtype;
int tgnargs;
char *tgargs;
bool tgisconstraint;
char *tgconstrname;
Oid tgconstrrelid;
char *tgconstrrelname;
char tgenabled;
+ bool tgisinternal;
bool tgdeferrable;
bool tginitdeferred;
char *tgdef;
} TriggerInfo;
typedef struct _evttriggerInfo
{
DumpableObject dobj;
char *evtname;
char *evtevent;
char *evtowner;
char *evttags;
char *evtfname;
char evtenabled;
} EventTriggerInfo;
/*
* struct ConstraintInfo is used for all constraint types. However we
* use a different objType for foreign key constraints, to make it easier
* to sort them the way we want.
*
* Note: condeferrable and condeferred are currently only valid for
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
@@ -2385,48 +2385,66 @@ my %tests = (
binary_upgrade => 1,
},
},
'Creation of row-level trigger in partitioned table' => {
create_order => 92,
create_sql => 'CREATE TRIGGER test_trigger
AFTER INSERT ON dump_test.measurement
FOR EACH ROW EXECUTE PROCEDURE dump_test.trigger_func()',
regexp => qr/^
\QCREATE TRIGGER test_trigger AFTER INSERT ON dump_test.measurement \E
\QFOR EACH ROW \E
\QEXECUTE FUNCTION dump_test.trigger_func();\E
/xm,
like => {
%full_runs, %dump_test_schema_runs, section_post_data => 1,
},
unlike => {
exclude_dump_test_schema => 1,
},
},
- # 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 => {},
},
'CREATE TABLE test_fourth_table_zero_col' => {
create_order => 6,
create_sql => 'CREATE TABLE dump_test.test_fourth_table (
);',
regexp => qr/^
\QCREATE TABLE dump_test.test_fourth_table (\E
\n\);
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
},
'CREATE TABLE test_fifth_table' => {
create_order => 53,
create_sql => 'CREATE TABLE dump_test.test_fifth_table (
col1 integer,
col2 boolean,
@@ -2983,47 +3001,47 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_privs => 1,
},
},
'GRANT SELECT ON TABLE measurement' => {
create_order => 91,
create_sql => 'GRANT SELECT ON
TABLE dump_test.measurement
TO regress_dump_test_role;',
regexp =>
qr/^\QGRANT SELECT ON TABLE dump_test.measurement TO regress_dump_test_role;\E/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
exclude_dump_test_schema => 1,
no_privs => 1,
},
},
'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,
like => {
%full_runs,
role => 1,
section_pre_data => 1,
},
unlike => { no_privs => 1, },
},
'GRANT ALL ON LARGE OBJECT ...' => {
create_order => 60,
create_sql => 'DO $$
DECLARE myoid oid;
BEGIN
SELECT loid FROM pg_largeobject INTO myoid;
EXECUTE \'GRANT ALL ON LARGE OBJECT \' || myoid || \' TO regress_dump_test_role;\';
END;
$$;',
regexp => qr/^
\QGRANT ALL ON LARGE OBJECT \E[0-9]+\Q TO regress_dump_test_role;\E
--
2.17.0
