gpMgmt: fix five small defects the Behave suite exposes

gpMgmt/bin/analyzedb

@@ -167,12 +167,24 @@ SELECT schemaname, tablename FROM
 ORDER BY tableoid DESC;
 """
 
+def quote_literal(value):
+    """
+    Quote a name for use inside a SQL string literal.
+
+    Not pg.escape_string(): PyGreSQL's C implementation encodes its argument as
+    ASCII and raises UnicodeEncodeError on a schema, table or column name that
+    contains non-ASCII characters. With standard_conforming_strings on -- the
+    default -- doubling single quotes is the whole of the escaping needed.
+    """
+    return value.replace("'", "''")
+
+
 def validate_schema_exists(pg_port, dbname, schema):
     conn = None
     try:
         dburl = dbconn.DbURL(port=pg_port, dbname=dbname)
         conn = dbconn.connect(dburl)
-        count = dbconn.querySingleton(conn, "select count(*) from pg_namespace where nspname='%s';" % pg.escape_string(schema))
+        count = dbconn.querySingleton(conn, "select count(*) from pg_namespace where nspname='%s';" % quote_literal(schema))
         if count == 0:
             raise ExceptionNoStackTraceNeeded("Schema %s does not exist in database %s." % (schema, dbname))
     finally:
@@ -982,7 +994,7 @@ def get_oid_str(table_list):
 def regclass_schema_tbl(schema, tbl):
     schema_tbl = "%s.%s" % (escape_identifier(schema), escape_identifier(tbl))
 
-    return "to_regclass('%s')" % (pg.escape_string(schema_tbl))
+    return "to_regclass('%s')" % quote_literal(schema_tbl)
 
 
 # Escape double-quotes in a string, so that the resulting string is suitable for
@@ -1250,7 +1262,7 @@ def validate_tables(conn, tablenames):
     while curr_batch < nbatches:
         batch = tablenames[curr_batch * batch_size:(curr_batch + 1) * batch_size]
 
-        oid_str = ','.join(map((lambda x: "('%s')" % pg.escape_string(x)), batch))
+        oid_str = ','.join(map((lambda x: "('%s')" % quote_literal(x)), batch))
         if not oid_str:
             break
 
@@ -1266,7 +1278,7 @@ def get_include_cols_from_exclude(conn, schema, table, exclude_cols):
     """
     Given a list of excluded columns of a table, get the list of included columns
     """
-    quoted_exclude_cols = ','.join(["'%s'" % pg.escape_string(x) for x in exclude_cols])
+    quoted_exclude_cols = ','.join(["'%s'" % quote_literal(x) for x in exclude_cols])
 
     oid_str = regclass_schema_tbl(schema, table)
     cols = run_sql(conn, GET_INCLUDED_COLUMNS_FROM_EXCLUDE_SQL % (oid_str, quoted_exclude_cols))
@@ -1282,7 +1294,7 @@ def validate_columns(conn, schema, table, column_list):
         return
 
     sql = VALIDATE_COLUMN_NAMES_SQL % (regclass_schema_tbl(schema, table),
-                                       ','.join(["'%s'" % pg.escape_string(x) for x in column_list]))
+                                       ','.join(["'%s'" % quote_literal(x) for x in column_list]))
     valid_col_count = dbconn.querySingleton(conn, sql)
 
     if int(valid_col_count) != len(column_list):

gpMgmt/bin/gppylib/commands/unix.py

@@ -55,16 +55,41 @@ def check_pid_on_remotehost(pid, host):
 
 
 def check_pid(pid):
-    """ Check For the existence of a unix pid. """
+    """
+    Check for the existence of a unix pid, meaning a process that is still
+    running.
+
+    os.kill(pid, 0) on its own is not that question: it keeps succeeding for a
+    process that has exited and has not been reaped, and nothing reaps an orphan
+    in a container started without an init, so such a zombie answers yes for the
+    life of the container. Every caller here is deciding whether something is
+    still working -- a postmaster, a utility holding a lock, a segment that was
+    just signalled -- and for all of them a zombie has gone.
+    """
+
+    try:
+        pid = int(pid)
+    except (TypeError, ValueError):
+        # A lock file that was created but never written leaves an empty PID
+        # behind; whatever wrote it is not running either way, and the callers
+        # ask this question to decide whether to clean up.
+        return False
 
     if pid == 0:
         return False
 
     try:
-        os.kill(int(pid), signal.SIG_DFL)
+        os.kill(pid, signal.SIG_DFL)
     except OSError:
         return False
-    else:
+
+    try:
+        with open('/proc/%d/stat' % pid) as statfile:
+            # The comm field is parenthesised and may itself contain spaces and
+            # parentheses, so state is the first field after the last ')'.
+            return statfile.read().rpartition(')')[2].split()[0] != 'Z'
+    except (IOError, OSError, IndexError, ValueError):
+        # No /proc to consult; the kill() above is the best answer available.
         return True
 
 

gpMgmt/bin/gppylib/mainUtils.py

@@ -307,13 +307,13 @@ def simple_main_internal(createOptionParserFn, createCommandFn, mainOptions):
 
     # at this point we have whatever lock we require
     try:
-        simple_main_locked(parserOptions, parserArgs, createCommandFn, mainOptions)
+        simple_main_locked(parser, parserOptions, parserArgs, createCommandFn, mainOptions)
     finally:
         if sml is not None:
             sml.release()
 
 
-def simple_main_locked(parserOptions, parserArgs, createCommandFn, mainOptions):
+def simple_main_locked(parser, parserOptions, parserArgs, createCommandFn, mainOptions):
     """
     Not to be called externally -- use simple_main instead
     """
@@ -326,7 +326,6 @@ def simple_main_locked(parserOptions, parserArgs, createCommandFn, mainOptions):
     faultProberInterface.registerFaultProber(faultProberImplGpdb.GpFaultProberImplGpdb())
 
     commandObject = None
-    parser = None
 
     forceQuiet = mainOptions is not None and mainOptions.get("forceQuietOutput")
 

gpMgmt/bin/gppylib/test/unit/test_unit_seg_update_pg_hba.py

@@ -67,3 +67,87 @@ def test_remove_dup_with_spaces_add_entries(self):
         expected_out_entries = expected_block.\
             format(ip_mirror1='10.0.0.1', ip_mirror2='10.0.0.2', ip_mirror3='10.0.0.3').split('\n')
         self.assertEqual(result_out_entries, expected_out_entries)
+
+    def test_remove_stale_replication_entries(self):
+        existing_entries = """host replication gpadmin samehost trust
+host all gpadmin 10.0.0.1/32 trust
+host all gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.1/32 trust
+host replication gpadmin 10.0.0.2/32 trust""".split('\n')
+
+        input_entries = 'host replication gpadmin samehost trust\nhost all gpadmin 10.0.0.2/32 trust' \
+                        '\nhost replication gpadmin 10.0.0.2/32 trust\nhost replication gpadmin 10.0.0.3/32 trust'
+        unique_entries = seg_update_pg_hba.remove_dup_add_entries(existing_entries, input_entries)
+        result = seg_update_pg_hba.remove_stale_replication_entries(unique_entries, input_entries)
+
+        expected = """host replication gpadmin samehost trust
+host all gpadmin 10.0.0.1/32 trust
+host all gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.3/32 trust""".split('\n')
+        self.assertEqual(result, expected)
+
+    def test_pg_hba_with_no_replication_entries(self):
+        existing_entries = """# standby coordinator host ip addresses
+host all gpadmin 10.0.0.2/32 trust
+host all gpadmin 10.0.0.1/32 trust""".split('\n')
+        input_entries = '# standby coordinator host ip addresses\nhost all gpadmin 10.0.0.2/32 trust'
+
+        result = seg_update_pg_hba.remove_stale_replication_entries(existing_entries, input_entries)
+
+        self.assertEqual(result, existing_entries)
+
+    def test_remove_stale_replication_entries_without_removing_localhost(self):
+        existing_entries = """# Allow replication connections from localhost, by a user with the
+# replication privilege.
+local   replication     all                                     trust
+host    replication     all             127.0.0.1/32            trust
+host    replication     all             ::1/128                 trust
+host replication gpadmin samehost trust
+host all gpadmin 10.0.0.1/32 trust
+host all gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.1/32 trust
+host replication gpadmin 10.0.0.2/32 trust""".split('\n')
+        input_entries = 'host replication gpadmin samehost trust\nhost all gpadmin 10.0.0.2/32 trust\nhost replication gpadmin 10.0.0.2/32 trust'
+
+        result = seg_update_pg_hba.remove_stale_replication_entries(existing_entries, input_entries)
+
+        expected = """# Allow replication connections from localhost, by a user with the
+# replication privilege.
+local   replication     all                                     trust
+host    replication     all             127.0.0.1/32            trust
+host    replication     all             ::1/128                 trust
+host replication gpadmin samehost trust
+host all gpadmin 10.0.0.1/32 trust
+host all gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.2/32 trust""".split('\n')
+
+        self.assertEqual(result, expected)
+
+    def test_remove_stale_replication_entries_without_removing_samehost(self):
+        existing_entries = """# Allow replication connections from localhost, by a user with the
+# replication privilege.
+local   replication     all                                     trust
+host    replication     all             127.0.0.1/32            trust
+host    replication     all             ::1/128                 trust
+host replication gpadmin samehost trust
+host all gpadmin 10.0.0.1/32 trust
+host all gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.1/32 trust
+host replication gpadmin 10.0.0.2/32 trust""".split('\n')
+        input_entries = 'host all gpadmin 10.0.0.2/32 trust\nhost replication gpadmin 10.0.0.2/32 trust'
+
+        result = seg_update_pg_hba.remove_stale_replication_entries(existing_entries, input_entries)
+
+        expected = """# Allow replication connections from localhost, by a user with the
+# replication privilege.
+local   replication     all                                     trust
+host    replication     all             127.0.0.1/32            trust
+host    replication     all             ::1/128                 trust
+host replication gpadmin samehost trust
+host all gpadmin 10.0.0.1/32 trust
+host all gpadmin 10.0.0.2/32 trust
+host replication gpadmin 10.0.0.2/32 trust""".split('\n')
+
+        self.assertEqual(result, expected)
+

gpMgmt/bin/gppylib/utils.py

@@ -505,7 +505,12 @@ def escapeDoubleQuoteInSQLString(string, forceDoubleQuote=True):
 
 
 def Escape(query_str):
-    return pgdb.escape_string(query_str)
+    # pgdb.escape_string() is a thin wrapper around libpq's PQescapeString,
+    # and the str overload encodes with the ASCII codec, so any non-ASCII
+    # identifier or value raises UnicodeEncodeError.  Hand it UTF-8 bytes
+    # instead: libpq escapes those byte-for-byte and the result decodes back
+    # to the original text.
+    return pgdb.escape_string(query_str.encode('utf-8')).decode('utf-8')
 
 
 def escapeArrayElement(query_str):

gpMgmt/bin/gpstop

@@ -983,5 +983,16 @@ class GpStop:
                       etcd_hosts=options.etcd_hosts,
                       is_external_fts=external_fts)
 
+    # Cherry-picked from Greenplum Database (greenplum-db/gpdb-archive @ 482967c1b4),
+    # gpMgmt/bin/gpstop; the docstring is reworded.
+    @staticmethod
+    def mainOptions():
+        """
+        This method returns a dictionary instructing simple_main to check for
+        a gpstop.lock file under COORDINATOR_DATA_DIRECTORY in order to prevent
+        more than one gpstop running against the same cluster at a time.
+        """
+        return {'pidlockpath': 'gpstop.lock', 'parentpidvar': 'GPSTOPPID'}
+
 if __name__ == '__main__':
-    simple_main(GpStop.createParser, GpStop.createProgram)
+    simple_main(GpStop.createParser, GpStop.createProgram, GpStop.mainOptions())

gpMgmt/bin/minirepro

@@ -74,6 +74,11 @@ sysnslist = "('pg_toast', 'pg_bitmapindex', 'pg_catalog', 'information_schema',
 # unset search path due to CVE-2018-1058
 pgoptions = '-c optimizer=off -c gp_role=utility -c search_path='
 
+
+def escape_sql_literal(value):
+    return value.replace("'", "''")
+
+
 class MRQuery(object):
     def __init__(self):
         self.schemas = []
@@ -136,7 +141,7 @@ def dump_query(connectionInfo, query_file):
 
     with open(query_file, 'r') as query_f:
         sql_text = query_f.read()
-    query = "select pg_catalog.gp_dump_query_oids('%s')" % Escape(sql_text)
+    query = "select pg_catalog.gp_dump_query_oids('%s')" % escape_sql_literal(sql_text)
 
     toolkit_sql = PATH_PREFIX + 'toolkit.sql'
     with open(toolkit_sql, 'w') as toolkit_f:
@@ -165,6 +170,27 @@ def parse_oids(cursor, json_oids):
     if len(result.funcids) == 0:
         result.funcids = '0'
 
+    # In PG16, gp_dump_query_oids may return only view OIDs without their
+    # dependent table OIDs. Expand view dependencies so pg_dump includes
+    # the tables that views reference.
+    dep_query = "SELECT DISTINCT d.refobjid FROM pg_depend d " \
+                "JOIN pg_class c ON d.classid = 'pg_rewrite'::regclass " \
+                "JOIN pg_rewrite r ON d.objid = r.oid " \
+                "WHERE r.ev_class IN (%s) AND d.refobjid != r.ev_class " \
+                "AND d.refclassid = 'pg_class'::regclass " \
+                "AND d.refobjid NOT IN (%s)" % (result.relids, result.relids)
+    try:
+        cursor.execute(dep_query)
+        dep_oids = [str(row[0]) for row in result_iter(cursor)]
+        if dep_oids:
+            result.relids = result.relids + ',' + ','.join(dep_oids)
+    except pgdb.DatabaseError as e:
+        # The dump is still usable without the view's base tables, so warn
+        # and carry on with the OIDs gp_dump_query_oids gave us.
+        sys.stderr.write('\nWarning: could not expand view dependencies; '
+                         'the dump may be missing tables referenced by views.\n\n'
+                         + str(e) + '\n\n')
+
     cat_query = "SELECT distinct(nspname) FROM pg_class c, pg_namespace n WHERE " \
                 "c.relnamespace = n.oid AND c.oid IN (%s) " \
                 "AND n.nspname NOT IN %s" % (result.relids, sysnslist)

gpMgmt/sbin/seg_update_pg_hba.py

@@ -72,6 +72,36 @@ def remove_dup_add_entries(lines, entries):
 
     return out_entries
 
+# remove_stale_replication_entries() and is_stale_replication_entry() are
+# cherry-picked unmodified from Greenplum Database (greenplum-db/gpdb-archive @ 482967c1b4),
+# gpMgmt/sbin/seg_update_pg_hba.py.
+def remove_stale_replication_entries(existing_hba, entries):
+    final_entries = []
+    replication_entry = "replication" in entries
+
+    for line in existing_hba:
+        if line not in entries.split('\n'):
+            # check for stale replication entries on existing pg_hba.conf
+            if replication_entry and is_stale_replication_entry(line):
+                continue
+        final_entries.append(line.strip())
+
+    return final_entries
+
+def is_stale_replication_entry(line):
+    # Here considering all non-local replication entries as stale.
+    # This assumes 1:1 replication and will not work with 1:N replication.
+
+    localhost_entries = ["127.0.0.1/32", "::1/128", "local", "samehost"]
+    # return False if line is a comment or not a replication entry
+    if line.startswith('#') or "replication" not in line:
+        return False
+    # keep replication entries present for localhost
+    if any(entry in line for entry in localhost_entries):
+        return False
+
+    return True
+
 def run_pg_ctl_reload(datadir):
     name = "pg_ctl reload"
     cmd_str = "$GPHOME/bin/pg_ctl reload -D %s" % datadir
@@ -83,7 +113,8 @@ def main():
     hba_filename = options.datadir +'/pg_hba.conf'
     lines, temp_hba_filename = read_from_hba_file_and_get_empty_tempfile(hba_filename)
     out_entries = remove_dup_add_entries(lines, options.entries)
-    write_entries(out_entries, temp_hba_filename, hba_filename)
+    final_entries = remove_stale_replication_entries(out_entries, options.entries)
+    write_entries(final_entries, temp_hba_filename, hba_filename)
     run_pg_ctl_reload(options.datadir)
 
 if __name__ == "__main__":