Repository navigation
gpMgmt: fix five small defects the Behave suite exposes - #2180
Open
MisterRaindrop wants to merge 5 commits into
Open
MisterRaindrop wants to merge 5 commits into
MisterRaindrop wants to merge 5 commits into
Conversation
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 apache#1639 (originally 45a1155).
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 apache#1639 (originally 51955ac).
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 apache#1639 (originally cebf3ff).
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 apache#1639 (originally 0977ac5).
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 apache#1639 (originally 3fccb75).
This was referenced Oct 24, 2026
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Five small gpMgmt fixes that the gpMgmt Behave suite (#2170) exposes in gpstop, the shared gppylib helpers, seg_update_pg_hba, analyzedb and minirepro. Each is its own commit, and each commit message gives that fix's full reasoning.
simple_mainwithout the lock options, so nothing stopped a second gpstop from starting against a cluster the first one was still shutting down. gprecoverseg and gpmovemirrors already take their locks this way; only gpstop's was lost.simple_main_locked()never received the option parser, so every argument-validation error raisedAttributeErrorfrom inside the handler, and the tool exited 1 with a stack trace instead of 2.Escape()encoded its input as ASCII, so any non-ASCII identifier raisedUnicodeEncodeError.check_pid()treated a zombie as a running process. In a container without an init (which is how CI runs Behave), that makes gpstop report segments it did stop as failures, and makes a utility that died once look as if it is still running forever.pg_hba.confkeeps the replication entry for the old host forever.gprecoverseg_newhost.featurealready asserts the entries are removed; the code it tests had not been brought over. This commit brings it over with Greenplum's four unit tests.pg.escape_string.escape_stringencodes as ASCII, so analyzedb exits 2 on any table or schema name with non-ASCII characters. The literal is now built by doubling single quotes.gp_dump_query_oidsreturns only the view, so the dump could not reproduce the plan. View dependencies are now expanded throughpg_rewrite, and the literal is built withoutEscape().Split out of #1639 (originally 45a1155, 51955ac, cebf3ff, 0977ac5 and 3fccb75, by @tuhaihe).
Why one PR for five fixes: #1639 carried 19 fixes. Opening one PR per fix would leave a long queue of tiny PRs for reviewers, so the remaining ones are grouped by area into five PRs (this one, #2178 for gpinitsystem, and #2183 for gprecoverseg, #2184 for the gprecoverseg Greenplum ports, and #2182 for gpcheckcat). The commits are kept separate on purpose, so please merge this with "Rebase and merge" rather than squash, and each fix stays one revertable commit.
Test Plan
Rocky 9 build image, built from #2170's tree plus #2177 and #2178 (
767f12685a4andb971d481862, which the demo cluster needs), with a demo cluster created the way the Behave workflow creates it. I removed the@needs_fixtags that point at these five fixes, then ran each feature with CI's arguments. Run A has the fixes reversed and the tree reinstalled into$GPHOME; run B has them applied and installed.gpstop.feature(--tags ~@concourse_cluster)minirepro.feature(--tags ~@extended)analyzedb.featureThe scenarios that fail in A:
check_pid()treating the killed lock holder as alive).Escape()in fix 2, plus fix 5's literal).#2170's CI run before it tagged anything failed on the same scenarios. It also failed minirepro "Dump database objects of only functions", which is skipped in both runs here: it needs
pg_dump --function-oids(#2175) and is tagged for that PR. Fix 3 is asserted bygprecoverseg_newhost.feature, which needs a multi-host cluster (@concourse_cluster), so it is covered here by its unit tests. Fix 2'scheck_pid()andsimple_main_locked()parts are also exercised by the gprecoverseg and gpmovemirrors runs for #2183, which had this PR applied.Unit tests (
python3 -m unittest, withPYTHONPATHset the waymake -C gpMgmt/bin unitdevelsets it):test_unit_seg_update_pg_hba: the four new tests fail without the fix (the functions they test do not exist) and pass with it.gppylibunit suite, with this PR and gpcheckcat: fix the defects the Behave suite exposes #2182 applied together, has no new failures. The tests that fail in it fail the same way without these PRs; I have not looked into them.#2170 tags these scenarios
@needs_fixpointing at these commits. Whichever PR lands second removes those tags.Type of Change
Checklist