Обсуждение: Convert planner's AggInfo and AggTransInfo to Nodes
I got annoyed just now upon finding that pprint() applied to the planner's
"root" pointer doesn't dump root->agginfos or root->aggtransinfos. That's
evidently because AggInfo and AggTransInfo aren't proper Nodes, just bare
structs, which presumably is because somebody couldn't be bothered to
write outfuncs support for them. I'd say that was a questionable shortcut
even when it was made, and there's certainly precious little excuse now
that gen_node_support.pl can do all the heavy lifting. Hence, PFA a
little finger exercise to turn them into Nodes. I took the opportunity
to improve related comments too, and in particular to fix some comments
that leave the impression that preprocess_minmax_aggregates still does
its own scan of the query tree. (It was momentary confusion over that
idea that got me to the point of being annoyed in the first place.)
Any objections so far?
I'm kind of tempted to mount an effort to get rid of as many of
pathnodes.h's "read_write_ignore" annotations as possible. Some are
necessary to prevent infinite recursion, and others represent considered
judgments that they'd bloat node dumps more than they're worth --- but
I think quite a lot of them arose from plain laziness about updating
outfuncs.c. With the infrastructure we have now, that's no longer
a good reason.
In particular, I'm tempted to make a dump of PlannerInfo include
all the baserel RelOptInfos (not joins though; there could be a
mighty lot of those.) I think we didn't print the simple_rel_array[]
array before mostly because outfuncs didn't use to have reasonable
support for printing arrays.
Thoughts?
regards, tom lane
diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index 9330908cbf..0f5d8fd978 100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -137,8 +137,8 @@ preprocess_minmax_aggregates(PlannerInfo *root)
return;
/*
- * Scan the tlist and HAVING qual to find all the aggregates and verify
- * all are MIN/MAX aggregates. Stop as soon as we find one that isn't.
+ * Examine all the aggregates and verify all are MIN/MAX aggregates. Stop
+ * as soon as we find one that isn't.
*/
aggs_list = NIL;
if (!can_minmax_aggs(root, &aggs_list))
@@ -227,21 +227,21 @@ preprocess_minmax_aggregates(PlannerInfo *root)
/*
* can_minmax_aggs
- * Walk through all the aggregates in the query, and check
- * if they are all MIN/MAX aggregates. If so, build a list of the
- * distinct aggregate calls in the tree.
+ * Examine all the aggregates in the query, and check if they are
+ * all MIN/MAX aggregates. If so, build a list of MinMaxAggInfo
+ * nodes for them.
*
* Returns false if a non-MIN/MAX aggregate is found, true otherwise.
- *
- * This does not descend into subqueries, and so should be used only after
- * reduction of sublinks to subplans. There mustn't be outer-aggregate
- * references either.
*/
static bool
can_minmax_aggs(PlannerInfo *root, List **context)
{
ListCell *lc;
+ /*
+ * This function used to have to scan the query for itself, but now we can
+ * just thumb through the AggInfo list made by preprocess_aggrefs.
+ */
foreach(lc, root->agginfos)
{
AggInfo *agginfo = (AggInfo *) lfirst(lc);
diff --git a/src/backend/optimizer/prep/prepagg.c b/src/backend/optimizer/prep/prepagg.c
index 404a5f1dac..8f46111186 100644
--- a/src/backend/optimizer/prep/prepagg.c
+++ b/src/backend/optimizer/prep/prepagg.c
@@ -229,7 +229,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
}
else
{
- AggInfo *agginfo = palloc(sizeof(AggInfo));
+ AggInfo *agginfo = makeNode(AggInfo);
agginfo->finalfn_oid = aggfinalfn;
agginfo->representative_aggref = aggref;
@@ -266,7 +266,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
same_input_transnos);
if (transno == -1)
{
- AggTransInfo *transinfo = palloc(sizeof(AggTransInfo));
+ AggTransInfo *transinfo = makeNode(AggTransInfo);
transinfo->args = aggref->args;
transinfo->aggfilter = aggref->aggfilter;
@@ -452,7 +452,8 @@ find_compatible_trans(PlannerInfo *root, Aggref *newagg, bool shareable,
foreach(lc, transnos)
{
int transno = lfirst_int(lc);
- AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos, transno);
+ AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos,
+ transno);
/*
* if the transfns or transition state types are not the same then the
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 69ba254372..e650af5ff2 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -442,15 +442,15 @@ struct PlannerInfo
* Information about aggregates. Filled by preprocess_aggrefs().
*/
/* AggInfo structs */
- List *agginfos pg_node_attr(read_write_ignore);
+ List *agginfos;
/* AggTransInfo structs */
- List *aggtransinfos pg_node_attr(read_write_ignore);
- /* number w/ DISTINCT/ORDER BY/WITHIN GROUP */
- int numOrderedAggs pg_node_attr(read_write_ignore);
+ List *aggtransinfos;
+ /* number of aggs with DISTINCT/ORDER BY/WITHIN GROUP */
+ int numOrderedAggs;
/* does any agg not support partial mode? */
- bool hasNonPartialAggs pg_node_attr(read_write_ignore);
+ bool hasNonPartialAggs;
/* is any partial agg non-serializable? */
- bool hasNonSerialAggs pg_node_attr(read_write_ignore);
+ bool hasNonSerialAggs;
/*
* These fields are used only when hasRecursion is true:
@@ -3121,6 +3121,10 @@ typedef struct JoinCostWorkspace
*/
typedef struct AggInfo
{
+ pg_node_attr(no_copy_equal, no_read)
+
+ NodeTag type;
+
/*
* Link to an Aggref expr this state value is for.
*
@@ -3129,6 +3133,7 @@ typedef struct AggInfo
*/
Aggref *representative_aggref;
+ /* Transition state number for this aggregate */
int transno;
/*
@@ -3137,9 +3142,8 @@ typedef struct AggInfo
*/
bool shareable;
- /* Oid of the final function or InvalidOid */
+ /* Oid of the final function, or InvalidOid if none */
Oid finalfn_oid;
-
} AggInfo;
/*
@@ -3151,34 +3155,40 @@ typedef struct AggInfo
*/
typedef struct AggTransInfo
{
+ pg_node_attr(no_copy_equal, no_read)
+
+ NodeTag type;
+
+ /* Inputs for this transition state */
List *args;
Expr *aggfilter;
/* Oid of the state transition function */
Oid transfn_oid;
- /* Oid of the serialization function or InvalidOid */
+ /* Oid of the serialization function, or InvalidOid if none */
Oid serialfn_oid;
- /* Oid of the deserialization function or InvalidOid */
+ /* Oid of the deserialization function, or InvalidOid if none */
Oid deserialfn_oid;
- /* Oid of the combine function or InvalidOid */
+ /* Oid of the combine function, or InvalidOid if none */
Oid combinefn_oid;
/* Oid of state value's datatype */
Oid aggtranstype;
+
+ /* Additional data about transtype */
int32 aggtranstypmod;
int transtypeLen;
bool transtypeByVal;
+
+ /* Space-consumption estimate */
int32 aggtransspace;
- /*
- * initial value from pg_aggregate entry
- */
- Datum initValue;
+ /* Initial value from pg_aggregate entry */
+ Datum initValue pg_node_attr(read_write_ignore);
bool initValueIsNull;
-
} AggTransInfo;
#endif /* PATHNODES_H */
Tom Lane <tgl@sss.pgh.pa.us> writes:
> I got annoyed just now upon finding that pprint() applied to the planner's
> "root" pointer doesn't dump root->agginfos or root->aggtransinfos. That's
> evidently because AggInfo and AggTransInfo aren't proper Nodes, just bare
> structs, which presumably is because somebody couldn't be bothered to
> write outfuncs support for them. I'd say that was a questionable shortcut
> even when it was made, and there's certainly precious little excuse now
> that gen_node_support.pl can do all the heavy lifting. Hence, PFA a
> little finger exercise to turn them into Nodes. I took the opportunity
> to improve related comments too, and in particular to fix some comments
> that leave the impression that preprocess_minmax_aggregates still does
> its own scan of the query tree. (It was momentary confusion over that
> idea that got me to the point of being annoyed in the first place.)
>
> Any objections so far?
It seems like a reasonable idea, but I don't know enough to judge the
wider ramifications of it. But one thing that the patch should also do,
is switch to using the l*_node() functions instead of manual casting.
The ones I noticed in the patch/context are below, but there are a few
more:
> foreach(lc, root->agginfos)
> {
> AggInfo *agginfo = (AggInfo *) lfirst(lc);
AggInfo *agginfo = lfirst_node(AggInfo, lc);
[…]
> foreach(lc, transnos)
> {
> int transno = lfirst_int(lc);
> - AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos, transno);
> + AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos,
> + transno);
> + AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos,
> + transno);
AggTransInfo *pertrans = list_nth_node(AggTransInfo, root->aggtransinfos,
transno);
- ilmari
On 18.07.22 18:08, Tom Lane wrote: > I'm kind of tempted to mount an effort to get rid of as many of > pathnodes.h's "read_write_ignore" annotations as possible. Some are > necessary to prevent infinite recursion, and others represent considered > judgments that they'd bloat node dumps more than they're worth --- but > I think quite a lot of them arose from plain laziness about updating > outfuncs.c. With the infrastructure we have now, that's no longer > a good reason. That was my impression as well, and I agree it would be good to sort that out.
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> It seems like a reasonable idea, but I don't know enough to judge the
> wider ramifications of it. But one thing that the patch should also do,
> is switch to using the l*_node() functions instead of manual casting.
Hm, I didn't bother with that on the grounds that there's no question
what should be in those two lists. But I guess it's not much extra
code, so pushed that way.
regards, tom lane
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 18.07.22 18:08, Tom Lane wrote:
>> I'm kind of tempted to mount an effort to get rid of as many of
>> pathnodes.h's "read_write_ignore" annotations as possible. Some are
>> necessary to prevent infinite recursion, and others represent considered
>> judgments that they'd bloat node dumps more than they're worth --- but
>> I think quite a lot of them arose from plain laziness about updating
>> outfuncs.c. With the infrastructure we have now, that's no longer
>> a good reason.
> That was my impression as well, and I agree it would be good to sort
> that out.
I had a go at doing this, and ended up with something that seems
reasonable for now (attached). The thing that'd have to be done to
make additional progress is to convert a lot of partitioning-related
structs into full Nodes. That seems like it might possibly be
worth doing, but I don't feel like doing it. I doubt that making
planner node dumps smarter is a sufficient excuse for that anyway.
(But possibly if we then larded related code with castNode() and
sibling macros, there'd be enough of a gain in type-safety to
justify it?)
I learned a couple of interesting things along the way:
* I'd thought we already had outfuncs support for writing an array
of node pointers. We don't, but it's easily added. I chose to
write the array with parenthesis decoration, mainly because that
eases moving around it in emacs.
* WRITE_OID_ARRAY and WRITE_BOOL_ARRAY needed extension to handle a null
array pointer. I think we should make all the WRITE_FOO_ARRAY macros
work alike, so I added that to all of them. I first tried to make the
rest work like WRITE_INDEX_ARRAY, but that failed because readfuncs.c
isn't expecting "<>" for an empty array; it's expecting nothing at
all. (Note there is no readfuncs equivalent to WRITE_INDEX_ARRAY.)
What I've done here is to change WRITE_INDEX_ARRAY to work like the
others and print nothing for an empty array, but I wonder if now
wouldn't be a good time to redefine the serialized representation
to be more robust. I'm imagining "<>" for a NULL array pointer and
"(item item item)" otherwise, allowing a cross-check that we're
getting the right number of items.
* gen_node_support.pl was being insufficiently careful about parsing
type names, so I tightened its regexes a bit.
regards, tom lane
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index f3309c3000..bee48696c7 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -441,6 +441,8 @@ foreach my $infile (@ARGV)
$type =~ s/\s*$//;
# strip space between type and "*" (pointer) */
$type =~ s/\s+\*$/*/;
+ # strip space between type and "**" (array of pointers) */
+ $type =~ s/\s+\*\*$/**/;
die
"$infile:$lineno: cannot parse data type in \"$line\"\n"
@@ -745,8 +747,8 @@ _equal${n}(const $n *a, const $n *b)
unless $equal_ignore || $t eq 'CoercionForm';
}
}
- # scalar type pointer
- elsif ($t =~ /(\w+)\*/ and elem $1, @scalar_types)
+ # arrays of scalar types
+ elsif ($t =~ /^(\w+)\*$/ and elem $1, @scalar_types)
{
my $tt = $1;
if (!defined $array_size_field)
@@ -780,13 +782,14 @@ _equal${n}(const $n *a, const $n *b)
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
# node type
- elsif ($t =~ /(\w+)\*/ and elem $1, @node_types)
+ elsif (($t =~ /^(\w+)\*$/ or $t =~ /^struct\s+(\w+)\*$/)
+ and elem $1, @node_types)
{
print $cff "\tCOPY_NODE_FIELD($f);\n" unless $copy_ignore;
print $eff "\tCOMPARE_NODE_FIELD($f);\n" unless $equal_ignore;
}
# array (inline)
- elsif ($t =~ /\w+\[/)
+ elsif ($t =~ /^\w+\[\w+\]$/)
{
print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore;
print $eff "\tCOMPARE_ARRAY_FIELD($f);\n" unless $equal_ignore;
@@ -894,11 +897,16 @@ _read${n}(void)
my @a = @{ $node_type_info{$n}->{field_attrs}{$f} };
# extract per-field attributes
- my $read_write_ignore = 0;
+ my $array_size_field;
my $read_as_field;
+ my $read_write_ignore = 0;
foreach my $a (@a)
{
- if ($a =~ /^read_as\(([\w.]+)\)$/)
+ if ($a =~ /^array_size\(([\w.]+)\)$/)
+ {
+ $array_size_field = $1;
+ }
+ elsif ($a =~ /^read_as\(([\w.]+)\)$/)
{
$read_as_field = $1;
}
@@ -1015,19 +1023,10 @@ _read${n}(void)
print $off "\tWRITE_ENUM_FIELD($f, $t);\n";
print $rff "\tREAD_ENUM_FIELD($f, $t);\n" unless $no_read;
}
- # arrays
- elsif ($t =~ /(\w+)(\*|\[)/ and elem $1, @scalar_types)
+ # arrays of scalar types
+ elsif ($t =~ /^(\w+)(\*|\[\w+\])$/ and elem $1, @scalar_types)
{
my $tt = uc $1;
- my $array_size_field;
- foreach my $a (@a)
- {
- if ($a =~ /^array_size\(([\w.]+)\)$/)
- {
- $array_size_field = $1;
- last;
- }
- }
if (!defined $array_size_field)
{
die "no array size defined for $n.$f of type $t\n";
@@ -1080,11 +1079,38 @@ _read${n}(void)
. "\t\toutBitmapset(str, NULL);\n";
}
# node type
- elsif ($t =~ /(\w+)\*/ and elem $1, @node_types)
+ elsif (($t =~ /^(\w+)\*$/ or $t =~ /^struct\s+(\w+)\*$/)
+ and elem $1, @node_types)
{
print $off "\tWRITE_NODE_FIELD($f);\n";
print $rff "\tREAD_NODE_FIELD($f);\n" unless $no_read;
}
+ # arrays of node pointers (currently supported for write only)
+ elsif (($t =~ /^(\w+)\*\*$/ or $t =~ /^struct\s+(\w+)\*\*$/)
+ and elem $1, @node_types)
+ {
+ if (!defined $array_size_field)
+ {
+ die "no array size defined for $n.$f of type $t\n";
+ }
+ if ($node_type_info{$n}->{field_types}{$array_size_field} eq
+ 'List*')
+ {
+ print $off
+ "\tWRITE_NODE_ARRAY($f, list_length(node->$array_size_field));\n";
+ print $rff
+ "\tREAD_NODE_ARRAY($f, list_length(local_node->$array_size_field));\n"
+ unless $no_read;
+ }
+ else
+ {
+ print $off
+ "\tWRITE_NODE_ARRAY($f, node->$array_size_field);\n";
+ print $rff
+ "\tREAD_NODE_ARRAY($f, local_node->$array_size_field);\n"
+ unless $no_read;
+ }
+ }
elsif ($t eq 'struct CustomPathMethods*'
|| $t eq 'struct CustomScanMethods*')
{
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2b85f97f39..01d70a75e4 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -96,49 +96,63 @@ static void outChar(StringInfo str, char c);
(appendStringInfoString(str, " :" CppAsString(fldname) " "), \
outBitmapset(str, node->fldname))
+#define WRITE_NODE_ARRAY(fldname, len) \
+ do { \
+ appendStringInfoString(str, " :" CppAsString(fldname) " "); \
+ if (node->fldname) \
+ { \
+ appendStringInfoChar(str, '('); \
+ for (int i = 0; i < len; i++) \
+ { \
+ appendStringInfoChar(str, ' '); \
+ outNode(str, node->fldname[i]); \
+ } \
+ appendStringInfoChar(str, ')'); \
+ } \
+ else \
+ appendStringInfoString(str, "<>"); \
+ } while(0)
+
#define WRITE_ATTRNUMBER_ARRAY(fldname, len) \
do { \
appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %d", node->fldname[i]); \
+ if (node->fldname) \
+ for (int i = 0; i < len; i++) \
+ appendStringInfo(str, " %d", node->fldname[i]); \
} while(0)
#define WRITE_OID_ARRAY(fldname, len) \
do { \
appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %u", node->fldname[i]); \
+ if (node->fldname) \
+ for (int i = 0; i < len; i++) \
+ appendStringInfo(str, " %u", node->fldname[i]); \
} while(0)
-/*
- * This macro supports the case that the field is NULL. For the other array
- * macros, that is currently not needed.
- */
#define WRITE_INDEX_ARRAY(fldname, len) \
do { \
appendStringInfoString(str, " :" CppAsString(fldname) " "); \
if (node->fldname) \
for (int i = 0; i < len; i++) \
appendStringInfo(str, " %u", node->fldname[i]); \
- else \
- appendStringInfoString(str, "<>"); \
} while(0)
#define WRITE_INT_ARRAY(fldname, len) \
do { \
appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %d", node->fldname[i]); \
+ if (node->fldname) \
+ for (int i = 0; i < len; i++) \
+ appendStringInfo(str, " %d", node->fldname[i]); \
} while(0)
#define WRITE_BOOL_ARRAY(fldname, len) \
do { \
appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %s", booltostr(node->fldname[i])); \
+ if (node->fldname) \
+ for (int i = 0; i < len; i++) \
+ appendStringInfo(str, " %s", booltostr(node->fldname[i])); \
} while(0)
-
#define booltostr(x) ((x) ? "true" : "false")
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index e650af5ff2..1f95e46a58 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -89,7 +89,7 @@ typedef enum UpperRelationKind
* planned.
*
* Not all fields are printed. (In some cases, there is no print support for
- * the field type.)
+ * the field type; in others, doing so would lead to infinite recursion.)
*----------
*/
typedef struct PlannerGlobal
@@ -177,7 +177,8 @@ typedef struct PlannerGlobal
* either here or in that header, whichever is read first.
*
* Not all fields are printed. (In some cases, there is no print support for
- * the field type.)
+ * the field type; in others, doing so would lead to infinite recursion or
+ * bloat dump output more than seems useful.)
*----------
*/
#ifndef HAVE_PLANNERINFO_TYPEDEF
@@ -220,14 +221,15 @@ struct PlannerInfo
* does not correspond to a base relation, such as a join RTE or an
* unreferenced view RTE; or if the RelOptInfo hasn't been made yet.
*/
- struct RelOptInfo **simple_rel_array pg_node_attr(read_write_ignore);
+ struct RelOptInfo **simple_rel_array pg_node_attr(array_size(simple_rel_array_size));
/* allocated size of array */
- int simple_rel_array_size pg_node_attr(read_write_ignore);
+ int simple_rel_array_size;
/*
* simple_rte_array is the same length as simple_rel_array and holds
* pointers to the associated rangetable entries. Using this is a shade
- * faster than using rt_fetch(), mostly due to fewer indirections.
+ * faster than using rt_fetch(), mostly due to fewer indirections. (Not
+ * printed because it'd be redundant with parse->rtable.)
*/
RangeTblEntry **simple_rte_array pg_node_attr(read_write_ignore);
@@ -237,7 +239,7 @@ struct PlannerInfo
* child_relid, or NULL if the rel is not an appendrel child. The array
* itself is not allocated if append_rel_list is empty.
*/
- struct AppendRelInfo **append_rel_array pg_node_attr(read_write_ignore);
+ struct AppendRelInfo **append_rel_array pg_node_attr(array_size(simple_rel_array_size));
/*
* all_baserels is a Relids set of all base relids (but not "other"
@@ -274,7 +276,7 @@ struct PlannerInfo
* automatically added to the join_rel_level[join_cur_level] list.
* join_rel_level is NULL if not in use.
*/
- /* lists of join-relation RelOptInfos */
+ /* lists of join-relation RelOptInfos (too bulky to print) */
List **join_rel_level pg_node_attr(read_write_ignore);
/* index of list being extended */
int join_cur_level;
@@ -403,8 +405,8 @@ struct PlannerInfo
/*
* Fields filled during create_plan() for use in setrefs.c
*/
- /* for GroupingFunc fixup */
- AttrNumber *grouping_map pg_node_attr(array_size(update_colnos), read_write_ignore);
+ /* for GroupingFunc fixup (can't print: array length not known here) */
+ AttrNumber *grouping_map pg_node_attr(read_write_ignore);
/* List of MinMaxAggInfos */
List *minmax_aggs;
@@ -458,7 +460,7 @@ struct PlannerInfo
/* PARAM_EXEC ID for the work table */
int wt_param_id;
/* a path for non-recursive term */
- struct Path *non_recursive_path pg_node_attr(read_write_ignore);
+ struct Path *non_recursive_path;
/*
* These fields are workspace for createplan.c
@@ -470,7 +472,9 @@ struct PlannerInfo
/*
* These fields are workspace for setrefs.c. Each is an array
- * corresponding to glob->subplans.
+ * corresponding to glob->subplans. (We could probably teach
+ * gen_node_support.pl how to determine the array length, but it doesn't
+ * seem worth the trouble, so just mark them read_write_ignore.)
*/
bool *isAltSubplan pg_node_attr(read_write_ignore);
bool *isUsedSubplan pg_node_attr(read_write_ignore);
@@ -928,16 +932,17 @@ typedef struct RelOptInfo
* Number of partitions; -1 if not yet set; in case of a join relation 0
* means it's considered unpartitioned
*/
- int nparts pg_node_attr(read_write_ignore);
+ int nparts;
/* Partition bounds */
struct PartitionBoundInfoData *boundinfo pg_node_attr(read_write_ignore);
/* True if partition bounds were created by partition_bounds_merge() */
bool partbounds_merged;
/* Partition constraint, if not the root */
- List *partition_qual pg_node_attr(read_write_ignore);
+ List *partition_qual;
/*
* Array of RelOptInfos of partitions, stored in the same order as bounds
+ * (don't print, too bulky)
*/
struct RelOptInfo **part_rels pg_node_attr(read_write_ignore);
@@ -948,6 +953,12 @@ typedef struct RelOptInfo
Bitmapset *live_parts;
/* Relids set of all partition relids */
Relids all_partrels;
+
+ /*
+ * These arrays are of length partkey->partnatts, which we don't have at
+ * hand, so don't try to print
+ */
+
/* Non-nullable partition key expressions */
List **partexprs pg_node_attr(read_write_ignore);
/* Nullable partition key expressions */
@@ -1042,30 +1053,26 @@ struct IndexOptInfo
int nkeycolumns;
/*
- * array fields aren't really worth the trouble to print
+ * table column numbers of index's columns (both key and included
+ * columns), or 0 for expression columns
*/
-
- /*
- * column numbers of index's attributes both key and included columns, or
- * 0
- */
- int *indexkeys pg_node_attr(read_write_ignore);
+ int *indexkeys pg_node_attr(array_size(ncolumns));
/* OIDs of collations of index columns */
- Oid *indexcollations pg_node_attr(read_write_ignore);
+ Oid *indexcollations pg_node_attr(array_size(nkeycolumns));
/* OIDs of operator families for columns */
- Oid *opfamily pg_node_attr(read_write_ignore);
+ Oid *opfamily pg_node_attr(array_size(nkeycolumns));
/* OIDs of opclass declared input data types */
- Oid *opcintype pg_node_attr(read_write_ignore);
+ Oid *opcintype pg_node_attr(array_size(nkeycolumns));
/* OIDs of btree opfamilies, if orderable */
- Oid *sortopfamily pg_node_attr(read_write_ignore);
+ Oid *sortopfamily pg_node_attr(array_size(nkeycolumns));
/* is sort order descending? */
- bool *reverse_sort pg_node_attr(read_write_ignore);
+ bool *reverse_sort pg_node_attr(array_size(nkeycolumns));
/* do NULLs come first in the sort order? */
- bool *nulls_first pg_node_attr(read_write_ignore);
+ bool *nulls_first pg_node_attr(array_size(nkeycolumns));
/* opclass-specific options for columns */
bytea **opclassoptions pg_node_attr(read_write_ignore);
/* which index cols can be returned in an index-only scan? */
- bool *canreturn pg_node_attr(read_write_ignore);
+ bool *canreturn pg_node_attr(array_size(ncolumns));
/* OID of the access method (in pg_am) */
Oid relam;
@@ -1098,19 +1105,19 @@ struct IndexOptInfo
/*
* Remaining fields are copied from the index AM's API struct
- * (IndexAmRoutine). We don't bother to dump them.
+ * (IndexAmRoutine).
*/
- bool amcanorderbyop pg_node_attr(read_write_ignore);
- bool amoptionalkey pg_node_attr(read_write_ignore);
- bool amsearcharray pg_node_attr(read_write_ignore);
- bool amsearchnulls pg_node_attr(read_write_ignore);
+ bool amcanorderbyop;
+ bool amoptionalkey;
+ bool amsearcharray;
+ bool amsearchnulls;
/* does AM have amgettuple interface? */
- bool amhasgettuple pg_node_attr(read_write_ignore);
+ bool amhasgettuple;
/* does AM have amgetbitmap interface? */
- bool amhasgetbitmap pg_node_attr(read_write_ignore);
- bool amcanparallel pg_node_attr(read_write_ignore);
+ bool amhasgetbitmap;
+ bool amcanparallel;
/* does AM have ammarkpos interface? */
- bool amcanmarkpos pg_node_attr(read_write_ignore);
+ bool amcanmarkpos;
/* AM's cost estimator */
/* Rather than include amapi.h here, we declare amcostestimate like this */
void (*amcostestimate) () pg_node_attr(read_write_ignore);
@@ -1184,12 +1191,9 @@ typedef struct StatisticExtInfo
Oid statOid;
/* includes child relations */
- bool inherit pg_node_attr(read_write_ignore);
+ bool inherit;
- /*
- * back-link to statistic's table; don't print, infinite recursion on plan
- * tree dump
- */
+ /* back-link to statistic's table; don't print, else infinite recursion */
RelOptInfo *rel pg_node_attr(read_write_ignore);
/* statistics kind of this entry */
I wrote:
> * WRITE_OID_ARRAY and WRITE_BOOL_ARRAY needed extension to handle a null
> array pointer. I think we should make all the WRITE_FOO_ARRAY macros
> work alike, so I added that to all of them. I first tried to make the
> rest work like WRITE_INDEX_ARRAY, but that failed because readfuncs.c
> isn't expecting "<>" for an empty array; it's expecting nothing at
> all. (Note there is no readfuncs equivalent to WRITE_INDEX_ARRAY.)
> What I've done here is to change WRITE_INDEX_ARRAY to work like the
> others and print nothing for an empty array, but I wonder if now
> wouldn't be a good time to redefine the serialized representation
> to be more robust. I'm imagining "<>" for a NULL array pointer and
> "(item item item)" otherwise, allowing a cross-check that we're
> getting the right number of items.
Concretely, about like this.
regards, tom lane
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2b85f97f39..fe97d559b6 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -16,6 +16,7 @@
#include <ctype.h>
+#include "access/attnum.h"
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "nodes/bitmapset.h"
@@ -96,48 +97,30 @@ static void outChar(StringInfo str, char c);
(appendStringInfoString(str, " :" CppAsString(fldname) " "), \
outBitmapset(str, node->fldname))
+/* Write a variable-length array of AttrNumber */
#define WRITE_ATTRNUMBER_ARRAY(fldname, len) \
- do { \
- appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %d", node->fldname[i]); \
- } while(0)
+ (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ writeAttrNumberCols(str, node->fldname, len))
+/* Write a variable-length array of Oid */
#define WRITE_OID_ARRAY(fldname, len) \
- do { \
- appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %u", node->fldname[i]); \
- } while(0)
+ (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ writeOidCols(str, node->fldname, len))
-/*
- * This macro supports the case that the field is NULL. For the other array
- * macros, that is currently not needed.
- */
+/* Write a variable-length array of Index */
#define WRITE_INDEX_ARRAY(fldname, len) \
- do { \
- appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- if (node->fldname) \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %u", node->fldname[i]); \
- else \
- appendStringInfoString(str, "<>"); \
- } while(0)
+ (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ writeIndexCols(str, node->fldname, len))
+/* Write a variable-length array of int */
#define WRITE_INT_ARRAY(fldname, len) \
- do { \
- appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %d", node->fldname[i]); \
- } while(0)
+ (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ writeIntCols(str, node->fldname, len))
+/* Write a variable-length array of bool */
#define WRITE_BOOL_ARRAY(fldname, len) \
- do { \
- appendStringInfoString(str, " :" CppAsString(fldname) " "); \
- for (int i = 0; i < len; i++) \
- appendStringInfo(str, " %s", booltostr(node->fldname[i])); \
- } while(0)
-
+ (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ writeBoolCols(str, node->fldname, len))
#define booltostr(x) ((x) ? "true" : "false")
@@ -196,6 +179,37 @@ outChar(StringInfo str, char c)
outToken(str, in);
}
+/*
+ * common implementation for scalar-array-writing functions
+ *
+ * The data format is either "<>" for a NULL pointer or "(item item item)".
+ * fmtstr must include a leading space.
+ * convfunc can be empty, or the name of a conversion macro or function.
+ */
+#define WRITE_SCALAR_ARRAY(fnname, datatype, fmtstr, convfunc) \
+static void \
+fnname(StringInfo str, const datatype *arr, int len) \
+{ \
+ if (arr != NULL) \
+ { \
+ appendStringInfoChar(str, '('); \
+ for (int i = 0; i < len; i++) \
+ appendStringInfo(str, fmtstr, convfunc(arr[i])); \
+ appendStringInfoChar(str, ')'); \
+ } \
+ else \
+ appendStringInfoString(str, "<>"); \
+}
+
+WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",)
+WRITE_SCALAR_ARRAY(writeOidCols, Oid, " %u",)
+WRITE_SCALAR_ARRAY(writeIndexCols, Index, " %u",)
+WRITE_SCALAR_ARRAY(writeIntCols, int, " %d",)
+WRITE_SCALAR_ARRAY(writeBoolCols, bool, " %s", booltostr)
+
+/*
+ * Print a List.
+ */
static void
_outList(StringInfo str, const List *node)
{
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index a2391280be..c77c3984ca 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -502,97 +502,45 @@ readDatum(bool typbyval)
}
/*
- * readAttrNumberCols
- */
-AttrNumber *
-readAttrNumberCols(int numCols)
-{
- int tokenLength,
- i;
- const char *token;
- AttrNumber *attr_vals;
-
- if (numCols <= 0)
- return NULL;
-
- attr_vals = (AttrNumber *) palloc(numCols * sizeof(AttrNumber));
- for (i = 0; i < numCols; i++)
- {
- token = pg_strtok(&tokenLength);
- attr_vals[i] = atoi(token);
- }
-
- return attr_vals;
-}
-
-/*
- * readOidCols
- */
-Oid *
-readOidCols(int numCols)
-{
- int tokenLength,
- i;
- const char *token;
- Oid *oid_vals;
-
- if (numCols <= 0)
- return NULL;
-
- oid_vals = (Oid *) palloc(numCols * sizeof(Oid));
- for (i = 0; i < numCols; i++)
- {
- token = pg_strtok(&tokenLength);
- oid_vals[i] = atooid(token);
- }
-
- return oid_vals;
-}
-
-/*
- * readIntCols
+ * common implementation for scalar-array-reading functions
+ *
+ * The data format is either "<>" for a NULL pointer (in which case numCols
+ * is ignored) or "(item item item)" where the number of items must equal
+ * numCols. The convfunc must be okay with stopping at whitespace or a
+ * right parenthesis, since pg_strtok won't null-terminate the token.
*/
-int *
-readIntCols(int numCols)
-{
- int tokenLength,
- i;
- const char *token;
- int *int_vals;
-
- if (numCols <= 0)
- return NULL;
-
- int_vals = (int *) palloc(numCols * sizeof(int));
- for (i = 0; i < numCols; i++)
- {
- token = pg_strtok(&tokenLength);
- int_vals[i] = atoi(token);
- }
-
- return int_vals;
+#define READ_SCALAR_ARRAY(fnname, datatype, convfunc) \
+datatype * \
+fnname(int numCols) \
+{ \
+ datatype *vals; \
+ READ_TEMP_LOCALS(); \
+ token = pg_strtok(&length); \
+ if (token == NULL) \
+ elog(ERROR, "incomplete scalar array"); \
+ if (length == 0) \
+ return NULL; /* it was "<>", so return NULL pointer */ \
+ if (length != 1 || token[0] != '(') \
+ elog(ERROR, "unrecognized token: \"%.*s\"", length, token); \
+ vals = (datatype *) palloc(numCols * sizeof(datatype)); \
+ for (int i = 0; i < numCols; i++) \
+ { \
+ token = pg_strtok(&length); \
+ if (token == NULL) \
+ elog(ERROR, "incomplete scalar array"); \
+ vals[i] = convfunc(token); \
+ } \
+ token = pg_strtok(&length); \
+ if (token == NULL || length != 1 || token[0] != ')') \
+ elog(ERROR, "incomplete scalar array"); \
+ return vals; \
}
/*
- * readBoolCols
+ * Note: these functions are exported in nodes.h for possible use by
+ * extensions, so don't mess too much with their names or API.
*/
-bool *
-readBoolCols(int numCols)
-{
- int tokenLength,
- i;
- const char *token;
- bool *bool_vals;
-
- if (numCols <= 0)
- return NULL;
-
- bool_vals = (bool *) palloc(numCols * sizeof(bool));
- for (i = 0; i < numCols; i++)
- {
- token = pg_strtok(&tokenLength);
- bool_vals[i] = strtobool(token);
- }
-
- return bool_vals;
-}
+READ_SCALAR_ARRAY(readAttrNumberCols, int16, atoi)
+READ_SCALAR_ARRAY(readOidCols, Oid, atooid)
+READ_SCALAR_ARRAY(readIntCols, int, atoi)
+READ_SCALAR_ARRAY(readBoolCols, bool, strtobool)