gpconfig: allow unquoted strings as value args on cli

When you use gpconfig, users were forced to double quote the guc values.
For example:
    Prior:
       gpconfig -c pljava_classpath -v "'examples.jar'"

    Now:
       gpconfig -c pljava_classpath -v 'examples.jar' or gpconfig -c pljava_classpath -v examples.jar

Enhancement: Now all GUCs with vartype string will be wrapped in single
quotes before being written to the postgresql.conf.
Signed-off-by: NMarbin Tan <mtan@pivotal.io>

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":

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")