[Tom Lane <tgl@sss.pgh.pa.us>]

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

Attachments:

make-more-planner-struct-fields-dumpable-1.patch (text/x-diff, 14.4K) download
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 */