Skip to content

Commit

Permalink
Fix temp_spill_files_tablespaces GUC sync (#1074)
Browse files Browse the repository at this point in the history 
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 993b6c4)
  • Loading branch information
@silent-observer
silent-observer committed Oct 28, 2024
1 parent 42c0870 commit 7d06628
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 81 deletions.
61 changes: 8 additions & 53 deletions src/backend/utils/misc/guc_gp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5375,97 +5375,52 @@ 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)
{
case PGC_BOOL:
{
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:
{
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:
{
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
Expand Down
34 changes: 16 additions & 18 deletions src/test/regress/input/temp_tablespaces.source
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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';
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -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;
Expand All @@ -308,15 +315,13 @@ $$ 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
\! gpconfig -r temp_tablespaces
\! gpconfig -r temp_spill_files_tablespaces
\! gpstop -u
\c
-- end_ignore
SELECT * FROM gp_tablespace_file_report(1);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -343,17 +346,15 @@ 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);

-- Files should be in temp_filesN, where N = gp_session_id % 4, table should be
-- 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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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

--
Expand Down Expand Up @@ -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.
Expand All @@ -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;
26 changes: 16 additions & 10 deletions src/test/regress/output/temp_tablespaces.source
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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';
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -362,15 +369,14 @@ 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;
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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
---------------+---------------
Expand Down Expand Up @@ -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;

0 comments on commit 7d06628

Please sign in to comment.