Repository navigation
gpcheckcat: fix the defects the Behave suite exposes - #2182
MisterRaindrop wants to merge 1 commit into
Conversation
| pg_amproc | ||
| where | ||
| amproc :: oid in ( | ||
| 6140, 6141, 6142, 6143, 6144, 6145, 6146, |
There was a problem hiding this comment.
Legacy hash-function OIDs don’t match Cloudberry. they omit 19 of Cloudberry’s 29 legacy functions. For example, cdblegacyhash_date is OID 7187, and cdblegacyhash_bool is 7194. Those policies are classified as nonlegacy, causing false positives and missed mixed policies. The same incorrect list appears in both generated diagnostic queries.
There was a problem hiding this comment.
Confirmed. Only 6140-6149 of Greenplum's list are legacy cdbhash functions in Cloudberry, and some of the rest are unrelated (6154 is multirange_gist_consistent). On a demo cluster, a table distributed by cdbhash_date_ops next to one using the default int4 opclass passed, and with gp_use_legacy_hashops = on a database holding only that date table was reported as having non-legacy tables.
The functions are now looked up by name (starts_with(proname, 'cdblegacyhash_') in pg_catalog) in one shared CTE, which the check and both queries of gpcheckcat.distpolicy.sql use. Both cases above are now reported correctly, and the generated file lists the date and bool tables as legacy and the int4 one as non-legacy.
| row = rows[0] | ||
| n_legacy_dist_class = row[0] | ||
| n_total_dist_class = row[1] | ||
| GV.checkStatus = False |
There was a problem hiding this comment.
Detected policy errors still return exit status 0. it set GV.checkStatus = False without calling setError(). Both GUC-mismatch branches do the same. With no other errors, gpcheckcat reports failure but exits successfully, so automation misses it.
There was a problem hiding this comment.
Confirmed: each of the three errors exited 0 with -R mix_distribution_policy. They now call setError(ERROR_NOREPAIR), and gpcheckcat exits 3. Unit tests cover the mixed policy and both GUC mismatches.
|
|
||
| GV.checkStatus = True | ||
|
|
||
| if n_total_dist_class - n_legacy_dist_class > 0 and gp_use_legacy_hashops == "on": |
There was a problem hiding this comment.
GUC-mismatch errors reference a nonexistent diagnostic file. the condition instructs users to run gpcheckcat.distpolicy.sql, but only the mixed-policy branch generates it. Generate it for these mismatch cases too.
There was a problem hiding this comment.
Confirmed. Both GUC-mismatch branches now write gpcheckcat.distpolicy.sql before printing the message that refers to it, and a unit test checks that each one does. On the cluster, the file is now there after both errors.
1301edf to
2fa5206
Compare
| else: | ||
| if (n_legacy_dist_class == 0 or n_legacy_dist_class == n_total_dist_class): | ||
| #if this condition is true then we dont have mix distribution policy | ||
| gp_use_legacy_hashops = fetch_guc_value("gp_use_legacy_hashops") |
There was a problem hiding this comment.
We should add
if gp_use_legacy_hashops is None:
return
otherwise, printDistPolicyMsg() overwrites the failure status set by the failed GUC lookup and the summary then incorrectly reports “PASSED” and raises an AssertionError because the error code is still nonzero.
There was a problem hiding this comment.
Confirmed. With the show gp_use_legacy_hashops lookup made to fail on a demo cluster, gpcheckcat printed the execution error, then SUMMARY REPORT: PASSED and Found no catalog issue, and died on the assert(GV.retcode == SUCCESS) in checkcatReport. Fixed in cc8a16a: checkMixDistPolicy now returns when the lookup gives None, as you suggested. The same run now ends with SUMMARY REPORT: FAILED, lists mix_distribution_policy as a failed test, and exits 3. A unit test makes the lookup query raise and checks that the check stays failed; it fails with the previous code.
2fa5206 to
cc8a16a
Compare
* gpcheckcat: add the mix_distribution_policy check
Back-port from Greenplum a check Cloudberry was missing.
mix_distribution_policy reports tables whose distribution policy mixes
legacy and non-legacy hash opclasses, and cross-checks the result
against the gp_use_legacy_hashops GUC so the operator is told which of
the two states is inconsistent.
Add the SQL fixtures the corresponding Behave scenarios load.
Three things are changed from Greenplum's version. Greenplum finds the
legacy cdbhash functions by a list of OIDs (6140-6171). Only ten of
them are the same in Cloudberry, where cdblegacyhash_date is 7187 and
cdblegacyhash_bool is 7194, and several of the others are unrelated
functions. A table distributed by a legacy date or bool opclass was
therefore counted as non-legacy, so a mixed policy went unreported and
gp_use_legacy_hashops = on was reported as wrong when it was not. The
functions are now found by name in pg_catalog, in the check and in
both queries of the diagnostic file. The check also set checkStatus
without calling setError(), so gpcheckcat printed the error and still
exited 0. Each of the three errors now calls it. And the two
GUC-mismatch errors told the user to run gpcheckcat.distpolicy.sql
without writing it. They now write it too.
Greenplum's unit tests for the check mock a psycopg2 cursor, but this
gpcheckcat reaches the database through PyGreSQL (conn.query() and
getresult()) and never opens a cursor, so three of them failed and the
rest passed without exercising the code. They are rewritten against
conn.query(), with tests added for a mixed policy, a clean one, a GUC
that is read successfully, the lookup by name, and the exit status and
diagnostic file of each error.
Greenplum's companion ao_lastrownums check is deliberately not brought
over: it reads pg_attribute_encoding.lastrownums, a column Cloudberry's
catalog does not have, so the query errors out on every run and
gpcheckcat reports failure on a freshly created database.
* gpcheckcat: give pg_shdescription its TableMainColumn entry
TableMainColumn maps a catalog to the column that identifies the object
an entry belongs to. Greenplum has a row for pg_description and one for
pg_shdescription; Cloudberry has only the first. Without it,
processMissingDuplicateEntryResult() falls through to getOidFromPK(),
which looks the entry up by
SELECT oid FROM pg_shdescription WHERE ...
and pg_shdescription has no oid column, so missing_extraneous died with
Execution error: ERROR: column "oid" does not exist
LINE 4: SELECT oid FROM pg_shdescription
* gpcheckcat: attribute an AO table's visimap to the table in the owner check
checkOwners() compares each relation's owner across segments, and
folds an append-optimized table's auxiliary relations into the table
they belong to, so a mismatch is reported, and repaired, once, on the
table. It knew the segment relation, the block directory and the block
directory's index, but not the visimap relation or its index. Those
were reported as relations of their own, and the repair script tried
ALTER TABLE "pg_aoseg"."pg_aovisimap_17032" OWNER TO "gpadmin";
which the server refuses:
ERROR: permission denied: "pg_aovisimap_17032" is a system catalog
so "gpcheckcat -R owner" on an append-optimized table whose owner
differs on a segment produced a repair that could not run. Match the
visimap and its index, and the block directory's index by the table it
indexes. The repair is then the ALTER TABLE on the table itself, which
carries its auxiliary relations with it.
The feature-file half of this change -- dropping the two ao_lastrownums
scenarios and tagging the -x session GUC scenario, which Cloudberry no
longer supports -- edits the Behave suite as apache#2170 aligns it, so it goes
with that PR.
Split out of apache#1639 (originally 0bfc1d7).
87b9876 to
c13d0bb
Compare
What does this PR do?
Three gpcheckcat fixes that the gpMgmt Behave suite (#2170) exposes. They are in one commit, as they were in #1639. The commit message gives the full reasoning.
mix_distribution_policycheck. This back-ports from Greenplum a check that Cloudberry was missing. It reports tables whose distribution policy mixes legacy and non-legacy hash opclasses, and it cross-checks the result againstgp_use_legacy_hashops. The SQL fixtures its Behave scenarios load are added too. Greenplum's companionao_lastrownumscheck is deliberately not brought over: it readspg_attribute_encoding.lastrownums, which Cloudberry's catalog does not have.checkOwners()folds an AO table's auxiliary relations into the table, so an owner mismatch is reported, and repaired, once, on the table. It did not know the visimap relation or its index. The repair script it generated therefore also ranALTER TABLE "pg_aoseg"."pg_aovisimap_<oid>" OWNER TO ..., and the server refuses that withpermission denied: "pg_aovisimap_<oid>" is a system catalog. Sogpcheckcat -R ownerproduced a repair that could not run.pg_shdescriptionitsTableMainColumnentry. Without the entry,missing_extraneouslooked shared descriptions up byoid, whichpg_shdescriptiondoes not have, and it died withcolumn "oid" does not exist.Split out of #1639 (originally 0bfc1d7, by @tuhaihe).
One change from #1639: Greenplum's unit tests for the new check mock a psycopg2 cursor. This gpcheckcat reaches the database through PyGreSQL (
conn.query()andgetresult()) and never opens a cursor. As a result, three of those tests failed, and the rest passed without running the code they were meant to test. I rewrote them againstconn.query()and added tests for a mixed policy, a consistent one, and a GUC that is read successfully.What stays with #2170: in #1639 this commit also edited the Behave suite. It dropped the two
ao_lastrownumsscenarios, tagged the-xsession GUC scenario (which Cloudberry no longer supports)@not_implemented, and added--tags ~@not_implementedto the gpcheckcat job in the workflow. Those edits apply to the suite as #2170 aligns it, so they are made in #2170.This is one of five PRs that the remaining fixes from #1639 are grouped into, alongside #2178, #2180, #2183 (gprecoverseg) and #2184 (gprecoverseg Greenplum ports).
Test Plan
Rocky 9 build image, built from #2170's tree plus #2177 and #2178 (
767f12685a4andb971d481862, which the demo cluster needs) and #2180, with a demo cluster created the way the Behave workflow creates it. I tookgpcheckcat.featureas #2170 now has it, with the edits described above, and removed the eight@needs_fixtags that point at this change. I then ran it with CI's arguments (--tags ~@extended --tags ~@not_implemented).$GPHOME.gpcheckcat.featureBoth runs skip one scenario, the
-xone.The eight scenarios that fail in A are the eight #2170 tags:
mix_distribution_policyscenarios (fix 1). The check does not exist without this PR.ALTER TABLE. I reproduced this by hand: without the fix, the repair SQL alterst,t_idx,pg_aovisimap_<oid>andpg_aovisimap_<oid>_index, and fails on the third. With it, onlytandt_idxare altered and the script succeeds.Unit tests (
python3 -m unittest, withPYTHONPATHset the waymake -C gpMgmt/bin unitdevelsets it):test_unit_gpcheckcatpasses with this PR, all 41 tests. Without it, the ten new tests error out because the functions they test do not exist. The wholegppylibunit suite, with this PR and #2180 applied together, has no new failures; the tests that fail in it fail the same way without these PRs.#2170 tags these scenarios
@needs_fixpointing at this change. Whichever PR lands second removes those tags.Type of Change
mix_distribution_policycheckChecklist