Обсуждение: enable/disable broken for statement triggers on partitioned tables

Hi,

Simon reported $subject off-list.

For triggers on partitioned tables, various enable/disable trigger
variants recurse to also process partitions' triggers by way of
ATSimpleRecursion() done in the "prep" phase.  While that way of
recursing is fine for row-level triggers which are cloned to
partitions, it isn't for statement-level triggers which are not
cloned, so you get an unexpected error as follows:

create table p (a int primary key) partition by list (a);
create table p1 partition of p for values in (1);
create function trigfun () returns trigger language plpgsql as $$
begin raise notice 'insert on p'; end; $$;
create trigger trig before insert on p for statement execute function trigfun();
alter table p disable trigger trig;
ERROR:  trigger "trig" for table "p1" does not exist

The problem is that ATPrepCmd() is too soon to perform the recursion
in this case as it's not known at that stage if the trigger being
enabled/disabled is row-level or statement level, so it's better to
perform it during ATExecCmd().  Actually, that is how it used to be
done before bbb927b4db9b changed things to use ATSimpleRecursion() to
fix a related problem, which was that the ONLY specification was
ignored by the earlier implementation.  The information of whether
ONLY is specified in a given command is only directly available in the
"prep" phase and must be remembered somehow if the recursion must be
handled in the "exec" phase.  The way that's typically done that I see
in tablecmds.c is to have ATPrepCmd() change the AlterTableCmd.subtype
to a recursive variant of a given sub-command.  For example,
AT_ValidateConstraint by AT_ValidateConstraintRecurse if ONLY is not
specified.

So, I think we should do something like the attached.  A lot of
boilerplate is needed given that the various enable/disable trigger
variants are represented as separate sub-commands (AlterTableCmd
subtypes), which can perhaps be avoided by inventing a
EnableDisableTrigStmt sub-command node that stores (only?) the recurse
flag.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 24.05.22 23:23, Zhihong Yu wrote:
> Hi,
> 
>      AT_EnableTrig,              /* ENABLE TRIGGER name */
> +   AT_EnableTrigRecurse,       /* internal to commands/tablecmds.c */
>      AT_EnableAlwaysTrig,        /* ENABLE ALWAYS TRIGGER name */
> +   AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */
> 
> Is it better to put the new enum's at the end of the AlterTableType?
> 
> This way the numeric values for existing ones don't change.

That's a concern if backpatching.  Otherwise, it's better to put them 
like shown in the patch.

On Fri, May 27, 2022 at 5:11 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> On 24.05.22 23:23, Zhihong Yu wrote:
> > Hi,
> >
> >      AT_EnableTrig,              /* ENABLE TRIGGER name */
> > +   AT_EnableTrigRecurse,       /* internal to commands/tablecmds.c */
> >      AT_EnableAlwaysTrig,        /* ENABLE ALWAYS TRIGGER name */
> > +   AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */
> >
> > Is it better to put the new enum's at the end of the AlterTableType?
> >
> > This way the numeric values for existing ones don't change.
>
> That's a concern if backpatching.  Otherwise, it's better to put them
> like shown in the patch.

Agreed.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Tue, May 24, 2022 at 3:11 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Simon reported $subject off-list.
>
> For triggers on partitioned tables, various enable/disable trigger
> variants recurse to also process partitions' triggers by way of
> ATSimpleRecursion() done in the "prep" phase.  While that way of
> recursing is fine for row-level triggers which are cloned to
> partitions, it isn't for statement-level triggers which are not
> cloned, so you get an unexpected error as follows:
>
> create table p (a int primary key) partition by list (a);
> create table p1 partition of p for values in (1);
> create function trigfun () returns trigger language plpgsql as $$
> begin raise notice 'insert on p'; end; $$;
> create trigger trig before insert on p for statement execute function trigfun();
> alter table p disable trigger trig;
> ERROR:  trigger "trig" for table "p1" does not exist
>
> The problem is that ATPrepCmd() is too soon to perform the recursion
> in this case as it's not known at that stage if the trigger being
> enabled/disabled is row-level or statement level, so it's better to
> perform it during ATExecCmd().  Actually, that is how it used to be
> done before bbb927b4db9b changed things to use ATSimpleRecursion() to
> fix a related problem, which was that the ONLY specification was
> ignored by the earlier implementation.  The information of whether
> ONLY is specified in a given command is only directly available in the
> "prep" phase and must be remembered somehow if the recursion must be
> handled in the "exec" phase.  The way that's typically done that I see
> in tablecmds.c is to have ATPrepCmd() change the AlterTableCmd.subtype
> to a recursive variant of a given sub-command.  For example,
> AT_ValidateConstraint by AT_ValidateConstraintRecurse if ONLY is not
> specified.
>
> So, I think we should do something like the attached.  A lot of
> boilerplate is needed given that the various enable/disable trigger
> variants are represented as separate sub-commands (AlterTableCmd
> subtypes), which can perhaps be avoided by inventing a
> EnableDisableTrigStmt sub-command node that stores (only?) the recurse
> flag.

