From 7649b01025054b612118465a14c3fc90cbbdfee5 Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Thu, 17 Sep 2026 20:46:19 +0800 Subject: [PATCH 1/5] gpstop: take the lock that stops two of them running at once gpstop calls simple_main without mainOptions, so simple_main is never told to hold a lock and nothing stops a second gpstop starting against a cluster the first one is still shutting down. GPSTOPPID appears nowhere in the tree. The machinery is all here and in use: gprecoverseg asks for gprecoverseg.lock the same way, gpmovemirrors for its own, and SimpleMainLock builds the directory and writes the pid. Only gpstop's half of it was lost. gpstop.feature has covered this the whole time and could not pass: Then a sample gpstop.lock directory is created using the background pid in coordinator_data_directory And the user runs "gpstop -a" And gpstop should print "gpstop.lock indicates that an instance of gpstop is already running" to stdout together with the scenario that a leftover empty lock directory is cleaned up rather than treated as a live lock. Split out of #1639 (originally 45a115573c5). --- gpMgmt/bin/gpstop | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/gpMgmt/bin/gpstop b/gpMgmt/bin/gpstop index ab7655597c9..e4123261f0e 100755 --- a/gpMgmt/bin/gpstop +++ b/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()) From 87bf021cbf95f9fde3417eb4657fcc91a4517fce Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Sat, 12 Sep 2026 00:22:27 +0800 Subject: [PATCH 2/5] gpMgmt: fix three defects in the shared gppylib helpers Three unrelated defects in helpers the whole of gpMgmt shares: * gpMgmt: restore the parser argument simple_main_locked needs simple_main_locked() prints the option parser's help when a ProgramArgumentValidationException asks for it, but the parser was never passed in -- the function set a local 'parser = None' instead. Every such error therefore died with AttributeError: 'NoneType' object has no attribute 'print_help' raised from inside the except block, so the handler never reached its 'exit_status = 2' and the tool exited 1 with a stack trace. Pass the parser down from simple_main_internal(), as Greenplum does. Visible as 'gprecoverseg xyz' returning 1 instead of 2. * gpMgmt: let Escape() handle non-ASCII text Escape() calls pgdb.escape_string(), whose str overload encodes with the ASCII codec, so any non-ASCII identifier or value raises UnicodeEncodeError. minirepro dies half way through writing a dump: File ".../gppylib/utils.py", line 508, in Escape return pgdb.escape_string(query_str) UnicodeEncodeError: 'ascii' codec can't encode character '\xe4' Pass UTF-8 bytes instead. libpq escapes those byte-for-byte, so the result is identical for ASCII input and decodes back to the original text otherwise. Exercised by minirepro.feature:270. * check_pid: a zombie is not a running process check_pid answers os.kill(pid, 0), which keeps succeeding for a process that has exited and has not been reaped. Nothing reaps an orphan in a container started without an init -- which is how CI runs behave, and increasingly how people run the product -- so the zombie answers yes for the life of the container. Every caller is asking whether something is still working, and for all of them a zombie has gone. Two failures in one CI run, both from this. gpstop reports segments it did shut down as failures. gpsegstop.py kills the segment and then asks check_pid whether it is gone: DBID:5 FAILED host:'cdw' datadir:'/tmp/gpmovemirrors/...' with reason:'Failed forceful termnation' Segments with errors during stop = 1 Failed to shutdown 1 of 6 segment instances and gpstop exits 2, which is what broke the teardown in gpmovemirrors.feature:40. And a utility that died once can never be run again. SimpleMainLock decides a lock is stale the same way, so after gpstop.feature:89 kills the process whose pid is in gpstop.lock, the next gpstop is still told an instance is already running -- returning 0, because simple_main returns rather than exits on a held lock, so the cluster is never stopped, the scenario's own cleanup fails, and the lock directory is still there when gpstop.feature:100 tries to make its own. Ask /proc for the state and treat Z as gone; where there is no /proc the kill() is still the best answer available. Verified against a real zombie, a live process, an unused pid and pid 0. Split out of #1639 (originally 51955ac3d8f). --- gpMgmt/bin/gppylib/commands/unix.py | 31 ++++++++++++++++++++++++++--- gpMgmt/bin/gppylib/mainUtils.py | 5 ++--- gpMgmt/bin/gppylib/utils.py | 7 ++++++- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/gpMgmt/bin/gppylib/commands/unix.py b/gpMgmt/bin/gppylib/commands/unix.py index 4708b7ac574..b3f922bd2fe 100644 --- a/gpMgmt/bin/gppylib/commands/unix.py +++ b/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 diff --git a/gpMgmt/bin/gppylib/mainUtils.py b/gpMgmt/bin/gppylib/mainUtils.py index e947639591d..beaef5cd58f 100644 --- a/gpMgmt/bin/gppylib/mainUtils.py +++ b/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") diff --git a/gpMgmt/bin/gppylib/utils.py b/gpMgmt/bin/gppylib/utils.py index 1adb1e68cdc..42fca86592e 100644 --- a/gpMgmt/bin/gppylib/utils.py +++ b/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): From fdf45122419394851cbd0f8d5b1598530f2f1df5 Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Thu, 17 Sep 2026 16:16:54 +0800 Subject: [PATCH 3/5] seg_update_pg_hba: drop replication entries for a former mirror When a mirror moves to another host -- gprecoverseg -p, gpmovemirrors -- its primary's pg_hba.conf gains a replication entry for the new host but keeps the one for the old. Nothing ever removes them, so a primary accumulates replication access for every host that has ever held its mirror. gprecoverseg_newhost.feature already asserts the opposite: And pg_hba file on primary of mirrors on "sdw5" with "all" contains no replication entries for "sdw1" And verify that only replication connection primary has is to "sdw5" so the scenario was imported without the code it tests. Bring over remove_stale_replication_entries and is_stale_replication_entry and call them from main(), together with the four unit tests that cover them -- in particular that local, samehost, 127.0.0.1/32 and ::1/128 replication entries are left alone, since those are not per-mirror. Like upstream this assumes 1:1 replication. Split out of #1639 (originally cebf3ff6b3f). --- .../test/unit/test_unit_seg_update_pg_hba.py | 84 +++++++++++++++++++ gpMgmt/sbin/seg_update_pg_hba.py | 33 +++++++- 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_seg_update_pg_hba.py b/gpMgmt/bin/gppylib/test/unit/test_unit_seg_update_pg_hba.py index 5ac6aee9249..5ddcbb17bd6 100644 --- a/gpMgmt/bin/gppylib/test/unit/test_unit_seg_update_pg_hba.py +++ b/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) + diff --git a/gpMgmt/sbin/seg_update_pg_hba.py b/gpMgmt/sbin/seg_update_pg_hba.py index 8e8b4c800f2..6e076e52bcc 100755 --- a/gpMgmt/sbin/seg_update_pg_hba.py +++ b/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__": From ed08954057a89e9020f554041abf16ddd7c6510f Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Fri, 11 Sep 2026 14:45:09 +0800 Subject: [PATCH 4/5] analyzedb: do not escape identifiers with pg.escape_string PyGreSQL's escape_string is a C function that encodes its argument as ASCII, so it raises UnicodeEncodeError on any table or schema name containing non-ASCII characters, and analyzedb exits with status 2 on a database it can otherwise analyze perfectly well. With standard_conforming_strings on -- the default -- doubling single quotes is the whole of the escaping a string literal needs, so build the literal directly. Exercised by the "analyzedb can handle the table name with special utf-8 characters" Behave scenario. Split out of #1639 (originally 0977ac5d4ef). --- gpMgmt/bin/analyzedb | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/gpMgmt/bin/analyzedb b/gpMgmt/bin/analyzedb index ff7ced099f1..a0637d54a0a 100755 --- a/gpMgmt/bin/analyzedb +++ b/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): From a56f4b98aa73b17a6f26455549498a0f578444e3 Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Fri, 11 Sep 2026 12:24:24 +0800 Subject: [PATCH 5/5] minirepro: dump the tables a view reads minirepro asks gp_dump_query_oids which objects a query touches, and for a query over a view the answer is the view alone. The resulting dump cannot reproduce the plan, because the base tables and -- more to the point -- their statistics are missing. Expand view dependencies through pg_rewrite so the tables behind a view are dumped alongside it, and build the SQL literal by doubling quotes instead of calling Escape(), which mangled non-ASCII query text. Split out of #1639 (originally 3fccb752584). --- gpMgmt/bin/minirepro | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/gpMgmt/bin/minirepro b/gpMgmt/bin/minirepro index 79d4c9fbb31..9a93389b14c 100755 --- a/gpMgmt/bin/minirepro +++ b/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)