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
