v1-0001-Convert-node-test-compile-time-settings-into-run-.patch

From 568c620eb97f82aa8eab850cb3ce703c5c94a912 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 20 May 2024 09:13:23 +0200
Subject: [PATCH v1] Convert node test compile-time settings into run-time
 parameters

This converts

    COPY_PARSE_PLAN_TREES
    WRITE_READ_PARSE_PLAN_TREES
    RAW_EXPRESSION_COVERAGE_TEST

into run-time parameters

    debug_copy_parse_plan_trees
    debug_write_read_parse_plan_trees
    debug_raw_expression_coverage_test

They can be activated for tests using PG_TEST_INITDB_EXTRA_OPTS.

The compile-time symbols are kept for build farm compatibility, but
they now just determine the default value of the run-time settings.

TODO: config.sgml documentation
---
 .cirrus.tasks.yml                   |  3 +-
 src/backend/nodes/README            |  9 ++---
 src/backend/nodes/read.c            | 15 +-------
 src/backend/nodes/readfuncs.c       | 10 +-----
 src/backend/parser/analyze.c        | 14 +++-----
 src/backend/tcop/postgres.c         | 18 ++++------
 src/backend/utils/misc/guc_tables.c | 55 +++++++++++++++++++++++++++++
 src/include/nodes/nodes.h           |  2 --
 src/include/nodes/readfuncs.h       |  2 --
 src/include/pg_config_manual.h      | 21 -----------
 src/include/utils/guc.h             |  4 +++
 11 files changed, 79 insertions(+), 74 deletions(-)

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index a2388cd5036..6a9ff066391 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -133,9 +133,10 @@ task:
     DISK_SIZE: 50
 
     CCACHE_DIR: /tmp/ccache_dir
-    CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
     CFLAGS: -Og -ggdb
 
+    PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c debug_write_read_parse_plan_trees=on -c debug_raw_expression_coverage_test=on
     PG_TEST_PG_UPGRADE_MODE: --link
 
   <<: *freebsd_task_template
diff --git a/src/backend/nodes/README b/src/backend/nodes/README
index 52364470205..f8bbd605386 100644
--- a/src/backend/nodes/README
+++ b/src/backend/nodes/README
@@ -98,10 +98,11 @@ Suppose you want to define a node Foo:
    node types to find all the places to touch.
    (Except for frequently-created nodes, don't bother writing a creator
    function in makefuncs.c.)
-4. Consider testing your new code with COPY_PARSE_PLAN_TREES,
-   WRITE_READ_PARSE_PLAN_TREES, and RAW_EXPRESSION_COVERAGE_TEST to ensure
-   support has been added everywhere that it's necessary; see
-   pg_config_manual.h about these.
+4. Consider testing your new code with debug_copy_parse_plan_trees,
+   debug_write_read_parse_plan_trees, and
+   debug_raw_expression_coverage_test to ensure support has been added
+   everywhere that it's necessary (e.g., run the tests with
+   PG_TEST_INITDB_EXTRA_OPTS='-c debug_...=on').
 
 Adding a new node type moves the numbers associated with existing
 tags, so you'll need to recompile the whole tree after doing this.
diff --git a/src/backend/nodes/read.c b/src/backend/nodes/read.c
index 4eb42445c52..e2d2ce7374d 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -32,9 +32,7 @@
 static const char *pg_strtok_ptr = NULL;
 
 /* State flag that determines how readfuncs.c should treat location fields */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 bool		restore_location_fields = false;
-#endif
 
 
 /*
@@ -42,17 +40,14 @@ bool		restore_location_fields = false;
  *	  builds a Node tree from its string representation (assumed valid)
  *
  * restore_loc_fields instructs readfuncs.c whether to restore location
- * fields rather than set them to -1.  This is currently only supported
- * in builds with the WRITE_READ_PARSE_PLAN_TREES debugging flag set.
+ * fields rather than set them to -1.
  */
 static void *
 stringToNodeInternal(const char *str, bool restore_loc_fields)
 {
 	void	   *retval;
 	const char *save_strtok;
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 	bool		save_restore_location_fields;
-#endif
 
 	/*
 	 * We save and restore the pre-existing state of pg_strtok. This makes the
@@ -67,18 +62,14 @@ stringToNodeInternal(const char *str, bool restore_loc_fields)
 	/*
 	 * If enabled, likewise save/restore the location field handling flag.
 	 */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 	save_restore_location_fields = restore_location_fields;
 	restore_location_fields = restore_loc_fields;
-#endif
 
 	retval = nodeRead(NULL, 0); /* do the reading */
 
 	pg_strtok_ptr = save_strtok;
 
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 	restore_location_fields = save_restore_location_fields;
-#endif
 
 	return retval;
 }
