CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Justin Pryzby
CREATE TABLE t(i int) PARTITION BY RANGE(i);
CREATE TABLE t1 PARTITION OF t FOR VALUES FROM (1) TO (10);
CREATE OR REPLACE FUNCTION tgf() RETURNS trigger LANGUAGE plpgsql AS $$ begin raise exception 'except'; end $$;
CREATE TRIGGER tg AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION tgf();
ALTER TABLE t1 DISABLE TRIGGER tg;
INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
ALTER TABLE t DISABLE TRIGGER tg;
CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);

postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE tgrelid::regclass::text IN ('t1','t2');
 tgrelid | tgenabled
---------+-----------
 t1      | D
 t2      | O
(2 rows)

I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
(since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
the fix should be.


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Justin Pryzby
I'm hoping that Alvaro will comment on this.

On Wed, Sep 30, 2020 at 05:34:50PM -0500, Justin Pryzby wrote:

> CREATE TABLE t(i int) PARTITION BY RANGE(i);
> CREATE TABLE t1 PARTITION OF t FOR VALUES FROM (1) TO (10);
> CREATE OR REPLACE FUNCTION tgf() RETURNS trigger LANGUAGE plpgsql AS $$ begin raise exception 'except'; end $$;
> CREATE TRIGGER tg AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION tgf();
> ALTER TABLE t1 DISABLE TRIGGER tg;
> INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
> ALTER TABLE t DISABLE TRIGGER tg;
> CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
>
> postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE tgrelid::regclass::text IN ('t1','t2');
>  tgrelid | tgenabled
> ---------+-----------
>  t1      | D
>  t2      | O
> (2 rows)
>
> I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
> (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> the fix should be.


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
In reply to this post by Justin Pryzby
On 2020-Sep-30, Justin Pryzby wrote:

> postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE tgrelid::regclass::text IN ('t1','t2');
>  tgrelid | tgenabled
> ---------+-----------
>  t1      | D
>  t2      | O
> (2 rows)
>
> I consider this a bug,

Yeah.

> but CreateTrigStmt doesn't have any "enabled" member
> (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> the fix should be.

I suggest we add a new function, as in the attached.  It's important to
keep the ABI of CreateTrigger unchanged, for the sake of
backpatchability, but ISTM it's equally important to keep its API
unchanged, because outside callers such as ProcessUtility_slow does not
have to care about the new trigger's enabled state.

disable-triggers.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
Same, with a little test.

I also just noticed that ALTER TABLE ONLY recurses to children, which it
should not.

0001-When-cloning-triggers-preserve-enabling-state.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
On 2020-Oct-16, Alvaro Herrera wrote:

> I also just noticed that ALTER TABLE ONLY recurses to children, which it
> should not.

Apparently I wrote (bogus) bespoke code to handle recursion in
EnableDisableTrigger instead of using ATSimpleRecursion.  This patch
seems to fix this problem.


recurse-disable-trigger.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
In reply to this post by Justin Pryzby
On 2020-Sep-30, Justin Pryzby wrote:

> CREATE TABLE t(i int) PARTITION BY RANGE(i);
> CREATE TABLE t1 PARTITION OF t FOR VALUES FROM (1) TO (10);
> CREATE OR REPLACE FUNCTION tgf() RETURNS trigger LANGUAGE plpgsql AS $$ begin raise exception 'except'; end $$;
> CREATE TRIGGER tg AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION tgf();
> ALTER TABLE t1 DISABLE TRIGGER tg;
> INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
> ALTER TABLE t DISABLE TRIGGER tg;
> CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
>
> postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE tgrelid::regclass::text IN ('t1','t2');
>  tgrelid | tgenabled
> ---------+-----------
>  t1      | D
>  t2      | O
> (2 rows)
>
> I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
> (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> the fix should be.

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.

This is a judgement call, and mine says to backpatch, but I've been
wrong on that.


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Justin Pryzby
On Tue, Oct 20, 2020 at 04:04:20PM -0300, Alvaro Herrera wrote:

> On 2020-Sep-30, Justin Pryzby wrote:
>
> > CREATE TABLE t(i int) PARTITION BY RANGE(i);
> > CREATE TABLE t1 PARTITION OF t FOR VALUES FROM (1) TO (10);
> > CREATE OR REPLACE FUNCTION tgf() RETURNS trigger LANGUAGE plpgsql AS $$ begin raise exception 'except'; end $$;
> > CREATE TRIGGER tg AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION tgf();
> > ALTER TABLE t1 DISABLE TRIGGER tg;
> > INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
> > ALTER TABLE t DISABLE TRIGGER tg;
> > CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
> >
> > postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE tgrelid::regclass::text IN ('t1','t2');
> >  tgrelid | tgenabled
> > ---------+-----------
> >  t1      | D
> >  t2      | O
> > (2 rows)
> >
> > I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
> > (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> > the fix should be.
>
> 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.

+0.8 to backpatch.  To v13 if not further.

We don't normally disable triggers, otherwise I would say +1.

For context, I ran into this issue while migrating a customer to a new server
using pg_restore and a custom backup script which loops around pg_dump, and
handles partitioned tables differently depending if they're recent or historic.

Our backup job works well, but is technically a bit of a hack.  It doesn't do
the right thing (causes sql errors and pg_restore warnings) for inherited
indexes and, apparently, triggers.  Disabling the trigger was my 4th attempt to
handle an error restoring a specific table (mismatched column type between
parent dump and child dumped several days earlier).  I eventually (5th
or 6th attempt) dropped the parent trigger, created the child tables using
--section=pre-data, ALTERed a column to match, and then ran post-data and
attached it.

--
Justin


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Tom Lane-2
In reply to this post by Álvaro Herrera
Alvaro Herrera <[hidden email]> 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.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
In reply to this post by Álvaro Herrera
On 2020-Oct-16, Alvaro Herrera wrote:

> On 2020-Oct-16, Alvaro Herrera wrote:
>
> > I also just noticed that ALTER TABLE ONLY recurses to children, which it
> > should not.
>
> Apparently I wrote (bogus) bespoke code to handle recursion in
> EnableDisableTrigger instead of using ATSimpleRecursion.  This patch
> seems to fix this problem.

... but it affects legacy inheritance, which would be undesirable
because it has never recursed for that case.  So it needs to have a
relkind check here and only recurse if it's a new-style partitioned
table:

> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index 511f015a86..c8d6f78da2 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -4321,6 +4321,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
>   case AT_DisableTrigAll:
>   case AT_DisableTrigUser:
>   ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
> + ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
>   pass = AT_PASS_MISC;
>   break;
>   case AT_EnableRule: /* ENABLE/DISABLE RULE variants */

I'll add tests for both cases and push to all branches 11+.


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Justin Pryzby
In reply to this post by Tom Lane-2
On Tue, Oct 20, 2020 at 03:56:30PM -0400, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> 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.

The old catalog state is still possible - it's what you'd get if you did
CREATE TABLE child PARTITION OF..; ALTER TABLE child DISABLE TRIGGER.

I don't think pg_dump needs to do anyting special, since the restore will now
do what's wanted without added commands.  Note that pg_dump switched from
"PARTITION OF" to "ATTACH PARTITION" at commit 33a53130a.  This would handles
both on the server side.

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 ?

--
Justin


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
In reply to this post by Álvaro Herrera
On 2020-Oct-20, Alvaro Herrera wrote:

> > diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> > index 511f015a86..c8d6f78da2 100644
> > --- a/src/backend/commands/tablecmds.c
> > +++ b/src/backend/commands/tablecmds.c
> > @@ -4321,6 +4321,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
> >   case AT_DisableTrigAll:
> >   case AT_DisableTrigUser:
> >   ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
> > + ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
> >   pass = AT_PASS_MISC;
> >   break;
> >   case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
>
> I'll add tests for both cases and push to all branches 11+.

Pushed this part.


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
In reply to this post by Justin Pryzby
On 2020-Oct-20, Justin Pryzby wrote:

> On Tue, Oct 20, 2020 at 03:56:30PM -0400, Tom Lane wrote:
> > Alvaro Herrera <[hidden email]> 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
all child triggers that doesn't have the same tgenabled as the parent,
and append ALTER .. DISABLE commands for each one to the parent table
trigger creation command.  So pg_dump.c's getTriggers would have to
obtain the list of altered child triggers, and then dumpTrigger would
have to append the ALTER TABLE ONLY <partition> .. ENABLE/DISABLE
command for that particular trigger.



Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Justin Pryzby
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 <[hidden email]> 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

v1-0001-pg_dump-output-DISABLE-ENABLE-for-child-triggers.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Álvaro Herrera
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.

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.

Change 1 also means that the test with the "this shouldn't ever get
emitted" comment remains unchanged.

I'm not sure how to tackle change 2.  You need to search pg_depend for
entries with classid=pg_trigger and refclass=pg_trigger ... (commit
1fa846f1c9af might give some clue)


Reply | Threaded
Open this post in threaded view
|

Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers

Justin Pryzby
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

v2-0001-pg_dump-output-DISABLE-ENABLE-for-child-triggers.patch (7K) Download Attachment