Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o91Xm-0008HH-0D for pgsql-hackers@arkaria.postgresql.org; Wed, 06 Jul 2022 09:42:02 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1o91Xk-00050J-El for pgsql-hackers@arkaria.postgresql.org; Wed, 06 Jul 2022 09:42:00 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o91Xk-00050A-27 for pgsql-hackers@lists.postgresql.org; Wed, 06 Jul 2022 09:42:00 +0000 Received: from mail-il1-x134.google.com ([2607:f8b0:4864:20::134]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1o91Xh-0005Hz-SD for pgsql-hackers@lists.postgresql.org; Wed, 06 Jul 2022 09:41:59 +0000 Received: by mail-il1-x134.google.com with SMTP id k1so4300485ilu.1 for ; Wed, 06 Jul 2022 02:41:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=z85r3tGGNpDHgT8GpO173JsmJm2HkMvRWYzn9G+oD0s=; b=NqvehWGFHprnuL+5u410u8kSi2MoGehciB8AgnhhoKWjiIw7a0Czs339AWEteJ7XiH Y9HrvngrKn/zTDtFFQJKOlgABng+nINpUOE8ar/h+QabAaP3MyAHGaMGmzryiM3By6xO q5FGmXKGnpW1looFwvMM7xwjTIS7oTQeGly7wVhLu+VmEsRMzRssL3mdLCqYnswz8RyL H4gmlQSmTTCz2mQRmlDaOFJzONqUccCjBNZC8JYwNas7hi3Fsku8UZxn066MzG/oeHPc nqk9Z09CYYckOoLIOp0jJt10G0YYYzXjrZNIBBKqqTxKVFfEchdN4DKbKMiF3EFYRDEt QW/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=z85r3tGGNpDHgT8GpO173JsmJm2HkMvRWYzn9G+oD0s=; b=Gh8pEJeAUp2mE/liq8TH2OupJWpxBhxRZKCf8XKI/JUjGtxi9JyTo3CYnEM78UZwmW /K1VlFE7Epz2+eMq5daWvJ0RiNb+sH3a9OguKj3g7mhSSRdxhvNTIL7rLTTxyqRYF9ti 0rwnltS+cA2/I2GvBMgdyCFATTNwnvxN1QaW6qLuNo8BYCBSp5UP84yn4naw6rZpLRsl sGHAoj0gC3nhVwT3doszZRcRYI+qb2aOOJ//o7FU7lh7pJYo/0lPFHD4HYgAKGndFm/p HWs/X8hyVWlnKp8FYj540Wk/8eCWMkZPYqOimSLfrLk7bdN/qA9xF3rrXwlXyjvrwGeE AXZA== X-Gm-Message-State: AJIora+EZAF2keZoOlglD93jhTviparChKlwskEgC2vw+OaVl+xS383a EdA5STSsCEJqvxt5lyv89dDYi4UPfjB/o+etySAdpdFglJY= X-Google-Smtp-Source: AGRyM1t84jwxnzEGi96nHZZXlWzQWStwxad6bNPti/QMByNEMWltLBQXJJ3IY0Vtk8+vCrj8otQemr881myhU3osWFI= X-Received: by 2002:a05:6e02:1caf:b0:2dc:2bc9:7f5b with SMTP id x15-20020a056e021caf00b002dc2bc97f5bmr3490600ill.311.1657100515182; Wed, 06 Jul 2022 02:41:55 -0700 (PDT) MIME-Version: 1.0 References: <95731.1656711122@sss.pgh.pa.us> In-Reply-To: <95731.1656711122@sss.pgh.pa.us> From: Richard Guo Date: Wed, 6 Jul 2022 17:41:44 +0800 Message-ID: Subject: Re: Use outerPlanState macro instead of referring to leffttree To: Tom Lane Cc: Pg Hackers Content-Type: multipart/alternative; boundary="000000000000908ca105e31fc767" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000908ca105e31fc767 Content-Type: text/plain; charset="UTF-8" Thanks for reviewing this patch. On Sat, Jul 2, 2022 at 5:32 AM Tom Lane wrote: > Richard Guo 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 --000000000000908ca105e31fc767 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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> wr= ites:
> 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 a= re
> still some outliers. This patch tries to clean them up.

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

Yes= . We may do the same trick for righttree.
=C2=A0

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)=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (((Pl= anState *)(node))->righttree)
#define outerPlanState(node)=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (((Pl= anState *)(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<= br> on that.=C2=A0 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.
=C2=A0

Now, the typical coding pattern you've used:

=C2=A0ExecReScanHash(HashState *node)
=C2=A0{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0PlanState=C2=A0 *outerPlan =3D outerPlanState(n= ode);

is probably reasonably secure against wrong-pointer slip-ups.=C2=A0 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?
<= /blockquote>

Do you mean the pattern like below?

=C2= =A0 outerPlanState(hashstate) =3D ExecInitNode(outerPlan(node), estate, efl= ags);

It seems that this pattern is mostly used when initializing ch= ild nodes
with ExecInitNode(), and most calls to ExecInitNode() are usin= g this
pattern as a convention. Not sure if it's better to change th= em to
local-variable style.

Thanks
Richard
--000000000000908ca105e31fc767--