On Thu, Feb 09, 2023 at 06:12:50PM -0500, Tom Lane wrote:
> I'm okay with the pathnodes.h changes --- although surely you don't need
> changes like this:
>
> - pg_node_attr(abstract)
> + pg_node_attr(abstract, no_query_jumble)
>
> "abstract" should already imply "no_query_jumble".
Okay, understood. Following this string of thoughts, I am a bit
surprised for two cases, though:
- PartitionPruneStep.
- Plan.
Both are abstract and both are marked with no_equal. I guess that
applying no_query_jumble to both of them is fine, and that's what you
mean?
> I wonder too if you could shorten the changes by making no_query_jumble
> an inheritable attribute, and then just applying it to Path and Plan.
Ah. I did not catch what you meant here at first, but I think that I
do now. Are you referring to the part of gen_node_support.pl where we
propagate properties when a node is a supertype? This part would be
taken into account when a node is parsed but we find that its first
member is already tracked as a node:
# Propagate some node attributes from supertypes
if ($supertype)
{
push @no_copy, $in_struct
if elem $supertype, @no_copy;
push @no_equal, $in_struct
if elem $supertype, @no_equal;
push @no_read, $in_struct
if elem $supertype, @no_read;
+ push @no_query_jumble, $in_struct
+ if elem $supertype, @no_query_jumble;
}
A benefit of doing that would also discard all the Scan and Sort
nodes. So like the other no_* attributes, it makes sense to force the
inheritance here.
> The changes in parsenodes.h seem wrong, except for RawStmt. Those node
> types are used in parsed queries, aren't they?
RTEPermissionInfo is a recent addition, as of a61b1f7. This commit
documents it as a plan node, still it is part of a Query while being
ignored in the query jumbling since its introduction, so I am a bit
confused by this one.
Anyway, none of these need to be included in the query jumbling
currently because they are ignored, but I'd be fine to generate their
code by default as they could become relevant if other nodes begin to
rely on them more heavily, as being part of queries. Peter E. has
mentioned upthread that a few nodes should include more jumbling while
some other parts should be ignored. This should be analyzed
separately because ~15 does not seem to be strictly right, either.
Attached is a patch refreshed with all that. Feel free to ignore 0002
as that's just useful to enforce the tests to go through the jumbling
code. The attached reaches 95.0% of line coverage after a check-world
in funcs.c.
Thoughts?
--
Michael