@@ -92,16 +83,12 @@ stringToNode(const char *str)
 	return stringToNodeInternal(str, false);
 }
 
-#ifdef WRITE_READ_PARSE_PLAN_TREES
-
 void *
 stringToNodeWithLocations(const char *str)
 {
 	return stringToNodeInternal(str, true);
 }
 
-#endif
-
 
 /*****************************************************************************
  *
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index c4d01a441a0..32d6b1b2aba 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -19,7 +19,7 @@
  *
  *	  However, if restore_location_fields is true, we do restore location
  *	  fields from the string.  This is currently intended only for use by the
- *	  WRITE_READ_PARSE_PLAN_TREES test code, which doesn't want to cause
+ *	  debug_write_read_parse_plan_trees test code, which doesn't want to cause
  *	  any change in the node contents.
  *
  *-------------------------------------------------------------------------
@@ -118,18 +118,10 @@
 	local_node->fldname = nullable_string(token, length)
 
 /* Read a parse location field (and possibly throw away the value) */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 #define READ_LOCATION_FIELD(fldname) \
 	token = pg_strtok(&length);		/* skip :fldname */ \
 	token = pg_strtok(&length);		/* get field value */ \
 	local_node->fldname = restore_location_fields ? atoi(token) : -1
-#else
-#define READ_LOCATION_FIELD(fldname) \
-	token = pg_strtok(&length);		/* skip :fldname */ \
-	token = pg_strtok(&length);		/* get field value */ \
-	(void) token;				/* in case not used elsewhere */ \
-	local_node->fldname = -1	/* set field to "unknown" */
-#endif
 
 /* Read a Node field */
 #define READ_NODE_FIELD(fldname) \
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 28fed9d87f6..4b0f3c92c0a 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -50,6 +50,7 @@
 #include "parser/parsetree.h"
 #include "utils/backend_status.h"
 #include "utils/builtins.h"
+#include "utils/guc.h"
 #include "utils/rel.h"
 #include "utils/syscache.h"
 
@@ -84,9 +85,7 @@ static Query *transformCallStmt(ParseState *pstate,
 								CallStmt *stmt);
 static void transformLockingClause(ParseState *pstate, Query *qry,
 								   LockingClause *lc, bool pushedDown);
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
 static bool test_raw_expression_coverage(Node *node, void *context);
-#endif
 
 
 /*
@@ -313,11 +312,12 @@ transformStmt(ParseState *pstate, Node *parseTree)
 	Query	   *result;
 
 	/*
-	 * We apply RAW_EXPRESSION_COVERAGE_TEST testing to basic DML statements;
+	 * We apply debug_raw_expression_coverage_test testing to basic DML statements;
 	 * we can't just run it on everything because raw_expression_tree_walker()
 	 * doesn't claim to handle utility statements.
 	 */
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
+	if (Debug_raw_expression_coverage_test)
+	{
 	switch (nodeTag(parseTree))
 	{
 		case T_SelectStmt:
@@ -330,7 +330,7 @@ transformStmt(ParseState *pstate, Node *parseTree)
 		default:
 			break;
 	}
-#endif							/* RAW_EXPRESSION_COVERAGE_TEST */
+	}
 
 	/*
 	 * Caution: when changing the set of statement types that have non-default
@@ -3583,8 +3583,6 @@ applyLockingClause(Query *qry, Index rtindex,
  * applied in limited cases involving CTEs, and we don't really want to have
  * to test everything inside as well as outside a CTE.
  */
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
-
 static bool
 test_raw_expression_coverage(Node *node, void *context)
 {
@@ -3594,5 +3592,3 @@ test_raw_expression_coverage(Node *node, void *context)
 									  test_raw_expression_coverage,
 									  context);
 }
-
-#endif							/* RAW_EXPRESSION_COVERAGE_TEST */
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 45a3794b8e3..dac04004e26 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -622,8 +622,8 @@ pg_parse_query(const char *query_string)
 	if (log_parser_stats)
 		ShowUsage("PARSER STATISTICS");
 
