Subject: Use outerPlanState macro instead of referring to leffttree

From: Richard Guo <[email-masked]>
Date: Thu, 14 Apr 2022 14:49:23 +0800

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Jul 2022 17:32:02 -0400

> 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

---

From: Richard Guo <[email-masked]>
Date: Wed, 6 Jul 2022 17:41:44 +0800

Thanks for reviewing this patch.

> of those too with "grep".

Yes. We may do the same trick for righttree.

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

> 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

---

From: Tom Lane <[email-masked]>
Date: Wed, 06 Jul 2022 10:48:21 -0400

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

> pattern as a convention. Not sure if it's better to change them to

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

---

From: Richard Guo <[email-masked]>
Date: Thu, 7 Jul 2022 14:59:27 +0800

> 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

---

From: Tom Lane <[email-masked]>
Date: Thu, 07 Jul 2022 11:25:19 -0400

> 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