enable/disable broken for statement triggers on partitioned tables

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.

Cheers

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&amp;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"