Making CallContext and InlineCodeBlock less special-case-y

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]/messages/by-id/4159834.1657405226@sss.pgh.pa.us. 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]: /messages/by-id/4159834.1657405226@sss.pgh.pa.us

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.

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

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

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.