Thread: Use outerPlanState macro instead of referring to leffttree

Richard Guo <guofenglinux@gmail.com> writes:
> In the executor code, we mix use outerPlanState macro and referring to
> leffttree. Commit 40f42d2a tried to keep the code consistent by
> replacing referring to lefftree with outerPlanState macro, but there are
> still some outliers. This patch tries to clean them up.

Seems generally reasonable, but what about righttree?  I find a few
of those too with "grep".

Backing up a little bit, one thing not to like about the outerPlanState
and innerPlanState macros is that they lose all semblance of type
safety:

#define innerPlanState(node)        (((PlanState *)(node))->righttree)
#define outerPlanState(node)        (((PlanState *)(node))->lefttree)

You can pass any pointer you want, and the compiler will not complain.
I wonder if there's any trick (even a gcc-only one) that could improve
on that.  In the absence of such a check, people might feel that
increasing our reliance on these macros isn't such a hot idea.

Now, the typical coding pattern you've used:

 ExecReScanHash(HashState *node)
 {
+    PlanState  *outerPlan = outerPlanState(node);

is probably reasonably secure against wrong-pointer slip-ups.  But
I'm less convinced about that for in-line usages in the midst of
a function, particularly in the common case that the function has
a variable pointing to its Plan node as well as PlanState node.
Would it make sense to try to use the local-variable style everywhere?

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> On Sat, Jul 2, 2022 at 5:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Backing up a little bit, one thing not to like about the outerPlanState
>> and innerPlanState macros is that they lose all semblance of type
>> safety:

> Your concern makes sense. I think outerPlan and innerPlan macros share
> the same issue. Not sure if there is a way to do the type check.

Yeah, I don't know of one either.  It needn't hold up this patch.

>> Would it make sense to try to use the local-variable style everywhere?

> Do you mean the pattern like below?
>   outerPlanState(hashstate) = ExecInitNode(outerPlan(node), estate, eflags);
> It seems that this pattern is mostly used when initializing child nodes
> with ExecInitNode(), and most calls to ExecInitNode() are using this
> pattern as a convention. Not sure if it's better to change them to
> local-variable style.

That's probably fine, especially if it's a commonly used pattern.

Typically, if one applies outerPlan() or outerPlanState() to the
wrong pointer, the mistake will become obvious upon even minimal
testing.  My concern here is more about usages in edge cases that
perhaps escape testing, for instance in the arguments of an
elog() for some nearly-can't-happen case.

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> Update the patch to include changes about righttree. But this doesn't
> include changes for edge case usages. (A rough look through shows to me
> that the current usages should be able to be covered by tests.)

I found a couple other places by grepping, and adjusted those too.
Pushed.

            regards, tom lane