Re: enable/disable broken for statement triggers on partitioned tables

On Tue, Aug 2, 2022 at 3:58 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2022-Aug-01, Amit Langote wrote:
>
> > On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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