From 80f35c90e3414dabd879e8832ce1b89c685e5de5 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 24 May 2024 11:42:02 +0200 Subject: [PATCH v2] 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. Furthermore, support for these settings is not compiled in at all unless assertions are enabled, or the new symbol DEBUG_NODE_TESTS_ENABLED is defined at compile time, or any of the legacy compile-time setting symbols are defined. So there is no run-time overhead in production builds. (This is similar to the handling of DISCARD_CACHES_ENABLED.) Discussion: https://www.postgresql.org/message-id/flat/30747bd8-f51e-4e0c-a310-a6e2c37ec8aa%40eisentraut.org --- .cirrus.tasks.yml | 3 +- doc/src/sgml/config.sgml | 71 +++++++++++++++++++++++++++++ src/backend/nodes/README | 9 ++-- src/backend/nodes/read.c | 12 ++--- src/backend/nodes/readfuncs.c | 4 +- src/backend/parser/analyze.c | 44 ++++++++++-------- src/backend/tcop/postgres.c | 30 +++++++----- src/backend/utils/misc/guc_tables.c | 59 ++++++++++++++++++++++++ src/include/nodes/nodes.h | 2 +- src/include/nodes/readfuncs.h | 2 +- src/include/pg_config_manual.h | 34 +++++++------- src/include/utils/guc.h | 6 +++ 12 files changed, 212 insertions(+), 64 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/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 698169afdb6..c121a13b4c3 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11372,6 +11372,29 @@ Developer Options + + debug_copy_parse_plan_trees (boolean) + + debug_copy_parse_plan_trees configuration parameter + + + + + Enabling this forces all parse and plan trees to be passed through + copyObject(), to facilitate catching errors and + omissions in copyObject(). The default is off. + + + + This parameter is only available when + DEBUG_NODE_TESTS_ENABLED was defined at compile time + (which happens automatically when using the + configure option + ). + + + + debug_discard_caches (integer) @@ -11487,6 +11510,54 @@ Developer Options + + debug_raw_expression_coverage_test (boolean) + + debug_raw_expression_coverage_test configuration parameter + + + + + Enabling this forces all raw parse trees for DML statements to be + scanned by raw_expression_tree_walker(), to + facilitate catching errors and omissions in that function. The + default is off. + + + + This parameter is only available when + DEBUG_NODE_TESTS_ENABLED was defined at compile time + (which happens automatically when using the + configure option + ). + + + + + + debug_write_read_parse_plan_trees (boolean) + + debug_write_read_parse_plan_trees configuration parameter + + + + + Enabling this forces all parse and plan trees to be passed through + outfuncs.c/readfuncs.c, to + facilitate catching errors and omissions in those modules. The + default is off. + + + + This parameter is only available when + DEBUG_NODE_TESTS_ENABLED was defined at compile time + (which happens automatically when using the + configure option + ). + + + + ignore_system_indexes (boolean) 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..190099e5cf3 100644 --- a/src/backend/nodes/read.c +++ b/src/backend/nodes/read.c @@ -32,7 +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 +#ifdef DEBUG_NODE_TESTS_ENABLED bool restore_location_fields = false; #endif @@ -43,14 +43,14 @@ bool restore_location_fields = false; * * 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. + * in builds with DEBUG_NODE_TESTS_ENABLED defined. */ static void * stringToNodeInternal(const char *str, bool restore_loc_fields) { void *retval; const char *save_strtok; -#ifdef WRITE_READ_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED bool save_restore_location_fields; #endif @@ -67,7 +67,7 @@ stringToNodeInternal(const char *str, bool restore_loc_fields) /* * If enabled, likewise save/restore the location field handling flag. */ -#ifdef WRITE_READ_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED save_restore_location_fields = restore_location_fields; restore_location_fields = restore_loc_fields; #endif @@ -76,7 +76,7 @@ stringToNodeInternal(const char *str, bool restore_loc_fields) pg_strtok_ptr = save_strtok; -#ifdef WRITE_READ_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED restore_location_fields = save_restore_location_fields; #endif @@ -92,7 +92,7 @@ stringToNode(const char *str) return stringToNodeInternal(str, false); } -#ifdef WRITE_READ_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED void * stringToNodeWithLocations(const char *str) diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index c4d01a441a0..b47950764a4 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,7 +118,7 @@ local_node->fldname = nullable_string(token, length) /* Read a parse location field (and possibly throw away the value) */ -#ifdef WRITE_READ_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED #define READ_LOCATION_FIELD(fldname) \ token = pg_strtok(&length); /* skip :fldname */ \ token = pg_strtok(&length); /* get field value */ \ diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 28fed9d87f6..e901203424d 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,7 +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 +#ifdef DEBUG_NODE_TESTS_ENABLED static bool test_raw_expression_coverage(Node *node, void *context); #endif @@ -312,25 +313,30 @@ transformStmt(ParseState *pstate, Node *parseTree) { Query *result; +#ifdef DEBUG_NODE_TESTS_ENABLED + /* - * We apply 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. + * 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 - switch (nodeTag(parseTree)) + if (Debug_raw_expression_coverage_test) { - case T_SelectStmt: - case T_InsertStmt: - case T_UpdateStmt: - case T_DeleteStmt: - case T_MergeStmt: - (void) test_raw_expression_coverage(parseTree, NULL); - break; - default: - break; + switch (nodeTag(parseTree)) + { + case T_SelectStmt: + case T_InsertStmt: + case T_UpdateStmt: + case T_DeleteStmt: + case T_MergeStmt: + (void) test_raw_expression_coverage(parseTree, NULL); + break; + default: + break; + } } -#endif /* RAW_EXPRESSION_COVERAGE_TEST */ +#endif /* DEBUG_NODE_TESTS_ENABLED */ /* * Caution: when changing the set of statement types that have non-default @@ -3575,6 +3581,7 @@ applyLockingClause(Query *qry, Index rtindex, qry->rowMarks = lappend(qry->rowMarks, rc); } +#ifdef DEBUG_NODE_TESTS_ENABLED /* * Coverage testing for raw_expression_tree_walker(). * @@ -3583,8 +3590,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 +3599,4 @@ test_raw_expression_coverage(Node *node, void *context) test_raw_expression_coverage, context); } - -#endif /* RAW_EXPRESSION_COVERAGE_TEST */ +#endif /* DEBUG_NODE_TESTS_ENABLED */ diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 45a3794b8e3..3746e7e4d17 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -622,8 +622,10 @@ pg_parse_query(const char *query_string) if (log_parser_stats) ShowUsage("PARSER STATISTICS"); -#ifdef COPY_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED + /* Optional debugging check: pass raw parsetrees through copyObject() */ + if (Debug_copy_parse_plan_trees) { List *new_list = copyObject(raw_parsetree_list); @@ -633,13 +635,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 +652,8 @@ pg_parse_query(const char *query_string) else raw_parsetree_list = new_list; } -#endif + +#endif /* DEBUG_NODE_TESTS_ENABLED */ TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string); @@ -826,8 +828,10 @@ pg_rewrite_query(Query *query) if (log_parser_stats) ShowUsage("REWRITER STATISTICS"); -#ifdef COPY_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED + /* Optional debugging check: pass querytree through copyObject() */ + if (Debug_copy_parse_plan_trees) { List *new_list; @@ -838,10 +842,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 +871,8 @@ pg_rewrite_query(Query *query) else querytree_list = new_list; } -#endif + +#endif /* DEBUG_NODE_TESTS_ENABLED */ if (Debug_print_rewritten) elog_node_display(LOG, "rewritten parse tree", querytree_list, @@ -906,8 +910,10 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions, if (log_planner_stats) ShowUsage("PLANNER STATISTICS"); -#ifdef COPY_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED + /* Optional debugging check: pass plan tree through copyObject() */ + if (Debug_copy_parse_plan_trees) { PlannedStmt *new_plan = copyObject(plan); @@ -923,10 +929,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 +952,8 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions, #endif plan = new_plan; } -#endif + +#endif /* DEBUG_NODE_TESTS_ENABLED */ /* * Print plan if debugging. diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 46c258be282..d76b2cf0957 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -502,6 +502,12 @@ bool Debug_print_parse = false; bool Debug_print_rewritten = false; bool Debug_pretty_print = true; +#ifdef DEBUG_NODE_TESTS_ENABLED +bool Debug_copy_parse_plan_trees; +bool Debug_write_read_parse_plan_trees; +bool Debug_raw_expression_coverage_test; +#endif + bool log_parser_stats = false; bool log_planner_stats = false; bool log_executor_stats = false; @@ -1301,6 +1307,59 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, +#ifdef DEBUG_NODE_TESTS_ENABLED + { + {"debug_copy_parse_plan_trees", PGC_SUSET, 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_SUSET, 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_SUSET, 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 + }, +#endif /* DEBUG_NODE_TESTS_ENABLED */ { {"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..a96f7c97f16 100644 --- a/src/include/nodes/nodes.h +++ b/src/include/nodes/nodes.h @@ -202,7 +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 +#ifdef DEBUG_NODE_TESTS_ENABLED extern void *stringToNodeWithLocations(const char *str); #endif extern struct Bitmapset *readBitmapset(void); diff --git a/src/include/nodes/readfuncs.h b/src/include/nodes/readfuncs.h index 8466038ed06..5cd35d68706 100644 --- a/src/include/nodes/readfuncs.h +++ b/src/include/nodes/readfuncs.h @@ -19,7 +19,7 @@ /* * variable in read.c that needs to be accessible to readfuncs.c */ -#ifdef WRITE_READ_PARSE_PLAN_TREES +#ifdef DEBUG_NODE_TESTS_ENABLED extern PGDLLIMPORT bool restore_location_fields; #endif diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index f941ee2faf8..e3da9fa13d2 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -336,31 +336,31 @@ /* #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 DEBUG_NODE_TESTS_ENABLED to enable use of the GUCs + * debug_copy_parse_plan_trees, debug_write_read_parse_plan_trees, and + * debug_raw_expression_coverage_test, to test coverage of node support + * functions in src/backend/nodes/. + * + * USE_ASSERT_CHECKING builds default to enabling this. */ -/* #define COPY_PARSE_PLAN_TREES */ +/* #define DEBUG_NODE_TESTS_ENABLED */ -/* - * Define this to force Bitmapset reallocation on each modification. Helps - * to find dangling pointers to Bitmapset's. - */ -/* #define REALLOCATE_BITMAPSETS */ +#if defined(USE_ASSERT_CHECKING) && !defined(DEBUG_NODE_TESTS_ENABLED) +#define DEBUG_NODE_TESTS_ENABLED +#endif /* - * 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. + * Backwards compatibility for the older compile-time-only node-tests macros. */ -/* #define WRITE_READ_PARSE_PLAN_TREES */ +#if !defined(DEBUG_NODE_TESTS_ENABLED) && (defined(COPY_PARSE_PLAN_TREES) || defined(WRITE_READ_PARSE_PLAN_TREES) || defined(RAW_EXPRESSION_COVERAGE_TEST)) +#define DEBUG_NODE_TESTS_ENABLED +#endif /* - * 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 this to force Bitmapset reallocation on each modification. Helps + * to find dangling pointers to Bitmapset's. */ -/* #define RAW_EXPRESSION_COVERAGE_TEST */ +/* #define REALLOCATE_BITMAPSETS */ /* * Enable debugging print statements for lock-related operations. diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index e4a594b5e80..35f0ef6f5b6 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -245,6 +245,12 @@ extern PGDLLIMPORT bool Debug_print_parse; extern PGDLLIMPORT bool Debug_print_rewritten; extern PGDLLIMPORT bool Debug_pretty_print; +#ifdef DEBUG_NODE_TESTS_ENABLED +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; +#endif + extern PGDLLIMPORT bool log_parser_stats; extern PGDLLIMPORT bool log_planner_stats; extern PGDLLIMPORT bool log_executor_stats; base-commit: 53785d2a2aaa7899eb82fb4eba9af6da83680c8d -- 2.44.0