From 568c620eb97f82aa8eab850cb3ce703c5c94a912 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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