-#ifdef COPY_PARSE_PLAN_TREES
 	/* Optional debugging check: pass raw parsetrees through copyObject() */
+	if (Debug_copy_parse_plan_trees)
 	{
 		List	   *new_list = copyObject(raw_parsetree_list);
 
@@ -633,13 +633,12 @@ pg_parse_query(const char *query_string)
 		else
 			raw_parsetree_list = new_list;
 	}
-#endif
 
 	/*
 	 * Optional debugging check: pass raw parsetrees through
 	 * outfuncs/readfuncs
 	 */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+	if (Debug_write_read_parse_plan_trees)
 	{
 		char	   *str = nodeToStringWithLocations(raw_parsetree_list);
 		List	   *new_list = stringToNodeWithLocations(str);
@@ -651,7 +650,6 @@ pg_parse_query(const char *query_string)
 		else
 			raw_parsetree_list = new_list;
 	}
-#endif
 
 	TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);
 
@@ -826,8 +824,8 @@ pg_rewrite_query(Query *query)
 	if (log_parser_stats)
 		ShowUsage("REWRITER STATISTICS");
 
-#ifdef COPY_PARSE_PLAN_TREES
 	/* Optional debugging check: pass querytree through copyObject() */
+	if (Debug_copy_parse_plan_trees)
 	{
 		List	   *new_list;
 
@@ -838,10 +836,9 @@ pg_rewrite_query(Query *query)
 		else
 			querytree_list = new_list;
 	}
-#endif
 
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 	/* Optional debugging check: pass querytree through outfuncs/readfuncs */
+	if (Debug_write_read_parse_plan_trees)
 	{
 		List	   *new_list = NIL;
 		ListCell   *lc;
@@ -868,7 +865,6 @@ pg_rewrite_query(Query *query)
 		else
 			querytree_list = new_list;
 	}
