Resend this email to -hackers. Sorry for the noise.
Thanks
Richard
---------- Forwarded message ---------
From: Richard Guo <guofengli...@gmail.com>
Date: Thu, Feb 2, 2023 at 9:51 AM
Subject: Re: pgsql: Remove over-optimistic Assert.
To: Tom Lane <t...@sss.pgh.pa.us>
Cc: <pgsql-committ...@lists.postgresql.org>, PostgreSQL-development <
pgsql-hack...@postgresql.org>
On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Remove over-optimistic Assert.
>
> In commit 2489d76c4, I'd thought it'd be safe to assert that a
> PlaceHolderVar appearing in a scan-level expression has empty
> nullingrels. However this is not so, as when we determine that a
> join relation is certainly empty we'll put its targetlist into a
> Result-with-constant-false-qual node, and nothing is done to adjust
> the nullingrels of the Vars or PHVs therein. (Arguably, a Result
> used in this way isn't really a scan-level node, but it certainly
> isn't an upper node either ...)
It seems this is the only case we can have PlaceHolderVar with non-empty
nullingrels at scan level. So I wonder if we can manually adjust the
nullingrels of PHVs in this special case, and keep the assertion about
phnullingrels being NULL in fix_scan_expr. I think that assertion is
asserting the right thing in most cases. It's a pity to lose it.
Currently for the tlist of a childless Result, we special-case ROWID_VAR
Vars in set_plan_refs and thus keep assertions about varno != ROWID_VAR
in fix_scan_expr. Do you think we can special-case PHVs at the same
place by setting its phnullingrels to NULL? I'm imagining something
like attached.
Thanks
Richard
From 41fc3b6855e5b6e525ec46a276a0892a321045e0 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 2 Feb 2023 09:06:53 +0800
Subject: [PATCH v1] Adjust phnullingrels for childless Result
---
src/backend/optimizer/plan/setrefs.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 186fc8014b..f6d8eaf1df 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1045,16 +1045,32 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
* expected to occur here, it seems safer to special-case
* it here and keep the assertions that ROWID_VARs
* shouldn't be seen by fix_scan_expr.
+ *
+ * The tlist of a childless Result could contain PHVs with
+ * non-empty phnullingrels, in case it's a dummy plan
+ * generated for empty join relation. Special-case it here
+ * by setting phnullingrels to NULL and keep the assertion
+ * about phnullingrels being NULL in fix_scan_expr.
*/
foreach(l, splan->plan.targetlist)
{
TargetEntry *tle = (TargetEntry *) lfirst(l);
- Var *var = (Var *) tle->expr;
- if (var && IsA(var, Var) && var->varno == ROWID_VAR)
- tle->expr = (Expr *) makeNullConst(var->vartype,
- var->vartypmod,
- var->varcollid);
+ if (tle->expr && IsA(tle->expr, Var))
+ {
+ Var *var = (Var *) tle->expr;
+
+ if (var->varno == ROWID_VAR)
+ tle->expr = (Expr *) makeNullConst(var->vartype,
+ var->vartypmod,
+ var->varcollid);
+ }
+ else if (tle->expr && IsA(tle->expr, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) tle->expr;
+
+ phv->phnullingrels = NULL;
+ }
}
splan->plan.targetlist =
@@ -2205,7 +2221,7 @@ fix_scan_expr_mutator(Node *node, fix_scan_expr_context *context)
/* At scan level, we should always just evaluate the contained expr */
PlaceHolderVar *phv = (PlaceHolderVar *) node;
- /* XXX can we assert something about phnullingrels? */
+ Assert(phv->phnullingrels == NULL);
return fix_scan_expr_mutator((Node *) phv->phexpr, context);
}
if (IsA(node, AlternativeSubPlan))
--
2.31.0
