Horton

Thread

Commits

  1. Invent nodetag_only attribute for Nodes.

  1. Making CallContext and InlineCodeBlock less special-case-y

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-09T23:50:15Z

    As committed, gen_node_support.pl excludes CallContext and InlineCodeBlock
from getting unneeded support functions via some very ad-hoc code.
(Right now, there are some other node types that are handled similarly,
but I'm looking to drive that set to empty.)  After looking at the
situation a bit, I think the problem is that these nodes are declared
in parsenodes.h even though they have exactly nothing to do with
parse trees.  What they are is function-calling API infrastructure,
so it seems like the most natural home for them is fmgr.h.  A weaker
case could be made for funcapi.h, perhaps.

So I tried moving them to fmgr.h, and it blew up because they need
typedef NodeTag while fmgr.h does not #include nodes.h.  I feel that
the most reasonable approach is to just give up on that bit of
micro-optimization and let fmgr.h include nodes.h.  It was already
doing a bit of hackery to compile "Node *" references without that
inclusion, so this seems more clean not less so.

Hence, I propose the attached.  (The changes in the PL files are
just to align them on a common best practice for an InlineCodeBlock
argument.)

			regards, tom lane

  2. Re: Making CallContext and InlineCodeBlock less special-case-y

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-10T00:45:07Z

    I wrote:
> As committed, gen_node_support.pl excludes CallContext and InlineCodeBlock
> from getting unneeded support functions via some very ad-hoc code.
> (Right now, there are some other node types that are handled similarly,
> but I'm looking to drive that set to empty.)  After looking at the
> situation a bit, I think the problem is that these nodes are declared
> in parsenodes.h even though they have exactly nothing to do with
> parse trees.  What they are is function-calling API infrastructure,
> so it seems like the most natural home for them is fmgr.h.  A weaker
> case could be made for funcapi.h, perhaps.

On further thought, another way we could do this is to leave them where
they are but label them with a new attribute pg_node_attr(node_tag_only).
The big advantage of this idea is that it lets us explain
gen_node_support.pl's handling of execnodes.h and some other files as
"Nodes declared in these files are automatically assumed to be
node_tag_only.  At some future date we might label them explicitly
and remove the file-level assumption."  That gives us an easy fix
if we ever find ourselves wanting to supply support functions for
a subset of the nodes in one of those files.

This ties in a little bit with an idea I had for cleaning up the
other ad-hocery remaining in gen_node_support.pl.  It looks like
we are heading towards marking all the raw-parse-tree nodes and
utility-statement nodes as no_read, so as to be able to support them
in outfuncs but not readfuncs.  But if we're going to touch all of
those declarations, how about doing something a bit higher-level,
and marking them with semantic categories?  That is,
"pg_node_attr(raw_parse_node)" if the node appears in raw parse
trees but not anywhere later in the pipeline, or
"pg_node_attr(utility_statement)" if that's what it is.  Currently
these labels would just act as "no_read", but this approach would
make it a whole lot easier to change our minds later about how to
handle these categories of nodes.

I'm not entirely sure whether pg_node_attr(utility_statement) is
a better or worse idea than the inherit-from-UtilityStmt method
I posited in a nearby thread [1].  In principle we could do the
raw-parse-node labeling that way too, but for some reason it
doesn't seem quite as nice for raw parse nodes, mainly because
a subclass for them doesn't seem as well defined as one for
utility statements.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/4159834.1657405226%40sss.pgh.pa.us

  3. Re: Making CallContext and InlineCodeBlock less special-case-y

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-11T13:59:21Z

    On 10.07.22 01:50, Tom Lane wrote:
> As committed, gen_node_support.pl excludes CallContext and InlineCodeBlock
> from getting unneeded support functions via some very ad-hoc code.

Couldn't we just enable those support functions?  I think they were just 
excluded because they didn't have any before and nobody bothered to make 
any.

  4. Re: Making CallContext and InlineCodeBlock less special-case-y

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T14:11:49Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 10.07.22 01:50, Tom Lane wrote:
>> As committed, gen_node_support.pl excludes CallContext and InlineCodeBlock
>> from getting unneeded support functions via some very ad-hoc code.

> Couldn't we just enable those support functions?  I think they were just 
> excluded because they didn't have any before and nobody bothered to make 
> any.

Well, we could I suppose, but that path leads to a lot of dead code in
backend/nodes/ --- obviously these two alone are negligible, but I want
a story other than "it's a hack" for execnodes.h and the other files
we exclude from generation of support code.

After sleeping on it, I'm thinking the "pg_node_attr(nodetag_only)"
solution is the way to go, as that can lead to per-node rather than
per-file exclusion of support code, which we're surely going to want
eventually in more places.

			regards, tom lane

  5. Re: Making CallContext and InlineCodeBlock less special-case-y

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T23:01:33Z

    I wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 10.07.22 01:50, Tom Lane wrote:
>>> As committed, gen_node_support.pl excludes CallContext and InlineCodeBlock
>>> from getting unneeded support functions via some very ad-hoc code.

>> Couldn't we just enable those support functions?  I think they were just 
>> excluded because they didn't have any before and nobody bothered to make 
>> any.

> Well, we could I suppose, but that path leads to a lot of dead code in
> backend/nodes/ --- obviously these two alone are negligible, but I want
> a story other than "it's a hack" for execnodes.h and the other files
> we exclude from generation of support code.

Here's a proposed patch for this bit.  Again, whether these two
node types have unnecessary support functions is not the point ---
obviously we could afford to waste that much space.  Rather, what
I'm after is to have a more explainable and flexible way of dealing
with the file-level exclusions applied to a lot of other node types.
This patch doesn't make any change in the script's output now, but
it gives us flexibility for the future.

			regards, tom lane

  6. Re: Making CallContext and InlineCodeBlock less special-case-y

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-12T18:58:53Z

    On 12.07.22 01:01, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>>> On 10.07.22 01:50, Tom Lane wrote:
>>>> As committed, gen_node_support.pl excludes CallContext and InlineCodeBlock
>>>> from getting unneeded support functions via some very ad-hoc code.
> 
>>> Couldn't we just enable those support functions?  I think they were just
>>> excluded because they didn't have any before and nobody bothered to make
>>> any.
> 
>> Well, we could I suppose, but that path leads to a lot of dead code in
>> backend/nodes/ --- obviously these two alone are negligible, but I want
>> a story other than "it's a hack" for execnodes.h and the other files
>> we exclude from generation of support code.
> 
> Here's a proposed patch for this bit.  Again, whether these two
> node types have unnecessary support functions is not the point ---
> obviously we could afford to waste that much space.  Rather, what
> I'm after is to have a more explainable and flexible way of dealing
> with the file-level exclusions applied to a lot of other node types.
> This patch doesn't make any change in the script's output now, but
> it gives us flexibility for the future.

Yeah, looks reasonable.