Added to the next CF: https://commitfest.postgresql.org/38/3728/

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Hi!

I've looked through the code and everything looks good.
But there is one thing I doubt.
Patch changes result of test:
----

create function trig_nothing() returns trigger language plpgsql
   as $$ begin return null; end $$;
create table parent (a int) partition by list (a);
create table child1 partition of parent for values in (1);

create trigger tg after insert on parent
   for each row execute procedure trig_nothing();
select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;
alter table only parent enable always trigger tg; -- no recursion
select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;
alter table parent enable always trigger tg; -- recursion
select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;

drop table parent, child1;
drop function trig_nothing();

----
Results of vanilla + patch:
----
CREATE FUNCTION
CREATE TABLE
CREATE TABLE
CREATE TRIGGER
  tgrelid | tgname | tgenabled
---------+--------+-----------
  child1  | tg     | O
  parent  | tg     | O
(2 rows)

ALTER TABLE
  tgrelid | tgname | tgenabled
---------+--------+-----------
  child1  | tg     | O
  parent  | tg     | A
(2 rows)

ALTER TABLE
  tgrelid | tgname | tgenabled
---------+--------+-----------
  child1  | tg     | O
  parent  | tg     | A
(2 rows)

DROP TABLE
DROP FUNCTION

----
Results of vanilla:
----
CREATE FUNCTION
CREATE TABLE
CREATE TABLE
CREATE TRIGGER
  tgrelid | tgname | tgenabled
---------+--------+-----------
  child1  | tg     | O
  parent  | tg     | O
(2 rows)

ALTER TABLE
  tgrelid | tgname | tgenabled
---------+--------+-----------
  child1  | tg     | O
  parent  | tg     | A
(2 rows)

ALTER TABLE
  tgrelid | tgname | tgenabled
---------+--------+-----------
  child1  | tg     | A
  parent  | tg     | A
(2 rows)

DROP TABLE
DROP FUNCTION
----
The patch doesn't start recursion in case 'tgenabled' flag of parent 
table is not changes.
Probably vanilla result is more correct.

-- 
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com

Hi,

On Fri, Jul 8, 2022 at 3:44 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
> I've looked through the code and everything looks good.
> But there is one thing I doubt.
> Patch changes result of test:
> ----
>
> create function trig_nothing() returns trigger language plpgsql
>    as $$ begin return null; end $$;
> create table parent (a int) partition by list (a);
> create table child1 partition of parent for values in (1);
>
> create trigger tg after insert on parent
>    for each row execute procedure trig_nothing();
> select tgrelid::regclass, tgname, tgenabled from pg_trigger
>    where tgrelid in ('parent'::regclass, 'child1'::regclass)
>    order by tgrelid::regclass::text;
> alter table only parent enable always trigger tg; -- no recursion
> select tgrelid::regclass, tgname, tgenabled from pg_trigger
>    where tgrelid in ('parent'::regclass, 'child1'::regclass)
>    order by tgrelid::regclass::text;
> alter table parent enable always trigger tg; -- recursion
> select tgrelid::regclass, tgname, tgenabled from pg_trigger
>    where tgrelid in ('parent'::regclass, 'child1'::regclass)
>    order by tgrelid::regclass::text;
>
> drop table parent, child1;
> drop function trig_nothing();
>
> ----
> Results of vanilla + patch:
> ----
> CREATE FUNCTION
> CREATE TABLE
> CREATE TABLE
> CREATE TRIGGER
>   tgrelid | tgname | tgenabled
> ---------+--------+-----------
>   child1  | tg     | O
>   parent  | tg     | O
> (2 rows)
>
> ALTER TABLE
>   tgrelid | tgname | tgenabled
> ---------+--------+-----------
>   child1  | tg     | O
>   parent  | tg     | A
> (2 rows)
>
> ALTER TABLE
>   tgrelid | tgname | tgenabled
> ---------+--------+-----------
>   child1  | tg     | O
>   parent  | tg     | A
> (2 rows)
>
> DROP TABLE
> DROP FUNCTION
>
> ----
> Results of vanilla:
> ----
> CREATE FUNCTION
> CREATE TABLE
> CREATE TABLE
> CREATE TRIGGER
>   tgrelid | tgname | tgenabled
> ---------+--------+-----------
>   child1  | tg     | O
>   parent  | tg     | O
> (2 rows)
>
> ALTER TABLE
>   tgrelid | tgname | tgenabled
> ---------+--------+-----------
>   child1  | tg     | O
>   parent  | tg     | A
> (2 rows)
>
> ALTER TABLE
>   tgrelid | tgname | tgenabled
> ---------+--------+-----------
>   child1  | tg     | A
>   parent  | tg     | A
> (2 rows)
>
> DROP TABLE
> DROP FUNCTION
> ----
> The patch doesn't start recursion in case 'tgenabled' flag of parent
> table is not changes.
> Probably vanilla result is more correct.

