Thread: Making CallContext and InlineCodeBlock less special-case-y
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
diff --git a/src/backend/nodes/Makefile b/src/backend/nodes/Makefile
index 79ce0a532f..03c488304e 100644
--- a/src/backend/nodes/Makefile
+++ b/src/backend/nodes/Makefile
@@ -43,6 +43,7 @@ node_headers = \
nodes/parsenodes.h \
nodes/replnodes.h \
nodes/value.h \
+ fmgr.h \
commands/trigger.h \
commands/event_trigger.h \
foreign/fdwapi.h \
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 4a7902e6bf..eef2deb2d8 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -97,10 +97,8 @@ push @scalar_types, qw(QualCost);
# XXX various things we are not publishing right now to stay level
# with the manual system
-push @no_copy, qw(CallContext InlineCodeBlock);
-push @no_equal, qw(CallContext InlineCodeBlock);
push @no_read_write,
- qw(AccessPriv AlterTableCmd CallContext CreateOpClassItem FunctionParameter InferClause InlineCodeBlock
ObjectWithArgsOnConflictClause PartitionCmd RoleSpec VacuumRelation);
+ qw(AccessPriv AlterTableCmd CreateOpClassItem FunctionParameter InferClause ObjectWithArgs OnConflictClause
PartitionCmdRoleSpec VacuumRelation);
push @no_read, qw(A_ArrayExpr A_Indices A_Indirection AlterStatsStmt
CollateClause ColumnDef ColumnRef CreateForeignTableStmt CreateStatsStmt
CreateStmt FuncCall ImportForeignSchemaStmt IndexElem IndexStmt
@@ -298,7 +296,7 @@ foreach my $infile (@ARGV)
# Nodes from these files don't need support functions,
# just node tags.
if (elem basename($infile),
- qw(execnodes.h trigger.h event_trigger.h amapi.h tableam.h
+ qw(execnodes.h fmgr.h trigger.h event_trigger.h amapi.h tableam.h
tsmapi.h fdwapi.h tuptable.h replnodes.h supportnodes.h)
)
{
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 5314b73705..dfbf24897e 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -18,8 +18,9 @@
#ifndef FMGR_H
#define FMGR_H
-/* We don't want to include primnodes.h here, so make some stub references */
-typedef struct Node *fmNodePtr;
+#include "nodes/nodes.h"
+
+/* We don't want to include primnodes.h here, so make a stub reference */
typedef struct Aggref *fmAggrefPtr;
/* Likewise, avoid including execnodes.h here */
@@ -63,7 +64,7 @@ typedef struct FmgrInfo
unsigned char fn_stats; /* collect stats if track_functions > this */
void *fn_extra; /* extra space for use by handler */
MemoryContext fn_mcxt; /* memory context to store fn_extra in */
- fmNodePtr fn_expr; /* expression parse tree for call, or NULL */
+ Node *fn_expr; /* expression parse tree for call, or NULL */
} FmgrInfo;
/*
@@ -85,8 +86,8 @@ typedef struct FmgrInfo
typedef struct FunctionCallInfoBaseData
{
FmgrInfo *flinfo; /* ptr to lookup info used for this call */
- fmNodePtr context; /* pass info about context of call */
- fmNodePtr resultinfo; /* pass or return extra info about result */
+ Node *context; /* pass info about context of call */
+ Node *resultinfo; /* pass or return extra info about result */
Oid fncollation; /* collation for function to use */
#define FIELDNO_FUNCTIONCALLINFODATA_ISNULL 4
bool isnull; /* function must set true if result is NULL */
@@ -708,9 +709,9 @@ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *f
extern Oid fmgr_internal_function(const char *proname);
extern Oid get_fn_expr_rettype(FmgrInfo *flinfo);
extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum);
-extern Oid get_call_expr_argtype(fmNodePtr expr, int argnum);
+extern Oid get_call_expr_argtype(Node *expr, int argnum);
extern bool get_fn_expr_arg_stable(FmgrInfo *flinfo, int argnum);
-extern bool get_call_expr_arg_stable(fmNodePtr expr, int argnum);
+extern bool get_call_expr_arg_stable(Node *expr, int argnum);
extern bool get_fn_expr_variadic(FmgrInfo *flinfo);
extern bytea *get_fn_opclass_options(FmgrInfo *flinfo);
extern bool has_fn_opclass_options(FmgrInfo *flinfo);
@@ -751,6 +752,37 @@ extern void AggRegisterCallback(FunctionCallInfo fcinfo,
fmExprContextCallbackFunction func,
Datum arg);
+/*
+ * Support for DO blocks
+ *
+ * If a procedural language supports DO, it will provide a "laninline"
+ * handler function. DO invokes that function passing an InlineCodeBlock
+ * node as the single argument.
+ */
+typedef struct InlineCodeBlock
+{
+ NodeTag type;
+ char *source_text; /* source text of anonymous code block */
+ Oid langOid; /* OID of selected language */
+ bool langIsTrusted; /* trusted property of the language */
+ bool atomic; /* atomic execution context? */
+} InlineCodeBlock;
+
+/*
+ * Support for procedures
+ *
+ * Procedures are called using the same low-level mechanism as functions,
+ * but the fcinfo->context field is a CallContext node. This lets the
+ * callee know that it is being called via CALL rather than function
+ * syntax, and provides information about whether the semantic context
+ * is atomic or non-atomic.
+ */
+typedef struct CallContext
+{
+ NodeTag type;
+ bool atomic; /* atomic execution context? */
+} CallContext;
+
/*
* We allow plugin modules to hook function entry/exit. This is intended
* as support for loadable security policy modules, which may want to
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 0b6a7bb365..8011224d22 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3370,8 +3370,6 @@ typedef struct AlterFunctionStmt
/* ----------------------
* DO Statement
- *
- * DoStmt is the raw parser output, InlineCodeBlock is the execution-time API
* ----------------------
*/
typedef struct DoStmt
@@ -3380,15 +3378,6 @@ typedef struct DoStmt
List *args; /* List of DefElem nodes */
} DoStmt;
-typedef struct InlineCodeBlock
-{
- NodeTag type;
- char *source_text; /* source text of anonymous code block */
- Oid langOid; /* OID of selected language */
- bool langIsTrusted; /* trusted property of the language */
- bool atomic; /* atomic execution context */
-} InlineCodeBlock;
-
/* ----------------------
* CALL statement
*
@@ -3406,12 +3395,6 @@ typedef struct CallStmt
List *outargs; /* transformed output-argument expressions */
} CallStmt;
-typedef struct CallContext
-{
- NodeTag type;
- bool atomic;
-} CallContext;
-
/* ----------------------
* Alter Object Rename Statement
* ----------------------
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index edb93ec1c4..62c74a6da0 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -1881,7 +1881,7 @@ Datum
plperl_inline_handler(PG_FUNCTION_ARGS)
{
LOCAL_FCINFO(fake_fcinfo, 0);
- InlineCodeBlock *codeblock = (InlineCodeBlock *) PG_GETARG_POINTER(0);
+ InlineCodeBlock *codeblock = castNode(InlineCodeBlock, PG_GETARG_POINTER(0));
FmgrInfo flinfo;
plperl_proc_desc desc;
plperl_call_data *volatile save_call_data = current_call_data;
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 190d286f1c..cee393e4e0 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -315,7 +315,7 @@ Datum
plpgsql_inline_handler(PG_FUNCTION_ARGS)
{
LOCAL_FCINFO(fake_fcinfo, 0);
- InlineCodeBlock *codeblock = castNode(InlineCodeBlock, DatumGetPointer(PG_GETARG_DATUM(0)));
+ InlineCodeBlock *codeblock = castNode(InlineCodeBlock, PG_GETARG_POINTER(0));
PLpgSQL_function *func;
FmgrInfo flinfo;
EState *simple_eval_estate;
diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c
index 0bce106495..a7012c6c5b 100644
--- a/src/pl/plpython/plpy_main.c
+++ b/src/pl/plpython/plpy_main.c
@@ -265,7 +265,7 @@ Datum
plpython3_inline_handler(PG_FUNCTION_ARGS)
{
LOCAL_FCINFO(fake_fcinfo, 0);
- InlineCodeBlock *codeblock = (InlineCodeBlock *) DatumGetPointer(PG_GETARG_DATUM(0));
+ InlineCodeBlock *codeblock = castNode(InlineCodeBlock, PG_GETARG_POINTER(0));
FmgrInfo flinfo;
PLyProcedure proc;
PLyExecutionContext *exec_ctx;
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index b8b1728df7..80d90294ba 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -855,6 +855,7 @@ EOF
nodes/parsenodes.h
nodes/replnodes.h
nodes/value.h
+ fmgr.h
commands/trigger.h
commands/event_trigger.h
foreign/fdwapi.h
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
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
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 2c06609726..056530a657 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -50,6 +50,8 @@ my @no_equal;
my @no_read;
# node types we don't want read/write support for
my @no_read_write;
+# node types we don't want any support functions for, just node tags
+my @nodetag_only;
# types that are copied by straight assignment
my @scalar_types = qw(
@@ -95,7 +97,10 @@ push @scalar_types, qw(EquivalenceClass* EquivalenceMember*);
# currently not required.
push @scalar_types, qw(QualCost);
-# Nodes from these input files don't need support functions, just node tags.
+# Nodes from these input files are automatically treated as nodetag_only.
+# In the future we might add explicit pg_node_attr labeling to some of these
+# files and remove them from this list, but for now this is the path of least
+# resistance.
my @nodetag_only_files = qw(
nodes/execnodes.h
access/amapi.h
@@ -113,10 +118,8 @@ my @nodetag_only_files = qw(
# XXX various things we are not publishing right now to stay level
# with the manual system
-push @no_copy, qw(CallContext InlineCodeBlock);
-push @no_equal, qw(CallContext InlineCodeBlock);
push @no_read_write,
- qw(AccessPriv AlterTableCmd CallContext CreateOpClassItem FunctionParameter InferClause InlineCodeBlock
ObjectWithArgsOnConflictClause PartitionCmd RoleSpec VacuumRelation);
+ qw(AccessPriv AlterTableCmd CreateOpClassItem FunctionParameter InferClause ObjectWithArgs OnConflictClause
PartitionCmdRoleSpec VacuumRelation);
push @no_read, qw(A_ArrayExpr A_Indices A_Indirection AlterStatsStmt
CollateClause ColumnDef ColumnRef CreateForeignTableStmt CreateStatsStmt
CreateStmt FuncCall ImportForeignSchemaStmt IndexElem IndexStmt
@@ -254,6 +257,10 @@ foreach my $infile (@ARGV)
{
push @no_read, $in_struct;
}
+ elsif ($attr eq 'nodetag_only')
+ {
+ push @nodetag_only, $in_struct;
+ }
elsif ($attr eq 'special_read_write')
{
# This attribute is called
@@ -314,13 +321,9 @@ foreach my $infile (@ARGV)
$node_type_info{$in_struct}->{field_types} = \%ft;
$node_type_info{$in_struct}->{field_attrs} = \%fa;
- # Exclude nodes in nodetag_only_files from support.
- if (elem $infile, @nodetag_only_files)
- {
- push @no_copy, $in_struct;
- push @no_equal, $in_struct;
- push @no_read_write, $in_struct;
- }
+ # Propagate nodetag_only marking from files to nodes
+ push @nodetag_only, $in_struct
+ if (elem $infile, @nodetag_only_files);
# Propagate some node attributes from supertypes
if ($supertype)
@@ -515,6 +518,7 @@ print $eff $node_includes;
foreach my $n (@node_types)
{
next if elem $n, @abstract_types;
+ next if elem $n, @nodetag_only;
my $struct_no_copy = (elem $n, @no_copy);
my $struct_no_equal = (elem $n, @no_equal);
next if $struct_no_copy && $struct_no_equal;
@@ -706,6 +710,7 @@ print $rff $node_includes;
foreach my $n (@node_types)
{
next if elem $n, @abstract_types;
+ next if elem $n, @nodetag_only;
next if elem $n, @no_read_write;
# XXX For now, skip all "Stmt"s except that ones that were there before.
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index adc549002a..c8ed4e0552 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -61,12 +61,17 @@ typedef enum NodeTag
*
* - no_read: Does not support nodeRead() at all.
*
+ * - nodetag_only: Does not support copyObject(), equal(), outNode(),
+ * or nodeRead().
+ *
* - special_read_write: Has special treatment in outNode() and nodeRead().
*
* Node types can be supertypes of other types whether or not they are marked
* abstract: if a node struct appears as the first field of another struct
* type, then it is the supertype of that type. The no_copy, no_equal, and
* no_read node attributes are automatically inherited from the supertype.
+ * (Notice that nodetag_only does not inherit, so it's not quite equivalent
+ * to a combination of other attributes.)
*
* Valid node field attributes:
*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 0b6a7bb365..e2ad761768 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3382,6 +3382,8 @@ typedef struct DoStmt
typedef struct InlineCodeBlock
{
+ pg_node_attr(nodetag_only) /* this is not a member of parse trees */
+
NodeTag type;
char *source_text; /* source text of anonymous code block */
Oid langOid; /* OID of selected language */
@@ -3408,6 +3410,8 @@ typedef struct CallStmt
typedef struct CallContext
{
+ pg_node_attr(nodetag_only) /* this is not a member of parse trees */
+
NodeTag type;
bool atomic;
} CallContext;
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.