Horton

Thread

Commits

  1. Remove stray references to lefttree/righttree in the executor.

  2. Use outerPlanState macro instead of referring to leffttree.

  1. Use outerPlanState macro instead of referring to leffttree

    Richard Guo <guofenglinux@gmail.com> — 2022-04-14T06:49:23Z

    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.

Thanks
Richard

  2. Re: Use outerPlanState macro instead of referring to leffttree

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-01T21:32:02Z

    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

  3. Re: Use outerPlanState macro instead of referring to leffttree

    Richard Guo <guofenglinux@gmail.com> — 2022-07-06T09:41:44Z

    Thanks for reviewing this patch.

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

> 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".
>

Yes. We may do the same trick for righttree.


>
> 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.
>

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.


>
> 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?
>

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.

Thanks
Richard

  4. Re: Use outerPlanState macro instead of referring to leffttree

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-06T14:48:21Z

    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

  5. Re: Use outerPlanState macro instead of referring to leffttree

    Richard Guo <guofenglinux@gmail.com> — 2022-07-07T06:59:27Z

    On Wed, Jul 6, 2022 at 10:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 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.


Yeah, concur with that. For edge case usages maybe we can use the
local-variable style to avoid wrong-pointer mistakes.

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.)

Thanks
Richard

  6. Re: Use outerPlanState macro instead of referring to leffttree

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-07T15:25:19Z

    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