Thanks for the review and this test case.

I agree that the patch shouldn't have changed that behavior, so I've
fixed the patch so that EnableDisableTrigger() recurses even if the
parent trigger is unchanged.


--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

> I agree that the patch shouldn't have changed that behavior, so I've
> fixed the patch so that EnableDisableTrigger() recurses even if the
> parent trigger is unchanged.

Thanks, I think this patch is ready for committer.

-- 
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com

On Thu, Jul 14, 2022 at 8:20 PM Dmitry Koval <d.koval@postgrespro.ru> wrote:
> > I agree that the patch shouldn't have changed that behavior, so I've
> > fixed the patch so that EnableDisableTrigger() recurses even if the
> > parent trigger is unchanged.
>
> Thanks, I think this patch is ready for committer.

Great, thanks.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-May-24, Amit Langote wrote:

> So, I think we should do something like the attached.  A lot of
> boilerplate is needed given that the various enable/disable trigger
> variants are represented as separate sub-commands (AlterTableCmd
> subtypes), which can perhaps be avoided by inventing a
> EnableDisableTrigStmt sub-command node that stores (only?) the recurse
> flag.

Yeah, I don't know about adding tons of values to that enum just so that
we can use that to hide a boolean inside.  Why not add a boolean to the
containing struct?  Something like the attached.

We can later use the same thing to undo what happens in in AddColumn,
DropColumn, etc.  It all looks pretty strange and confusing to me.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Yeah, I don't know about adding tons of values to that enum just so that
> we can use that to hide a boolean inside.  Why not add a boolean to the
> containing struct?  Something like the attached.

I do not think it's a great idea to have ALTER TABLE scribbling on
the source parsetree.  That tree could be in plancache and subject
to reuse later.

Mind you, I don't say that we're perfectly clean about this elsewhere.
But there is a pretty hard expectation that the executor doesn't
modify plan trees, and I think the same rule should apply to utility
statements.

            regards, tom lane

On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Yeah, I don't know about adding tons of values to that enum just so that
> > we can use that to hide a boolean inside.  Why not add a boolean to the
> > containing struct?  Something like the attached.
>
> I do not think it's a great idea to have ALTER TABLE scribbling on
> the source parsetree.

Hmm, I think we already do scribble on the source parse tree even
before this patch, for example, as ATPrepCmd() does for DROP
CONSTRAINT:

            if (recurse)
                cmd->subtype = AT_DropConstraintRecurse;

>  That tree could be in plancache and subject
> to reuse later.

I see that 7c337b6b527b added 'readOnlyTree' to
standard_ProcessUtility()'s API, I guess, to make any changes that
AlterTable() and underlings make to the input AlterTableStmt be local
to a given execution.  Though, maybe that's not really a permission to
add more code that makes such changes?

In this case of needing to remember the inh/recurse flag mentioned in
the original AT command, we could avoid scribbling over the input
AlterTableStmt by setting a new flag in AlteredTableInfo, instead of
AlterTableCmd.  AlteredTableInfo has other runtime info about the
relation being altered and perhaps it wouldn't be too bad if it also
stores the inh/recurse flag.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-Aug-01, Amit Langote wrote:

> On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> > I do not think it's a great idea to have ALTER TABLE scribbling on
> > the source parsetree.
> 
> Hmm, I think we already do scribble on the source parse tree even
> before this patch, for example, as ATPrepCmd() does for DROP
> CONSTRAINT:
> 
>             if (recurse)
>                 cmd->subtype = AT_DropConstraintRecurse;

No, actually nothing scribbles on the parsetree, because ATPrepCmd is
working on a copy of the node, so there's no harm done to the original.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu.  Five minutes later I realize that it's also talking
about food" (Donald Knuth)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> No, actually nothing scribbles on the parsetree, because ATPrepCmd is
> working on a copy of the node, so there's no harm done to the original.

Oh, okay then.  Maybe this needs to be noted somewhere?

            regards, tom lane

On Tue, Aug 2, 2022 at 3:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Aug-01, Amit Langote wrote:
>
> > On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > > I do not think it's a great idea to have ALTER TABLE scribbling on
> > > the source parsetree.
> >
> > Hmm, I think we already do scribble on the source parse tree even
> > before this patch, for example, as ATPrepCmd() does for DROP
> > CONSTRAINT:
> >
> >             if (recurse)
> >                 cmd->subtype = AT_DropConstraintRecurse;
>
> No, actually nothing scribbles on the parsetree, because ATPrepCmd is
> working on a copy of the node, so there's no harm done to the original.

