RangeTblEntry.inh vs. RTE_SUBQUERY
Various code comments say that the RangeTblEntry field inh may only be
set for entries of kind RTE_RELATION.
For example
* inh is true for relation references that should be expanded to
include
* inheritance children, if the rel has any. This *must* be false for
* RTEs other than RTE_RELATION entries.
and various comments in other files.
(Confusingly, it is also listed under "Fields valid in all RTEs:", but
that definitely seems wrong.)
I have been deploying some assertions to see if the claims in the
RangeTblEntry comments are all correct, and I tripped over something.
The function pull_up_simple_union_all() in prepjointree.c sets ->inh to
true for RTE_SUBQUERY entries:
/*
* Mark the parent as an append relation.
*/
rte->inh = true;
Whatever this is doing appears to be some undocumented magic. If I
remove the line, then regression tests fail with plan differences, so it
definitely seems to do something.
Is this something we should explain the RangeTblEntry comments?
On Fri, 23 Feb 2024 at 14:35, Peter Eisentraut <peter@eisentraut.org> wrote:
Various code comments say that the RangeTblEntry field inh may only be
set for entries of kind RTE_RELATION.The function pull_up_simple_union_all() in prepjointree.c sets ->inh to
true for RTE_SUBQUERY entries:/*
* Mark the parent as an append relation.
*/
rte->inh = true;Whatever this is doing appears to be some undocumented magic.
Yes, it's explained a bit more clearly/accurately in expand_inherited_rtentry():
/*
* expand_inherited_rtentry
* Expand a rangetable entry that has the "inh" bit set.
*
* "inh" is only allowed in two cases: RELATION and SUBQUERY RTEs.
*
* "inh" on a plain RELATION RTE means that it is a partitioned table or the
* parent of a traditional-inheritance set. In this case we must add entries
* for all the interesting child tables to the query's rangetable, and build
* additional planner data structures for them, including RelOptInfos,
* AppendRelInfos, and possibly PlanRowMarks.
*
* Note that the original RTE is considered to represent the whole inheritance
* set. In the case of traditional inheritance, the first of the generated
* RTEs is an RTE for the same table, but with inh = false, to represent the
* parent table in its role as a simple member of the inheritance set. For
* partitioning, we don't need a second RTE because the partitioned table
* itself has no data and need not be scanned.
*
* "inh" on a SUBQUERY RTE means that it's the parent of a UNION ALL group,
* which is treated as an appendrel similarly to inheritance cases; however,
* we already made RTEs and AppendRelInfos for the subqueries. We only need
* to build RelOptInfos for them, which is done by expand_appendrel_subquery.
*/
Is this something we should explain the RangeTblEntry comments?
+1
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
On Fri, 23 Feb 2024 at 14:35, Peter Eisentraut <peter@eisentraut.org> wrote:
Various code comments say that the RangeTblEntry field inh may only be
set for entries of kind RTE_RELATION.
Yes, it's explained a bit more clearly/accurately in expand_inherited_rtentry():
* "inh" is only allowed in two cases: RELATION and SUBQUERY RTEs.
Yes. The latter has been accurate for a very long time, so I'm
surprised that there are any places that think otherwise. We need
to fix them --- where did you see this exactly?
(Note that RELATION-only is accurate within the parser and rewriter,
so maybe clarifications about context are in order.)
regards, tom lane
On 23.02.24 16:19, Tom Lane wrote:
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
On Fri, 23 Feb 2024 at 14:35, Peter Eisentraut <peter@eisentraut.org> wrote:
Various code comments say that the RangeTblEntry field inh may only be
set for entries of kind RTE_RELATION.Yes, it's explained a bit more clearly/accurately in expand_inherited_rtentry():
* "inh" is only allowed in two cases: RELATION and SUBQUERY RTEs.
Yes. The latter has been accurate for a very long time, so I'm
surprised that there are any places that think otherwise. We need
to fix them --- where did you see this exactly?
In nodes/parsenodes.h, it says both
This *must* be false for RTEs other than RTE_RELATION entries.
and also puts it under
Fields valid in all RTEs:
which are both wrong on opposite ends of the spectrum.
I think it would make more sense to group inh under "Fields valid for a
plain relation RTE" and then explain the exception for subqueries, like
it is done for several other fields.
See attached patch for a proposal. (I also shuffled a few fields around
to make the order a bit more logical.)
Attachments:
0001-Fix-description-and-grouping-of-RangeTblEntry.inh.patchtext/plain; charset=UTF-8; name=0001-Fix-description-and-grouping-of-RangeTblEntry.inh.patchDownload
From 631fa6a211f7d2022e78e5683ed234a00ae144bd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 29 Feb 2024 13:06:48 +0100
Subject: [PATCH] Fix description and grouping of RangeTblEntry.inh
---
src/backend/nodes/outfuncs.c | 5 +++--
src/backend/nodes/queryjumblefuncs.c | 2 +-
src/backend/nodes/readfuncs.c | 5 +++--
src/backend/parser/parse_relation.c | 11 ++---------
src/include/nodes/parsenodes.h | 15 +++++++++------
5 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2c30bba212..1c66be3af8 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -505,8 +505,9 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
WRITE_OID_FIELD(relid);
WRITE_CHAR_FIELD(relkind);
WRITE_INT_FIELD(rellockmode);
- WRITE_NODE_FIELD(tablesample);
WRITE_UINT_FIELD(perminfoindex);
+ WRITE_BOOL_FIELD(inh);
+ WRITE_NODE_FIELD(tablesample);
break;
case RTE_SUBQUERY:
WRITE_NODE_FIELD(subquery);
@@ -516,6 +517,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
WRITE_CHAR_FIELD(relkind);
WRITE_INT_FIELD(rellockmode);
WRITE_UINT_FIELD(perminfoindex);
+ WRITE_BOOL_FIELD(inh);
break;
case RTE_JOIN:
WRITE_ENUM_FIELD(jointype, JoinType);
@@ -564,7 +566,6 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
}
WRITE_BOOL_FIELD(lateral);
- WRITE_BOOL_FIELD(inh);
WRITE_BOOL_FIELD(inFromCl);
WRITE_NODE_FIELD(securityQuals);
}
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index 82f725baaa..1900b8bca4 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -364,8 +364,8 @@ _jumbleRangeTblEntry(JumbleState *jstate, Node *node)
{
case RTE_RELATION:
JUMBLE_FIELD(relid);
- JUMBLE_NODE(tablesample);
JUMBLE_FIELD(inh);
+ JUMBLE_NODE(tablesample);
break;
case RTE_SUBQUERY:
JUMBLE_NODE(subquery);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index b1e2f2b440..b80441e0c2 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -359,8 +359,9 @@ _readRangeTblEntry(void)
READ_OID_FIELD(relid);
READ_CHAR_FIELD(relkind);
READ_INT_FIELD(rellockmode);
- READ_NODE_FIELD(tablesample);
READ_UINT_FIELD(perminfoindex);
+ READ_BOOL_FIELD(inh);
+ READ_NODE_FIELD(tablesample);
break;
case RTE_SUBQUERY:
READ_NODE_FIELD(subquery);
@@ -370,6 +371,7 @@ _readRangeTblEntry(void)
READ_CHAR_FIELD(relkind);
READ_INT_FIELD(rellockmode);
READ_UINT_FIELD(perminfoindex);
+ READ_BOOL_FIELD(inh);
break;
case RTE_JOIN:
READ_ENUM_FIELD(jointype, JoinType);
@@ -428,7 +430,6 @@ _readRangeTblEntry(void)
}
READ_BOOL_FIELD(lateral);
- READ_BOOL_FIELD(inh);
READ_BOOL_FIELD(inFromCl);
READ_NODE_FIELD(securityQuals);
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 34a0ec5901..9a62c822e1 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1502,6 +1502,7 @@ addRangeTableEntry(ParseState *pstate,
rte->relid = RelationGetRelid(rel);
rte->relkind = rel->rd_rel->relkind;
rte->rellockmode = lockmode;
+ rte->inh = inh;
/*
* Build the list of effective column names using user-supplied aliases
@@ -1517,7 +1518,6 @@ addRangeTableEntry(ParseState *pstate,
* which is the right thing for all except target tables.
*/
rte->lateral = false;
- rte->inh = inh;
rte->inFromCl = inFromCl;
perminfo = addRTEPermissionInfo(&pstate->p_rteperminfos, rte);
@@ -1587,6 +1587,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
rte->relid = RelationGetRelid(rel);
rte->relkind = rel->rd_rel->relkind;
rte->rellockmode = lockmode;
+ rte->inh = inh;
/*
* Build the list of effective column names using user-supplied aliases
@@ -1602,7 +1603,6 @@ addRangeTableEntryForRelation(ParseState *pstate,
* which is the right thing for all except target tables.
*/
rte->lateral = false;
- rte->inh = inh;
rte->inFromCl = inFromCl;
perminfo = addRTEPermissionInfo(&pstate->p_rteperminfos, rte);
@@ -1700,7 +1700,6 @@ addRangeTableEntryForSubquery(ParseState *pstate,
* addRTEPermissionInfo().
*/
rte->lateral = lateral;
- rte->inh = false; /* never true for subqueries */
rte->inFromCl = inFromCl;
/*
@@ -2023,7 +2022,6 @@ addRangeTableEntryForFunction(ParseState *pstate,
* ExecCheckPermissions()), so no need to perform addRTEPermissionInfo().
*/
rte->lateral = lateral;
- rte->inh = false; /* never true for functions */
rte->inFromCl = inFromCl;
/*
@@ -2108,7 +2106,6 @@ addRangeTableEntryForTableFunc(ParseState *pstate,
* ExecCheckPermissions()), so no need to perform addRTEPermissionInfo().
*/
rte->lateral = lateral;
- rte->inh = false; /* never true for tablefunc RTEs */
rte->inFromCl = inFromCl;
/*
@@ -2189,7 +2186,6 @@ addRangeTableEntryForValues(ParseState *pstate,
* addRTEPermissionInfo().
*/
rte->lateral = lateral;
- rte->inh = false; /* never true for values RTEs */
rte->inFromCl = inFromCl;
/*
@@ -2280,7 +2276,6 @@ addRangeTableEntryForJoin(ParseState *pstate,
* addRTEPermissionInfo().
*/
rte->lateral = false;
- rte->inh = false; /* never true for joins */
rte->inFromCl = inFromCl;
/*
@@ -2425,7 +2420,6 @@ addRangeTableEntryForCTE(ParseState *pstate,
* addRTEPermissionInfo().
*/
rte->lateral = false;
- rte->inh = false; /* never true for subqueries */
rte->inFromCl = inFromCl;
/*
@@ -2545,7 +2539,6 @@ addRangeTableEntryForENR(ParseState *pstate,
* addRTEPermissionInfo().
*/
rte->lateral = false;
- rte->inh = false; /* never true for ENRs */
rte->inFromCl = inFromCl;
/*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index baa6a97c7e..69dd01227a 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -981,10 +981,6 @@ typedef struct PartitionCmd
* them from the joinaliasvars list, because that would affect the attnums
* of Vars referencing the rest of the list.)
*
- * inh is true for relation references that should be expanded to include
- * inheritance children, if the rel has any. This *must* be false for
- * RTEs other than RTE_RELATION entries.
- *
* inFromCl marks those range variables that are listed in the FROM clause.
* It's false for RTEs that are added to a query behind the scenes, such
* as the NEW and OLD variables for a rule, or the subqueries of a UNION.
@@ -1044,6 +1040,9 @@ typedef struct RangeTblEntry
* target table. We leave such RTEs with their original lockmode so as to
* avoid getting an additional, lesser lock.
*
+ * inh is true for relation references that should be expanded to include
+ * inheritance children, if the rel has any.
+ *
* perminfoindex is 1-based index of the RTEPermissionInfo belonging to
* this RTE in the containing struct's list of same; 0 if permissions need
* not be checked for this RTE.
@@ -1056,6 +1055,10 @@ typedef struct RangeTblEntry
* in the query anymore, and the most expedient way to do that is to
* retain these fields from the old state of the RTE.
*
+ * As a special case, inh may also be true for RTE_SUBQUERY entries in the
+ * planner (but not in the parser or rewriter); see
+ * expand_inherited_rtentry().
+ *
* As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate
* that the tuple format of the tuplestore is the same as the referenced
* relation. This allows plans referencing AFTER trigger transition
@@ -1064,8 +1067,9 @@ typedef struct RangeTblEntry
Oid relid; /* OID of the relation */
char relkind; /* relation kind (see pg_class.relkind) */
int rellockmode; /* lock level that query requires on the rel */
- struct TableSampleClause *tablesample; /* sampling info, or NULL */
Index perminfoindex;
+ bool inh; /* inheritance requested? */
+ struct TableSampleClause *tablesample; /* sampling info, or NULL */
/*
* Fields valid for a subquery RTE (else NULL):
@@ -1191,7 +1195,6 @@ typedef struct RangeTblEntry
Alias *alias; /* user-written alias clause, if any */
Alias *eref; /* expanded reference names */
bool lateral; /* subquery, function, or values is LATERAL? */
- bool inh; /* inheritance requested? */
bool inFromCl; /* present in FROM clause? */
List *securityQuals; /* security barrier quals to apply, if any */
} RangeTblEntry;
--
2.43.2
Peter Eisentraut <peter@eisentraut.org> writes:
In nodes/parsenodes.h, it says both
This *must* be false for RTEs other than RTE_RELATION entries.
Well, that's true in the parser ...
and also puts it under
Fields valid in all RTEs:
which are both wrong on opposite ends of the spectrum.
I think it would make more sense to group inh under "Fields valid for a
plain relation RTE" and then explain the exception for subqueries, like
it is done for several other fields.
Dunno. The adjacent "lateral" field is also used for only selected
RTE kinds.
I'd be inclined to leave it where it is and just improve the
commentary. That could read like
* inh is true for relation references that should be expanded to include
* inheritance children, if the rel has any. In the parser this
* will only be true for RTE_RELATION entries. The planner also uses
* this field to mark RTE_SUBQUERY entries that contain UNION ALL
* queries that it has flattened into pulled-up subqueries
* (creating a structure much like the effects of inheritance).
If you do insist on moving it, please put it next to relkind so it
packs better.
I agree that perminfoindex seems to have suffered from add-at-the-end
syndrome, and if we do touch the field order you made an improvement
there. (BTW, who thought they needn't bother with a comment for
perminfoindex?)
regards, tom lane
On 2024-Feb-29, Tom Lane wrote:
I agree that perminfoindex seems to have suffered from add-at-the-end
syndrome, and if we do touch the field order you made an improvement
there. (BTW, who thought they needn't bother with a comment for
perminfoindex?)
There is a comment for it, or at least a61b1f74823c added one, though
not immediately adjacent. I do see that it's now further away than it
was. Perhaps we could add /* index of RTEPermissionInfo entry, or 0 */
to the line.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The ability of users to misuse tools is, of course, legendary" (David Steele)
/messages/by-id/11b38a96-6ded-4668-b772-40f992132797@pgmasters.net
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2024-Feb-29, Tom Lane wrote:
I agree that perminfoindex seems to have suffered from add-at-the-end
syndrome, and if we do touch the field order you made an improvement
there. (BTW, who thought they needn't bother with a comment for
perminfoindex?)
There is a comment for it, or at least a61b1f74823c added one, though
not immediately adjacent. I do see that it's now further away than it
was. Perhaps we could add /* index of RTEPermissionInfo entry, or 0 */
to the line.
That'd be enough for me.
regards, tom lane
On 29.02.24 19:14, Tom Lane wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
In nodes/parsenodes.h, it says both
This *must* be false for RTEs other than RTE_RELATION entries.Well, that's true in the parser ...
and also puts it under
Fields valid in all RTEs:
which are both wrong on opposite ends of the spectrum.
I think it would make more sense to group inh under "Fields valid for a
plain relation RTE" and then explain the exception for subqueries, like
it is done for several other fields.Dunno. The adjacent "lateral" field is also used for only selected
RTE kinds.
The section is
/*
* Fields valid in all RTEs:
*/
Alias *alias; /* user-written alias clause, if any */
Alias *eref; /* expanded reference names */
bool lateral; /* subquery, function, or values is
LATERAL? */
bool inh; /* inheritance requested? */
bool inFromCl; /* present in FROM clause? */
List *securityQuals; /* security barrier quals to apply, if
any */
According to my testing, lateral is used for RTE_RELATION, RTE_SUBQUERY,
RTE_FUNCTION, RTE_TABLEFUNC, RTE_VALUES, which is 5 out of 9 possible.
So I think it might be okay to relabel that section (in actuality or
mentally) as "valid in several/many/most RTEs".
But I'm not sure what reason there would be for having inh there, which
is better described as "valid for RTE_RELATION, but also borrowed by
RTE_SUBQUERY", which is pretty much exactly what is the case for relid,
relkind, etc.
On 03.03.24 11:02, Peter Eisentraut wrote:
On 29.02.24 19:14, Tom Lane wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
In nodes/parsenodes.h, it says both
This *must* be false for RTEs other than RTE_RELATION entries.Well, that's true in the parser ...
and also puts it under
Fields valid in all RTEs:
which are both wrong on opposite ends of the spectrum.
I think it would make more sense to group inh under "Fields valid for a
plain relation RTE" and then explain the exception for subqueries, like
it is done for several other fields.Dunno. The adjacent "lateral" field is also used for only selected
RTE kinds.The section is
/*
* Fields valid in all RTEs:
*/
Alias *alias; /* user-written alias clause, if any */
Alias *eref; /* expanded reference names */
bool lateral; /* subquery, function, or values is
LATERAL? */
bool inh; /* inheritance requested? */
bool inFromCl; /* present in FROM clause? */
List *securityQuals; /* security barrier quals to apply, if
any */According to my testing, lateral is used for RTE_RELATION, RTE_SUBQUERY,
RTE_FUNCTION, RTE_TABLEFUNC, RTE_VALUES, which is 5 out of 9 possible.
So I think it might be okay to relabel that section (in actuality or
mentally) as "valid in several/many/most RTEs".But I'm not sure what reason there would be for having inh there, which
is better described as "valid for RTE_RELATION, but also borrowed by
RTE_SUBQUERY", which is pretty much exactly what is the case for relid,
relkind, etc.
I have committed the patches for this discussion.
Related discussion will continue at
/messages/by-id/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
/ https://commitfest.postgresql.org/47/4697/ .