-#endif
 
 	if (Debug_print_rewritten)
 		elog_node_display(LOG, "rewritten parse tree", querytree_list,
@@ -906,8 +902,8 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
 	if (log_planner_stats)
 		ShowUsage("PLANNER STATISTICS");
 
-#ifdef COPY_PARSE_PLAN_TREES
 	/* Optional debugging check: pass plan tree through copyObject() */
+	if (Debug_copy_parse_plan_trees)
 	{
 		PlannedStmt *new_plan = copyObject(plan);
 
@@ -923,10 +919,9 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
 #endif
 			plan = new_plan;
 	}
-#endif
 
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 	/* Optional debugging check: pass plan tree through outfuncs/readfuncs */
+	if (Debug_write_read_parse_plan_trees)
 	{
 		char	   *str;
 		PlannedStmt *new_plan;
@@ -947,7 +942,6 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
 #endif
 			plan = new_plan;
 	}
-#endif
 
 	/*
 	 * Print plan if debugging.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 46c258be282..82ddb3db730 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -502,6 +502,10 @@ bool		Debug_print_parse = false;
 bool		Debug_print_rewritten = false;
 bool		Debug_pretty_print = true;
 
+bool		Debug_copy_parse_plan_trees;
+bool		Debug_write_read_parse_plan_trees;
+bool		Debug_raw_expression_coverage_test;
+
 bool		log_parser_stats = false;
 bool		log_planner_stats = false;
 bool		log_executor_stats = false;
@@ -1301,6 +1305,57 @@ struct config_bool ConfigureNamesBool[] =
 		false,
 		NULL, NULL, NULL
 	},
+	{
+		{"debug_copy_parse_plan_trees", PGC_USERSET, DEVELOPER_OPTIONS,
+			gettext_noop("Set this to force all parse and plan trees to be passed through "
+						 "copyObject(), to facilitate catching errors and omissions in "
+						 "copyObject()."),
+			NULL,
+			GUC_NOT_IN_SAMPLE
+		},
+		&Debug_copy_parse_plan_trees,
+/* support for legacy compile-time setting */
+#ifdef COPY_PARSE_PLAN_TREES
+		true,
+#else
+		false,
+#endif
+		NULL, NULL, NULL
+	},
+	{
+		{"debug_write_read_parse_plan_trees", PGC_USERSET, DEVELOPER_OPTIONS,
+			gettext_noop("Set this to force all parse and plan trees to be passed through "
+						 "outfuncs.c/readfuncs.c, to facilitate catching errors and omissions in "
+						 "those modules."),
+			NULL,
+			GUC_NOT_IN_SAMPLE
+		},
+		&Debug_write_read_parse_plan_trees,
+/* support for legacy compile-time setting */
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+		true,
+#else
+		false,
+#endif
+		NULL, NULL, NULL
+	},
+	{
+		{"debug_raw_expression_coverage_test", PGC_USERSET, DEVELOPER_OPTIONS,
+			gettext_noop("Set this to force all raw parse trees for DML statements to be scanned "
+						 "by raw_expression_tree_walker(), to facilitate catching errors and "
+						 "omissions in that function."),
+			NULL,
+			GUC_NOT_IN_SAMPLE
+		},
+		&Debug_raw_expression_coverage_test,
+/* support for legacy compile-time setting */
+#ifdef RAW_EXPRESSION_COVERAGE_TEST
+		true,
+#else
+		false,
+#endif
+		NULL, NULL, NULL
+	},
 	{
 		{"debug_print_parse", PGC_USERSET, LOGGING_WHAT,
 			gettext_noop("Logs each query's parse tree."),
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 855009fd6e2..6e290224fe5 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -202,9 +202,7 @@ extern char *bmsToString(const struct Bitmapset *bms);
  * nodes/{readfuncs.c,read.c}
  */
 extern void *stringToNode(const char *str);
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 extern void *stringToNodeWithLocations(const char *str);
-#endif
 extern struct Bitmapset *readBitmapset(void);
 extern uintptr_t readDatum(bool typbyval);
 extern bool *readBoolCols(int numCols);
diff --git a/src/include/nodes/readfuncs.h b/src/include/nodes/readfuncs.h
index 8466038ed06..80d83e77ef0 100644
--- a/src/include/nodes/readfuncs.h
+++ b/src/include/nodes/readfuncs.h
@@ -19,9 +19,7 @@
 /*
  * variable in read.c that needs to be accessible to readfuncs.c
  */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
 extern PGDLLIMPORT bool restore_location_fields;
-#endif
 
 /*
  * prototypes for functions in read.c (the lisp token parser)
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index f941ee2faf8..392a5e8db6b 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -335,33 +335,12 @@
  /* #define RECOVER_RELATION_BUILD_MEMORY 0 */	/* Force disable */
  /* #define RECOVER_RELATION_BUILD_MEMORY 1 */	/* Force enable */
 
-/*
- * Define this to force all parse and plan trees to be passed through
- * copyObject(), to facilitate catching errors and omissions in
- * copyObject().
- */
-/* #define COPY_PARSE_PLAN_TREES */
-
 /*
  * Define this to force Bitmapset reallocation on each modification.  Helps
  * to find dangling pointers to Bitmapset's.
  */
 /* #define REALLOCATE_BITMAPSETS */
 
-/*
- * Define this to force all parse and plan trees to be passed through
- * outfuncs.c/readfuncs.c, to facilitate catching errors and omissions in
- * those modules.
- */
-/* #define WRITE_READ_PARSE_PLAN_TREES */
-
-/*
- * Define this to force all raw parse trees for DML statements to be scanned
- * by raw_expression_tree_walker(), to facilitate catching errors and
- * omissions in that function.
- */
-/* #define RAW_EXPRESSION_COVERAGE_TEST */
-
 /*
  * Enable debugging print statements for lock-related operations.
  */
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e4a594b5e80..f63ee448848 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -245,6 +245,10 @@ extern PGDLLIMPORT bool Debug_print_parse;
 extern PGDLLIMPORT bool Debug_print_rewritten;
 extern PGDLLIMPORT bool Debug_pretty_print;
 
+extern PGDLLIMPORT bool Debug_copy_parse_plan_trees;
+extern PGDLLIMPORT bool Debug_write_read_parse_plan_trees;
+extern PGDLLIMPORT bool Debug_raw_expression_coverage_test;
+
 extern PGDLLIMPORT bool log_parser_stats;
 extern PGDLLIMPORT bool log_planner_stats;
 extern PGDLLIMPORT bool log_executor_stats;

base-commit: fa25dfcd7ec1c72cce68e17aae99bb0f0349749f
-- 
2.44.0