diff --git a/gpMgmt/bin/gpconfig b/gpMgmt/bin/gpconfig index 01ae5390d5f8af91e8dfa70a7ee148d55f0ffdef..208373ebd01c6d99c9cf320745c7191272415a76 100755 --- a/gpMgmt/bin/gpconfig +++ b/gpMgmt/bin/gpconfig @@ -267,7 +267,8 @@ def do_add_config_script(pool, hostname, segs, value, options): print_verbosely(options, hostname, seg.hostname, seg.datadir) - cmd = GpAddConfigScript(hostname, directory_string, options.entry, value, options.remove, ctxt=REMOTE, + cmd = GpAddConfigScript(hostname, directory_string, options.entry, value=value, removeonly=options.remove, + ctxt=REMOTE, remoteHost=hostname) pool.addCommand(cmd) @@ -280,7 +281,15 @@ def do_change(options): gp_array = GpArray.initFromCatalog(dbconn.DbURL(), utility=True) if not options.skipvalidation: - validate_change_options(options) + conn = dbconn.connect(dbconn.DbURL(), True) + guc = get_normal_guc(conn, options) + + # Force the postgresql.conf parser to detect vartype string as GUC_STRING in the guc-file.c/guc-file.l + options.value = quote_string(guc, options.value) + options.mastervalue = quote_string(guc, options.mastervalue) + + validate_change_options(options, conn, guc) + conn.close() except DatabaseError as ex: LOGGER.error(ex.__str__()) @@ -319,8 +328,8 @@ def do_change(options): # do the master if options.mastervalue or options.remove: print_verbosely(options, gp_array.master.hostname, gp_array.master.hostname, gp_array.master.datadir) - cmd = GpAddConfigScript("master", gp_array.master.datadir, options.entry, options.mastervalue, - options.remove, ctxt=REMOTE, remoteHost=gp_array.master.hostname) + cmd = GpAddConfigScript("master", gp_array.master.datadir, options.entry, value=options.mastervalue, + removeonly=options.remove, ctxt=REMOTE, remoteHost=gp_array.master.hostname) pool.addCommand(cmd) # do the standby master @@ -328,7 +337,7 @@ def do_change(options): print_verbosely(options, gp_array.standbyMaster.hostname, gp_array.standbyMaster.hostname, gp_array.standbyMaster.datadir) cmd = GpAddConfigScript("standbymaster", gp_array.standbyMaster.datadir, options.entry, - options.mastervalue, options.remove, ctxt=REMOTE, + value=options.mastervalue, removeonly=options.remove, ctxt=REMOTE, remoteHost=gp_array.standbyMaster.hostname) pool.addCommand(cmd) @@ -355,6 +364,13 @@ def do_change(options): LOGGER.info("completed successfully") +def quote_string(guc, value): + if value and guc and guc.vartype == "string": + if not value.startswith("'") and not value.endswith("'"): + value = "'" + value + "'" + return value + + def _is_guc_writeable(options): """ FYI, metadata about gucs, like GUC_DISALLOW_IN_FILE, is not available via @@ -376,14 +392,12 @@ sql. We work around that by making use of a file already created during 'make in return options.entry not in read_only_gucs -def validate_change_options(options): +def validate_change_options(options, conn, guc): if not _is_guc_writeable(options): msg = "not a modifiable GUC: '%s'" % options.entry LOGGER.fatal(msg) raise Exception(msg) - conn = dbconn.connect(dbconn.DbURL(), True) - guc = get_normal_guc(conn, options) if not guc: # Hidden gucs: a guc is considered hidden if both: # 1. It is not present with normal gucs in pg_settings @@ -399,11 +413,12 @@ def validate_change_options(options): "Please refer to gpconfig documentation." % options.entry LOGGER.fatal(msg) raise Exception(msg) - conn.close() + if options.entry in PROHIBITED_GUCS: msg = "The parameter '%s' is not modifiable with this tool" % options.entry LOGGER.fatal(msg) raise Exception(msg) + if options.value: msg = guc.validate(options.value, options.mastervalue, options) if msg != "ok": diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py b/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py index 40dc6c136769d30b27be7e943b93aab7134127b4..0777dd282aa6b0da98dcbeafd12edbf4334164b0 100644 --- a/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py +++ b/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py @@ -376,6 +376,94 @@ class GpConfig(GpTestCase): pass self.assertEqual(len(self.subject.read_only_gucs), 2) + def test_change_of_unquoted_string_to_quoted_succeeds(self): + vartype = 'string' + unquoted_value = 'baz' + sys.argv = ["gpconfig", "--change", "my_property_name", "--value", unquoted_value] + self.cursor.set_result_for_testing([['my_property_name', 'setting', 'unit', 'short_desc', + 'context', vartype, 'min_val', 'max_val']]) + + self.subject.do_main() + + for call in self.pool.addCommand.call_args_list: + # call_obj[0] returns all unnamed arguments -> ['arg1', 'arg2'] + # In this case, we have an object as an argument to poo.addCommand + # call_obj[1] returns a dict for all named arguments -> {key='arg3', key2='arg4'} + gp_add_config_script_obj = call[0][0] + value = base64.urlsafe_b64encode(pickle.dumps("'baz'")) + try: + self.assertTrue(value in gp_add_config_script_obj.cmdStr) + except AssertionError as e: + raise Exception("\nAssert failed: %s\n cmdStr:\n%s\nvs:\nvalue: %s" % (str(e), + gp_add_config_script_obj.cmdStr, + value)) + + def test_change_of_master_value_with_quotes_succeeds(self): + already_quoted_master_value = "'baz'" + vartype = 'string' + sys.argv = ["gpconfig", "--change", "my_property_name", "--value", 'baz', '--mastervalue', already_quoted_master_value] + self.cursor.set_result_for_testing([['my_property_name', 'setting', 'unit', 'short_desc', + 'context', vartype, 'min_val', 'max_val']]) + + self.subject.do_main() + + for call in self.pool.addCommand.call_args_list: + # call_obj[0] returns all unnamed arguments -> ['arg1', 'arg2'] + # In this case, we have an object as an argument to poo.addCommand + # call_obj[1] returns a dict for all named arguments -> {key='arg3', key2='arg4'} + gp_add_config_script_obj = call[0][0] + value = base64.urlsafe_b64encode(pickle.dumps("'baz'")) + try: + self.assertTrue(value in gp_add_config_script_obj.cmdStr) + except AssertionError as e: + raise Exception("\nAssert failed: %s\n cmdStr:\n%s\nvs:\nvalue: %s" % (str(e), + gp_add_config_script_obj.cmdStr, + value)) + + def test_change_of_master_only_quotes_succeeds(self): + unquoted_master_value = "baz" + vartype = 'string' + sys.argv = ["gpconfig", "--change", "my_property_name", "--value", unquoted_master_value, '--masteronly'] + self.cursor.set_result_for_testing([['my_property_name', 'setting', 'unit', 'short_desc', + 'context', vartype, 'min_val', 'max_val']]) + + self.subject.do_main() + + for call in self.pool.addCommand.call_args_list: + # call_obj[0] returns all unnamed arguments -> ['arg1', 'arg2'] + # In this case, we have an object as an argument to poo.addCommand + # call_obj[1] returns a dict for all named arguments -> {key='arg3', key2='arg4'} + gp_add_config_script_obj = call[0][0] + value = base64.urlsafe_b64encode(pickle.dumps("'baz'")) + try: + self.assertTrue(value in gp_add_config_script_obj.cmdStr) + except AssertionError as e: + raise Exception("\nAssert failed: %s\n cmdStr:\n%s\nvs:\nvalue: %s" % (str(e), + gp_add_config_script_obj.cmdStr, + value)) + + def test_change_of_bool_guc_does_not_quote(self): + unquoted_value = "baz" + vartype = 'bool' + sys.argv = ["gpconfig", "--change", "my_property_name", "--value", unquoted_value] + self.cursor.set_result_for_testing([['my_property_name', 'setting', 'unit', 'short_desc', + 'context', vartype, 'min_val', 'max_val']]) + + self.subject.do_main() + + for call in self.pool.addCommand.call_args_list: + # call_obj[0] returns all unnamed arguments -> ['arg1', 'arg2'] + # In this case, we have an object as an argument to poo.addCommand + # call_obj[1] returns a dict for all named arguments -> {key='arg3', key2='arg4'} + gp_add_config_script_obj = call[0][0] + value = base64.urlsafe_b64encode(pickle.dumps("baz")) + try: + self.assertTrue(value in gp_add_config_script_obj.cmdStr) + except AssertionError as e: + raise Exception("\nAssert failed: %s\n cmdStr:\n%s\nvs:\nvalue: %s" % (str(e), + gp_add_config_script_obj.cmdStr, + value)) + def test_change_when_disallowed_gucs_file_is_missing_gives_warning(self): os.remove(self.guc_disallowed_readonly_file) db_singleton_side_effect_list.append("some happy result")