Oh, I missed this bit in ATPrepCmd():

    /*
     * Copy the original subcommand for each table.  This avoids conflicts
     * when different child tables need to make different parse
     * transformations (for example, the same column may have different column
     * numbers in different children).
     */
    cmd = copyObject(cmd);

That's copying for a different purpose than what Tom mentioned, but
copying nonetheless.  Maybe we should modify this comment a bit to
clarify about Tom's concern?

Regarding the patch, I agree that storing the recurse flag rather than
overwriting subtype might be better.

+   bool        execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must
+                                   * recurse to children */

Might it be better to call this field simply 'recurse'?  I think it's
clear from the context and the comment above the flag is to be used
during execution.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-Aug-02, Amit Langote wrote:

> Regarding the patch, I agree that storing the recurse flag rather than
> overwriting subtype might be better.
> 
> +   bool        execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must
> +                                   * recurse to children */
> 
> Might it be better to call this field simply 'recurse'?  I think it's
> clear from the context and the comment above the flag is to be used
> during execution.

Yeah, I guess we can do that and also reword the overall ALTER TABLE
comment about recursion.  That's in the attached first patch, which is
intended as backpatchable.

The second patch is just to show how we'd rewrite AT_AddColumn to no
longer use the Recurse separate enum value but instead use the ->recurse
flag.  This is pretty straightforward and it's a clear net reduction of
code.  We can't backpatch this kind of thing of course, both because of
the ABI break (easily fixed) and because potential destabilization
(scary).  We can do similar tihngs for the other AT enum values for
recursion.  This isn't complete since there are a few other values in
that enum that we should process in this way too; I don't intend it to
push it just yet.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"XML!" Exclaimed C++.  "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/

On Thu, Aug 4, 2022 at 3:01 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Aug-02, Amit Langote wrote:
> > Regarding the patch, I agree that storing the recurse flag rather than
> > overwriting subtype might be better.
> >
> > +   bool        execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must
> > +                                   * recurse to children */
> >
> > Might it be better to call this field simply 'recurse'?  I think it's
> > clear from the context and the comment above the flag is to be used
> > during execution.
>
> Yeah, I guess we can do that and also reword the overall ALTER TABLE
> comment about recursion.  That's in the attached first patch, which is
> intended as backpatchable.

Thanks.  This one looks good to me.

> The second patch is just to show how we'd rewrite AT_AddColumn to no
> longer use the Recurse separate enum value but instead use the ->recurse
> flag.  This is pretty straightforward and it's a clear net reduction of
> code.  We can't backpatch this kind of thing of course, both because of
> the ABI break (easily fixed) and because potential destabilization
> (scary).  We can do similar tihngs for the other AT enum values for
> recursion.  This isn't complete since there are a few other values in
> that enum that we should process in this way too; I don't intend it to
> push it just yet.

I like the idea of removing all AT_*Recurse subtypes in HEAD.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Another point for backpatch: EnableDisableTrigger() changes API, which
is potentially not good.  In backbranches I'll keep the function
unchanged and add another function with the added argument,
EnableDisableTriggerNew().

So extensions that want to be compatible with both old and current
versions (assuming any users of that function exist out of core; I
didn't find any) could do something like

#if PG_VERSION_NUM <= 160000
    EnableDisableTriggerNew( all args )
#else
    EnableDisableTrigger( all args )
#endif

and otherwise they're compatible as compiled today.

Since there are no known users of this interface, it doesn't seem to
warrant any more convenient treatment.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&cid=4647152)

On Thu, Aug 4, 2022 at 9:56 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Another point for backpatch: EnableDisableTrigger() changes API, which
> is potentially not good.  In backbranches I'll keep the function
> unchanged and add another function with the added argument,
> EnableDisableTriggerNew().

+1

> So extensions that want to be compatible with both old and current
> versions (assuming any users of that function exist out of core; I
> didn't find any) could do something like
>
> #if PG_VERSION_NUM <= 160000
>         EnableDisableTriggerNew( all args )
> #else
>         EnableDisableTrigger( all args )
> #endif
>
> and otherwise they're compatible as compiled today.
>
> Since there are no known users of this interface, it doesn't seem to
> warrant any more convenient treatment.

Makes sense.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

OK, pushed.  This soon caused buildfarm to show a failure due to
underspecified ORDER BY, so I just pushed a fix for that too.

Thanks Simon for reporting the problem, and thanks Amit for the patch.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Si quieres ser creativo, aprende el arte de perder el tiempo"

On Fri, Aug 5, 2022 at 6:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> OK, pushed.  This soon caused buildfarm to show a failure due to
> underspecified ORDER BY, so I just pushed a fix for that too.

Thank you.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com