From 3f4063046acbe51fb96e0b555529284c70ef381c Mon Sep 17 00:00:00 2001 From: Maxim Michkov Date: Fri, 25 Oct 2024 15:09:20 +0300 Subject: [PATCH] Fix temp_spill_files_tablespaces GUC sync (#1074) Previously GUC sync worked by sending SET commands to segments. However, some values (for example the empty string for quoted GUCs) cannot be set this way. This affects specifically temp_spill_files_tablespaces since "" and "\"\"" have different semantic meaning for it. This patch changes the way GUC sync works by using pg_catalog.set_config() function instead of SET commands. This function sets the value of the GUC directly without any quoting issues, and so now empty strings are handled correctly. Ticket: ADBDEV-6438 (cherry picked from commit 993b6c4b24f3773762fe924851da9f6518cb002d) --- src/backend/utils/misc/guc_gp.c | 61 +++---------------- .../regress/input/temp_tablespaces.source | 34 +++++------ .../regress/output/temp_tablespaces.source | 26 +++++--- 3 files changed, 40 insertions(+), 81 deletions(-) diff --git a/src/backend/utils/misc/guc_gp.c b/src/backend/utils/misc/guc_gp.c index 95338003ca22..fd9c3969165a 100644 --- a/src/backend/utils/misc/guc_gp.c +++ b/src/backend/utils/misc/guc_gp.c @@ -5375,7 +5375,7 @@ dispatch_sync_pg_variable_internal(struct config_generic * gconfig, bool is_expl initStringInfo( &buffer ); - appendStringInfo(&buffer, "SET "); + appendStringInfo(&buffer, "SELECT pg_catalog.set_config(%s, ", quote_literal_cstr(gconfig->name)); switch (gconfig->vartype) { @@ -5383,21 +5383,21 @@ dispatch_sync_pg_variable_internal(struct config_generic * gconfig, bool is_expl { struct config_bool *bguc = (struct config_bool *) gconfig; - appendStringInfo(&buffer, "%s TO %s", gconfig->name, *(bguc->variable) ? "true" : "false"); + appendStringInfoString(&buffer, *(bguc->variable) ? "'true'" : "'false'"); break; } case PGC_INT: { struct config_int *iguc = (struct config_int *) gconfig; - appendStringInfo(&buffer, "%s TO %d", gconfig->name, *iguc->variable); + appendStringInfo(&buffer, "'%d'", *iguc->variable); break; } case PGC_REAL: { struct config_real *rguc = (struct config_real *) gconfig; - appendStringInfo(&buffer, " %s TO %f", gconfig->name, *rguc->variable); + appendStringInfo(&buffer, "'%f'", *rguc->variable); break; } case PGC_STRING: @@ -5405,40 +5405,7 @@ dispatch_sync_pg_variable_internal(struct config_generic * gconfig, bool is_expl struct config_string *sguc = (struct config_string *) gconfig; const char *str = *sguc->variable; - appendStringInfo(&buffer, "%s TO ", gconfig->name); - - /* - * If it's a list, we need to split the list into elements and - * quote the elements individually. - * else if it's empty or not a list, we should quote the whole src. - * - * This is the copied from pg_get_functiondef()'s handling of - * proconfig options. - * . - */ - if (sguc->gen.flags & GUC_LIST_QUOTE && str[0] != '\0') - { - List *namelist; - ListCell *lc; - - /* Parse string into list of identifiers */ - if (!SplitGUCList((char *) pstrdup(str), ',', &namelist)) - { - /* this shouldn't fail really */ - elog(ERROR, "invalid list syntax in proconfig item"); - } - foreach(lc, namelist) - { - char *curname = (char *) lfirst(lc); - - appendStringInfoString(&buffer, quote_literal_cstr(curname)); - if (lnext(lc)) - appendStringInfoString(&buffer, ", "); - } - } - else - appendStringInfoString(&buffer, quote_literal_cstr(str)); - + appendStringInfoString(&buffer, quote_literal_cstr(str)); break; } case PGC_ENUM: @@ -5446,26 +5413,14 @@ dispatch_sync_pg_variable_internal(struct config_generic * gconfig, bool is_expl struct config_enum *eguc = (struct config_enum *) gconfig; int value = *eguc->variable; const char *str = config_enum_lookup_by_value(eguc, value); - int i; - - appendStringInfo(&buffer, "%s TO ", gconfig->name); - /* - * All whitespace characters must be escaped. See - * pg_split_opts() in the backend. (Not sure if an enum value - * can have whitespace, but let's be prepared.) - */ - for (i = 0; str[i] != '\0'; i++) - { - if (isspace((unsigned char) str[i])) - appendStringInfoChar(&buffer, '\\'); - - appendStringInfoChar(&buffer, str[i]); - } + appendStringInfoString(&buffer, quote_literal_cstr(str)); break; } } + appendStringInfo(&buffer, ", false)"); + if (is_explicit) CdbDispatchSetCommandForSync(buffer.data); else diff --git a/src/test/regress/input/temp_tablespaces.source b/src/test/regress/input/temp_tablespaces.source index 694da7629a85..15275973185e 100644 --- a/src/test/regress/input/temp_tablespaces.source +++ b/src/test/regress/input/temp_tablespaces.source @@ -219,8 +219,8 @@ ANALYZE tts_jazz; CREATE TABLESPACE temp_tables LOCATION '@testtablespace@_temp_tables'; CREATE TABLESPACE temp_files0 LOCATION '@testtablespace@_temp_files0'; -CREATE TABLESPACE temp_files1 LOCATION '@testtablespace@_temp_files1'; -CREATE TABLESPACE temp_files2 LOCATION '@testtablespace@_temp_files2'; +CREATE TABLESPACE "temp_files1'" LOCATION '@testtablespace@_temp_files1'; +CREATE TABLESPACE "temp_files2""" LOCATION '@testtablespace@_temp_files2'; CREATE TABLESPACE temp_files3 LOCATION '@testtablespace@_temp_files3'; RESET temp_tablespaces; @@ -243,6 +243,7 @@ DECLARE owo int; session_file_ts_name text; temp_table_ts_name text; + session_file_ts_filename text; BEGIN SET optimizer = off; SET statement_mem = '1MB'; @@ -262,7 +263,13 @@ BEGIN -- Find out the tablespace that will be used for files. -- N = gp_session_id % number of tablespaces in GUC. SELECT 'temp_files' || (current_setting('gp_session_id')::int % n_temp_spill_files_tablespaces) - INTO session_file_ts_name; + INTO session_file_ts_filename; + SELECT CASE current_setting('gp_session_id')::int % n_temp_spill_files_tablespaces + WHEN 0 THEN 'temp_files0' + WHEN 1 THEN 'temp_files1''' + WHEN 2 THEN 'temp_files2"' + WHEN 3 THEN 'temp_files3' + END INTO session_file_ts_name; -- Create the temporary directory. WITH out11 AS ( @@ -278,7 +285,7 @@ BEGIN -- Start monitoring temporary tablespace usage. PERFORM gp_tablespace_watch_start(gp_execution_dbid(), 'default', gp_temptablespace_path((SELECT dattablespace FROM pg_database WHERE datname = current_database() LIMIT 1))); PERFORM gp_tablespace_watch_start(gp_execution_dbid(), 'temp_tables', gp_temptablespace_path((SELECT oid FROM pg_tablespace WHERE spcname = 'temp_tables' LIMIT 1))); - PERFORM gp_tablespace_watch_start(gp_execution_dbid(), session_file_ts_name, gp_temptablespace_path((SELECT oid FROM pg_tablespace WHERE spcname = session_file_ts_name LIMIT 1))); + PERFORM gp_tablespace_watch_start(gp_execution_dbid(), session_file_ts_filename, gp_temptablespace_path((SELECT oid FROM pg_tablespace WHERE spcname = session_file_ts_name LIMIT 1))); -- Run the query again to create temporary hashjoin files in the tablespaces. WITH out11 AS ( @@ -299,7 +306,7 @@ BEGIN CASE WHEN gp_tablespace_watch_match(dbid, 'default', 'HashJoin') > 0 THEN 'default' WHEN gp_tablespace_watch_match(dbid, 'temp_tables', 'HashJoin') > 0 THEN 'temp_tables' - WHEN gp_tablespace_watch_match(dbid, session_file_ts_name, 'HashJoin') > 0 THEN 'temp_filesN' + WHEN gp_tablespace_watch_match(dbid, session_file_ts_filename, 'HashJoin') > 0 THEN 'temp_filesN' END FROM gp_segment_configuration WHERE role = 'p' AND content >= 0; @@ -308,7 +315,6 @@ $$ LANGUAGE plpgsql; -- -- Global GUC value tests: --- FIXME: We shouldn't be forced to use \c to update GUCs on segments. -- -- Both files and the table should be in the default tablespace. @@ -316,7 +322,6 @@ $$ LANGUAGE plpgsql; \! gpconfig -r temp_tablespaces \! gpconfig -r temp_spill_files_tablespaces \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(1); @@ -325,7 +330,6 @@ SELECT * FROM gp_tablespace_file_report(1); \! gpconfig -c temp_tablespaces -v 'temp_tables' \! gpconfig -r temp_spill_files_tablespaces \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(1); @@ -334,7 +338,6 @@ SELECT * FROM gp_tablespace_file_report(1); \! gpconfig -r temp_tablespaces \! gpconfig -c temp_spill_files_tablespaces -v 'temp_files0' \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(1); @@ -343,7 +346,6 @@ SELECT * FROM gp_tablespace_file_report(1); \! gpconfig -c temp_tablespaces -v 'temp_tables' \! gpconfig -c temp_spill_files_tablespaces -v 'temp_files0' \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(1); @@ -351,9 +353,8 @@ SELECT * FROM gp_tablespace_file_report(1); -- in the default tablespace. -- start_ignore \! gpconfig -r 'temp_tablespaces' -\! gpconfig -c temp_spill_files_tablespaces -v 'temp_files0','temp_files1','temp_files2','temp_files3' +\! gpconfig -c temp_spill_files_tablespaces -v 'temp_files0','temp_files1'\''','temp_files2"','temp_files3' \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(4); @@ -362,7 +363,6 @@ SELECT * FROM gp_tablespace_file_report(4); \! gpconfig -c temp_tablespaces -v 'temp_tables' \! gpconfig -c temp_spill_files_tablespaces -v '""' \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(1); @@ -371,7 +371,6 @@ SELECT * FROM gp_tablespace_file_report(1); \! gpconfig -c temp_tablespaces -v 'temp_tables' \! gpconfig -c temp_spill_files_tablespaces -v 'pg_default' \! gpstop -u -\c -- end_ignore SELECT * FROM gp_tablespace_file_report(1); @@ -380,7 +379,6 @@ SELECT * FROM gp_tablespace_file_report(1); \! gpconfig -r temp_tablespaces \! gpconfig -r temp_spill_files_tablespaces \! gpstop -u -\c -- end_ignore -- @@ -410,7 +408,7 @@ SELECT * FROM gp_tablespace_file_report(1); -- Files should be in temp_filesN, where N = gp_session_id % 4, table should be -- in the default tablespace. RESET temp_tablespaces; -SET temp_spill_files_tablespaces = 'temp_files0','temp_files1','temp_files2','temp_files3'; +SET temp_spill_files_tablespaces = 'temp_files0','temp_files1''','temp_files2"','temp_files3'; SELECT * FROM gp_tablespace_file_report(4); -- Files should be in the default tablespace, table should be in temp_tables. @@ -432,6 +430,6 @@ DROP TABLE tts_jazz; DROP TABLESPACE temp_tables; DROP TABLESPACE temp_files0; -DROP TABLESPACE temp_files1; -DROP TABLESPACE temp_files2; +DROP TABLESPACE "temp_files1'"; +DROP TABLESPACE "temp_files2"""; DROP TABLESPACE temp_files3; diff --git a/src/test/regress/output/temp_tablespaces.source b/src/test/regress/output/temp_tablespaces.source index 1d746ec5c52c..87b1fe45edaf 100644 --- a/src/test/regress/output/temp_tablespaces.source +++ b/src/test/regress/output/temp_tablespaces.source @@ -284,8 +284,8 @@ ANALYZE tts_bar; ANALYZE tts_jazz; CREATE TABLESPACE temp_tables LOCATION '@testtablespace@_temp_tables'; CREATE TABLESPACE temp_files0 LOCATION '@testtablespace@_temp_files0'; -CREATE TABLESPACE temp_files1 LOCATION '@testtablespace@_temp_files1'; -CREATE TABLESPACE temp_files2 LOCATION '@testtablespace@_temp_files2'; +CREATE TABLESPACE "temp_files1'" LOCATION '@testtablespace@_temp_files1'; +CREATE TABLESPACE "temp_files2""" LOCATION '@testtablespace@_temp_files2'; CREATE TABLESPACE temp_files3 LOCATION '@testtablespace@_temp_files3'; RESET temp_tablespaces; RESET temp_spill_files_tablespaces; @@ -306,6 +306,7 @@ DECLARE owo int; session_file_ts_name text; temp_table_ts_name text; + session_file_ts_filename text; BEGIN SET optimizer = off; SET statement_mem = '1MB'; @@ -325,7 +326,13 @@ BEGIN -- Find out the tablespace that will be used for files. -- N = gp_session_id % number of tablespaces in GUC. SELECT 'temp_files' || (current_setting('gp_session_id')::int % n_temp_spill_files_tablespaces) - INTO session_file_ts_name; + INTO session_file_ts_filename; + SELECT CASE current_setting('gp_session_id')::int % n_temp_spill_files_tablespaces + WHEN 0 THEN 'temp_files0' + WHEN 1 THEN 'temp_files1''' + WHEN 2 THEN 'temp_files2"' + WHEN 3 THEN 'temp_files3' + END INTO session_file_ts_name; -- Create the temporary directory. WITH out11 AS ( @@ -341,7 +348,7 @@ BEGIN -- Start monitoring temporary tablespace usage. PERFORM gp_tablespace_watch_start(gp_execution_dbid(), 'default', gp_temptablespace_path((SELECT dattablespace FROM pg_database WHERE datname = current_database() LIMIT 1))); PERFORM gp_tablespace_watch_start(gp_execution_dbid(), 'temp_tables', gp_temptablespace_path((SELECT oid FROM pg_tablespace WHERE spcname = 'temp_tables' LIMIT 1))); - PERFORM gp_tablespace_watch_start(gp_execution_dbid(), session_file_ts_name, gp_temptablespace_path((SELECT oid FROM pg_tablespace WHERE spcname = session_file_ts_name LIMIT 1))); + PERFORM gp_tablespace_watch_start(gp_execution_dbid(), session_file_ts_filename, gp_temptablespace_path((SELECT oid FROM pg_tablespace WHERE spcname = session_file_ts_name LIMIT 1))); -- Run the query again to create temporary hashjoin files in the tablespaces. WITH out11 AS ( @@ -362,7 +369,7 @@ BEGIN CASE WHEN gp_tablespace_watch_match(dbid, 'default', 'HashJoin') > 0 THEN 'default' WHEN gp_tablespace_watch_match(dbid, 'temp_tables', 'HashJoin') > 0 THEN 'temp_tables' - WHEN gp_tablespace_watch_match(dbid, session_file_ts_name, 'HashJoin') > 0 THEN 'temp_filesN' + WHEN gp_tablespace_watch_match(dbid, session_file_ts_filename, 'HashJoin') > 0 THEN 'temp_filesN' END FROM gp_segment_configuration WHERE role = 'p' AND content >= 0; @@ -370,7 +377,6 @@ END; $$ LANGUAGE plpgsql; -- -- Global GUC value tests: --- FIXME: We shouldn't be forced to use \c to update GUCs on segments. -- -- Both files and the table should be in the default tablespace. -- start_ignore @@ -432,7 +438,7 @@ SELECT * FROM gp_tablespace_file_report(1); -- in the default tablespace. -- start_ignore \! gpconfig -r 'temp_tablespaces' -\! gpconfig -c temp_spill_files_tablespaces -v 'temp_files0','temp_files1','temp_files2','temp_files3' +\! gpconfig -c temp_spill_files_tablespaces -v 'temp_files0','temp_files1'\''','temp_files2"','temp_files3' \! gpstop -u -- end_ignore SELECT * FROM gp_tablespace_file_report(4); @@ -527,7 +533,7 @@ SELECT * FROM gp_tablespace_file_report(1); -- Files should be in temp_filesN, where N = gp_session_id % 4, table should be -- in the default tablespace. RESET temp_tablespaces; -SET temp_spill_files_tablespaces = 'temp_files0','temp_files1','temp_files2','temp_files3'; +SET temp_spill_files_tablespaces = 'temp_files0','temp_files1''','temp_files2"','temp_files3'; SELECT * FROM gp_tablespace_file_report(4); temp_table_in | temp_files_in ---------------+--------------- @@ -565,6 +571,6 @@ DROP TABLE tts_bar; DROP TABLE tts_jazz; DROP TABLESPACE temp_tables; DROP TABLESPACE temp_files0; -DROP TABLESPACE temp_files1; -DROP TABLESPACE temp_files2; +DROP TABLESPACE "temp_files1'"; +DROP TABLESPACE "temp_files2"""; DROP TABLESPACE temp_files3;