From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Andrew Dunstan <andrew@dunslane.net>
Cc: torikoshia <torikoshia@oss.nttdata.com>
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: EXPLAIN(VERBOSE) to CTE with SEARCH BREADTH FIRST fails
Date: Wed, 15 Sep 2021 19:40:31 -0400
Message-ID: <1126187.1631749231@sss.pgh.pa.us> (raw)
In-Reply-To: <984394.1631719708@sss.pgh.pa.us>
References: <5bafa66ad529e11860339565c9e7c166@oss.nttdata.com>
<3208199.1631039493@sss.pgh.pa.us>
<b6fecc71-60b9-bd44-4b78-f2a9dd74f46f@enterprisedb.com>
<d70fa4dfd333b1de1024e50c2ad1a6ea@oss.nttdata.com>
<24ce73f9-c714-9a7c-6cc0-34e724c8f1c9@dunslane.net>
<984394.1631719708@sss.pgh.pa.us>
I wrote:
> I do not think that patch is a proper solution, but we do need to do
> something about this.
I poked into this and decided it's an ancient omission within ruleutils.c.
The reason we've not seen it before is probably that you can't get to the
case through the parser. The SEARCH stuff is generating a query structure
basically equivalent to
regression=# with recursive cte (x,r) as (
select 42 as x, row(i, 2.3) as r from generate_series(1,3) i
union all
select x, row((c.r).f1, 4.5) from cte c
)
select * from cte;
ERROR: record type has not been registered
and as you can see, expandRecordVariable fails to figure out what
the referent of "c.r" is. I think that could be fixed (by looking
into the non-recursive term), but given the lack of field demand,
I'm not feeling that it's urgent.
So the omission is pretty obvious from the misleading comment:
actually, Vars referencing RTE_CTE RTEs can also appear in WorkTableScan
nodes, and we're not doing anything to support that. But we only reach
this code when trying to resolve a field of a Var of RECORD type, which
is a case that it seems like the parser can't produce.
It doesn't look too hard to fix: we just have to find the RecursiveUnion
that goes with the WorkTableScan, and drill down into that, much as we
would do in the CteScan case. See attached draft patch. I'm too tired
to beat on this heavily or add a test case, but I have verified that it
passes check-world and handles the example presented in this thread.
regards, tom lane
Attachments:fix-explain-for-search-breadth-first-1.patch (text/x-diff, 3.2K) downloaddiff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.cindex b932a83827..b15bd81b9c 100644--- a/src/backend/utils/adt/ruleutils.c+++ b/src/backend/utils/adt/ruleutils.c@@ -375,6 +375,8 @@ static void identify_join_columns(JoinExpr *j, RangeTblEntry *jrte,
deparse_columns *colinfo);
static char *get_rtable_name(int rtindex, deparse_context *context);
static void set_deparse_plan(deparse_namespace *dpns, Plan *plan);
+static Plan *find_recursive_union(deparse_namespace *dpns,+ WorkTableScan *wtscan);
static void push_child_plan(deparse_namespace *dpns, Plan *plan,
deparse_namespace *save_dpns);
static void pop_child_plan(deparse_namespace *dpns,
@@ -4866,6 +4868,9 @@ set_deparse_plan(deparse_namespace *dpns, Plan *plan)
* For a SubqueryScan, pretend the subplan is INNER referent. (We don't
* use OUTER because that could someday conflict with the normal meaning.)
* Likewise, for a CteScan, pretend the subquery's plan is INNER referent.
+ * For a WorkTableScan, locate the parent RecursiveUnion plan node and use+ * that as INNER referent.+ *
* For ON CONFLICT .. UPDATE we just need the inner tlist to point to the
* excluded expression's tlist. (Similar to the SubqueryScan we don't want
* to reuse OUTER, it's used for RETURNING in some modify table cases,
@@ -4876,6 +4881,9 @@ set_deparse_plan(deparse_namespace *dpns, Plan *plan)
else if (IsA(plan, CteScan))
dpns->inner_plan = list_nth(dpns->subplans,
((CteScan *) plan)->ctePlanId - 1);
+ else if (IsA(plan, WorkTableScan))+ dpns->inner_plan = find_recursive_union(dpns,+ (WorkTableScan *) plan);
else if (IsA(plan, ModifyTable))
dpns->inner_plan = plan;
else
@@ -4899,6 +4907,29 @@ set_deparse_plan(deparse_namespace *dpns, Plan *plan)
dpns->index_tlist = NIL;
}
+/*+ * Locate the ancestor plan node that is the RecursiveUnion generating+ * the WorkTableScan's work table. We can match on wtParam, since that+ * should be unique within the plan tree.+ */+static Plan *+find_recursive_union(deparse_namespace *dpns, WorkTableScan *wtscan)+{+ ListCell *lc;++ foreach(lc, dpns->ancestors)+ {+ Plan *ancestor = (Plan *) lfirst(lc);++ if (IsA(ancestor, RecursiveUnion) &&+ ((RecursiveUnion *) ancestor)->wtParam == wtscan->wtParam)+ return ancestor;+ }+ elog(ERROR, "could not find RecursiveUnion for WorkTableScan with wtParam %d",+ wtscan->wtParam);+ return NULL;+}+
/*
* push_child_plan: temporarily transfer deparsing attention to a child plan
*
@@ -7646,9 +7677,12 @@ get_name_for_var_field(Var *var, int fieldno,
{
/*
* We're deparsing a Plan tree so we don't have a CTE
- * list. But the only place we'd see a Var directly- * referencing a CTE RTE is in a CteScan plan node, and we- * can look into the subplan's tlist instead.+ * list. But the only places we'd see a Var directly+ * referencing a CTE RTE are in CteScan or WorkTableScan+ * plan nodes. For those cases, set_deparse_plan arranged+ * for dpns->inner_plan to be the plan node that emits the+ * CTE or RecursiveUnion result, and we can look at its+ * tlist instead.
*/
TargetEntry *tle;
deparse_namespace save_dpns;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, peter.eisentraut@enterprisedb.com, andrew@dunslane.net, torikoshia@oss.nttdata.com
Subject: Re: EXPLAIN(VERBOSE) to CTE with SEARCH BREADTH FIRST fails
In-Reply-To: <1126187.1631749231@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox