From b11fc6d53e7037b8afb751fd337c51af2daa8196 Mon Sep 17 00:00:00 2001 From: Jimmy Yih Date: Wed, 30 Mar 2016 13:53:20 -0700 Subject: [PATCH] Fix gp_default_storage_options GUC master/segment value conflict. The gp_default_storage_options GUC had two scenarios in which the value between master and segments could be conflicting which would cause tables to possibly have different storage parameters between master and segments. First Scenario: The below gpconfig example would give newly created user tables a default storage parameter of AO on segments while master sets the table as heap. Fixed by not allowing the user to make them different values. gpconfig -c gp_default_storage_options -v "'appendonly=true'" -m "'appendonly=false'" Second Scenario: During a psql session not in transaction, a session-level gp_default_storage_options GUC value can be set which will also be dispatched to segment processes. Those segment processes when idle for a period of time will be stopped and the next SQL (e.g. CREATE TABLE) will spawn new segment processes which will not have the session-level GUC value set. Fixed by setting the value at cdbgang creation. --- gpMgmt/bin/gpconfig | 9 +++++---- src/backend/utils/misc/guc_gp.c | 2 +- src/test/regress/expected/dsp.out | 31 +++++++++++++++++++++++++++++++ src/test/regress/sql/dsp.sql | 12 ++++++++++++ 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/gpMgmt/bin/gpconfig b/gpMgmt/bin/gpconfig index 8668c7969d..f9c73d23e2 100755 --- a/gpMgmt/bin/gpconfig +++ b/gpMgmt/bin/gpconfig @@ -22,6 +22,7 @@ except ImportError, e: EXECNAME = os.path.split(__file__)[-1] prohibitedGucs = set(["port", "listen_addresses"]) +sameValueGucs = set(["gp_default_storage_options"]) longShow = set(["port"]) def parseargs(): @@ -92,6 +93,10 @@ def parseargs(): logger.error("cannot use both value option and primaryvalue/mirrorvalue option") parser.exit() + if (options.masteronly or options.mastervalue) and options.entry in sameValueGucs: + logger.error("%s value cannot be different on master and segments", options.entry) + parser.exit() + if options.value and (not options.mastervalue): options.mastervalue = options.value @@ -283,10 +288,6 @@ options = parseargs() if options.debug: enable_verbose_logging() -if options.masteronly and options.entry == "gp_default_storage_options": - logger.fatal("gp_default_storage_options is not a masteronly parameter.") - sys.exit(1) - try: dburl = dbconn.DbURL() diff --git a/src/backend/utils/misc/guc_gp.c b/src/backend/utils/misc/guc_gp.c index 8f523fd968..5011d6e84b 100644 --- a/src/backend/utils/misc/guc_gp.c +++ b/src/backend/utils/misc/guc_gp.c @@ -5516,7 +5516,7 @@ struct config_string ConfigureNamesString_gp[] = {"gp_default_storage_options", PGC_USERSET, APPENDONLY_TABLES, gettext_noop("Default options for appendonly storage."), NULL, - GUC_NOT_IN_SAMPLE + GUC_NOT_IN_SAMPLE | GUC_GPDB_ADDOPT }, &gp_default_storage_options, "", assign_gp_default_storage_options, NULL }, diff --git a/src/test/regress/expected/dsp.out b/src/test/regress/expected/dsp.out index 1ea7047aff..a259470174 100644 --- a/src/test/regress/expected/dsp.out +++ b/src/test/regress/expected/dsp.out @@ -831,6 +831,37 @@ Error Log in File drop external table ext_t1; drop external table ext_t2; +-- Make sure gp_default_storage_options GUC value is set in newly created cdbgangs +-- after previous idle cdbgang is stopped +SET gp_vmem_idle_resource_timeout=30; +SET gp_default_storage_options='appendonly=true,blocksize=32768,compresstype=none,checksum=true,orientation=row'; +\! sleep 1 +CREATE TABLE check_guc_value_after_new_cdbgang (a int) DISTRIBUTED RANDOMLY; +SELECT DISTINCT relid::regclass FROM pg_appendonly WHERE relid='check_guc_value_after_new_cdbgang'::regclass; + relid +----------------------------------- + check_guc_value_after_new_cdbgang +(1 row) + +SELECT DISTINCT relid::regclass FROM gp_dist_random('pg_appendonly') WHERE relid='check_guc_value_after_new_cdbgang'::regclass; + relid +----------------------------------- + check_guc_value_after_new_cdbgang +(1 row) + +SELECT DISTINCT relname, reloptions FROM pg_class WHERE relname='check_guc_value_after_new_cdbgang'; + relname | reloptions +-----------------------------------+------------------- + check_guc_value_after_new_cdbgang | {appendonly=true} +(1 row) + +SELECT DISTINCT relname, reloptions FROM gp_dist_random('pg_class') WHERE relname='check_guc_value_after_new_cdbgang'; + relname | reloptions +-----------------------------------+------------------- + check_guc_value_after_new_cdbgang | {appendonly=true} +(1 row) + +RESET gp_vmem_idle_resource_timeout; -- cleanup \c postgres drop database dsp1; diff --git a/src/test/regress/sql/dsp.sql b/src/test/regress/sql/dsp.sql index 396583455b..071c1707b5 100644 --- a/src/test/regress/sql/dsp.sql +++ b/src/test/regress/sql/dsp.sql @@ -333,6 +333,18 @@ create external table ext_t2 (a int, b int) drop external table ext_t1; drop external table ext_t2; +-- Make sure gp_default_storage_options GUC value is set in newly created cdbgangs +-- after previous idle cdbgang is stopped +SET gp_vmem_idle_resource_timeout=30; +SET gp_default_storage_options='appendonly=true,blocksize=32768,compresstype=none,checksum=true,orientation=row'; +\! sleep 1 +CREATE TABLE check_guc_value_after_new_cdbgang (a int) DISTRIBUTED RANDOMLY; +SELECT DISTINCT relid::regclass FROM pg_appendonly WHERE relid='check_guc_value_after_new_cdbgang'::regclass; +SELECT DISTINCT relid::regclass FROM gp_dist_random('pg_appendonly') WHERE relid='check_guc_value_after_new_cdbgang'::regclass; +SELECT DISTINCT relname, reloptions FROM pg_class WHERE relname='check_guc_value_after_new_cdbgang'; +SELECT DISTINCT relname, reloptions FROM gp_dist_random('pg_class') WHERE relname='check_guc_value_after_new_cdbgang'; +RESET gp_vmem_idle_resource_timeout; + -- cleanup \c postgres drop database dsp1; -- GitLab