From: | Richard Guo <guofenglinux(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Use outerPlanState macro instead of referring to leffttree |
Date: | 2022-07-06 09:41:44 |
Message-ID: | CAMbWs4-yHp5-ejJ6hywNamiXJgv-6K4bnyFQqvS3GXu-XHoExw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for reviewing this patch.
On Sat, Jul 2, 2022 at 5:32 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Richard Guo <guofenglinux(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Jehan-Guillaume de Rorthais | 2022-07-06 09:54:10 | Re: Fix proposal for comparaison bugs in PostgreSQL::Version |
Previous Message | Amit Kapila | 2022-07-06 09:18:43 | Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication |