From d2696ff2dd42edec3872ce89525a36e10bd0678e Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Fri, 11 Sep 2026 12:23:52 +0800 Subject: [PATCH] gpcheckcat: fix the defects the Behave suite exposes * 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 #2170 aligns it, so it goes with that PR. Split out of #1639 (originally 0bfc1d76642). --- gpMgmt/bin/gpcheckcat | 189 +++++++++++++++++- .../gppylib/test/unit/test_unit_gpcheckcat.py | 171 ++++++++++++++++ .../create_legacy_hash_ops_tables.sql | 37 ++++ .../create_non_legacy_hashops_tables.sql | 30 +++ 4 files changed, 426 insertions(+), 1 deletion(-) create mode 100644 gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_legacy_hash_ops_tables.sql create mode 100644 gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_non_legacy_hashops_tables.sql diff --git a/gpMgmt/bin/gpcheckcat b/gpMgmt/bin/gpcheckcat index 00c2e4b21f3..2ddc49e5762 100755 --- a/gpMgmt/bin/gpcheckcat +++ b/gpMgmt/bin/gpcheckcat @@ -1075,9 +1075,12 @@ def checkOwners(): a.rolname, m.rolname as coordinator_rolname from gp_dist_random('pg_class') r join pg_class c on (c.oid = r.oid) + left join pg_index i on (c.oid = i.indexrelid) left join pg_appendonly ao on (c.oid = ao.segrelid or c.oid = ao.blkdirrelid or - c.oid = ao.blkdiridxid) + c.oid = ao.visimaprelid or + i.indrelid = ao.blkdirrelid or + i.indrelid = ao.visimaprelid) left join pg_class o on (o.oid = ao.relid or o.reltoastrelid = c.oid) join pg_authid a on (a.oid = r.relowner) @@ -1948,6 +1951,180 @@ def checkOrphanedToastTables(): issue_type="orphaned_toast_tables", description='Repairing orphaned TOAST tables') +# The mix_distribution_policy check below -- fetch_guc_value(), +# generateDistPolicyQueryFile(), checkMixDistPolicy() and their helpers -- +# is cherry-picked from Greenplum Database (greenplum-db/gpdb-archive @ 482967c1b4), +# gpMgmt/bin/gpcheckcat. +def fetch_guc_value(guc): + qry = ''' + show {} + '''.format(guc) + try: + conn = connect2(GV.cfg[GV.coordinator_dbid]) + curs = conn.query(qry) + rows = curs.getresult() + guc_value = rows[0][0] + return guc_value + + except Exception as e: + setError(ERROR_NOREPAIR) + GV.checkStatus = False + myprint('[ERROR] executing test: mix_distribution_policy') + myprint(' Execution error: ' + str(e)) + +# The opclasses of the families that hold a legacy cdbhash function. +# Greenplum lists those functions by OID, but most of Cloudberry's have +# other OIDs, so they are found by name, which both ports share. +LEGACY_OPCLASS_OIDS_CTE = '''legacy_opclass_oids(oid_array) as ( + select + array_agg(oid) + from + pg_opclass + where + opcfamily in ( + select + amprocfamily + from + pg_amproc + where + amproc in ( + select + oid + from + pg_proc + where + pronamespace = 'pg_catalog' :: regnamespace + and starts_with(proname, 'cdblegacyhash_') + ) + ) + )''' + +def generateDistPolicyQueryFile(): + + query_sql = ''' + -- all tables that use legacy policy: + with %(legacy)s + select + localoid :: regclass :: text as "Legacy Policy" + from + gp_distribution_policy, + legacy_opclass_oids + where + policytype = 'p' + and distclass :: oid[] && oid_array; + + -- all tables that don't use any legacy policy: + with %(legacy)s +select + localoid :: regclass :: text as "Non Legacy Policy" +from + gp_distribution_policy, + legacy_opclass_oids +where + policytype = 'p' + and not (distclass :: oid[] && oid_array); + ''' % {'legacy': LEGACY_OPCLASS_OIDS_CTE} + filename = 'gpcheckcat.distpolicy.sql' + + if not os.path.exists(filename) : + try: + with open(filename, 'w') as fp: + fp.write(query_sql + "\n") + except Exception as e: + logger.warning('Unable to generate verify file for {}'.format(filename)) + + +# Test to check if there are tables that use both legacy opclass/non legacy opclass +# in distribution policy +def checkMixDistPolicy() : + + qry = ''' + with %(legacy)s, + all_hash_ops(dc) as ( + select + distinct unnest(distclass :: oid[]) + from + gp_distribution_policy + ) + select + count(1) filter( + where + array[x.dc] && oid_array + ) as n_legacy_dist_class, + count(1) as n_total_dist_class + from + all_hash_ops x, + legacy_opclass_oids y; + ''' % {'legacy': LEGACY_OPCLASS_OIDS_CTE} + + try: + conn = connect2(GV.cfg[GV.coordinator_dbid]) + curs = conn.query(qry) + rows = curs.getresult() + + if rows: + row = rows[0] + n_legacy_dist_class = row[0] + n_total_dist_class = row[1] + GV.checkStatus = False + + if n_legacy_dist_class > 0 and n_total_dist_class > n_legacy_dist_class : + setError(ERROR_NOREPAIR) + generateDistPolicyQueryFile() + #if this condition is true then we have mix distribution Policy + myprint( + '[ERROR]: Found tables created using both legacy and non legacy hashops' + ' in distribution policy.' + 'Please run the gpcheckcat.distpolicy.sql file to list the tables.' + ) + 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") + # The lookup failed and has reported it; printDistPolicyMsg + # would mark the check as passed. + if gp_use_legacy_hashops is None: + return + printDistPolicyMsg(gp_use_legacy_hashops, + n_legacy_dist_class, + n_total_dist_class + ) + + except Exception as e: + setError(ERROR_NOREPAIR) + GV.checkStatus = False + myprint('[ERROR] executing test: mix_distribution_policy') + myprint(' Execution error: ' + str(e)) + +def printDistPolicyMsg(gp_use_legacy_hashops,n_legacy_dist_class, n_total_dist_class): + + GV.checkStatus = True + + if n_total_dist_class - n_legacy_dist_class > 0 and gp_use_legacy_hashops == "on": + myprint( + '[ERROR]: GUC gp_use_legacy_hashops is on.' + ' all newly created tables will use legacy hash ops by default for hash distributed table, ' + 'but there are tables using non-legacy hash ops in the cluster. ' + 'Please run the gpcheckcat.distpolicy.sql file to list the tables.' + ) + setError(ERROR_NOREPAIR) + generateDistPolicyQueryFile() + GV.checkStatus = False + + elif n_legacy_dist_class == 0 and gp_use_legacy_hashops == "off": + GV.checkStatus = True + + elif n_legacy_dist_class > 0 and gp_use_legacy_hashops == "off" : + myprint( + '[ERROR]: GUC gp_use_legacy_hashops is off.' + ' all newly created tables will use non legacy hash ops by default for hash distributed table, ' + 'but there are tables using legacy hash ops in the cluster. ' + 'Please run the gpcheckcat.distpolicy.sql file to list the tables.' + ) + setError(ERROR_NOREPAIR) + generateDistPolicyQueryFile() + GV.checkStatus = False + ############################################################################ # Help populating repair part for all checked types @@ -2082,7 +2259,16 @@ all_checks = { "version": 'main', "order": 15, "online": False + }, + "mix_distribution_policy": + { + "description": "Check for tables that use legacy opclass in distribution policy", + "fn": lambda: checkMixDistPolicy(), + "version": 'main', + "order": 17, + "online": True } + } @@ -2366,6 +2552,7 @@ TableMainColumn['pg_type_encoding'] = ['typid', 'pg_type'] TableMainColumn['pg_window'] = ['winfnoid', 'pg_proc'] TableMainColumn['pg_password_history'] = ['passhistroleid', 'pg_authid'] TableMainColumn['pg_description'] = ['objoid', 'pg_description'] +TableMainColumn['pg_shdescription'] = ['objoid', 'pg_shdescription'] # Table with OID (special case), these OIDs are known to be inconsistent TableMainColumn['pg_attrdef'] = ['adrelid', 'pg_class'] diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_gpcheckcat.py b/gpMgmt/bin/gppylib/test/unit/test_unit_gpcheckcat.py index ccdfb03a7ad..d5bfd0e9497 100755 --- a/gpMgmt/bin/gppylib/test/unit/test_unit_gpcheckcat.py +++ b/gpMgmt/bin/gppylib/test/unit/test_unit_gpcheckcat.py @@ -411,6 +411,177 @@ def count_joins(): self.num_batches += 1 self.num_joins = 0 self.num_starts = 0 + + # gpcheckcat talks to the database through PyGreSQL: connect2() returns a + # pg connection, conn.query() runs the statement and getresult() fetches + # the rows. Greenplum's versions of these tests mock a psycopg2 cursor, + # which this gpcheckcat never opens. + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_with_error_on_execution(self, mock_connect2): + mock_connect2.return_value.query.side_effect = Exception("Simulated error during execution") + + self.subject.checkMixDistPolicy() + + self.assertEqual(mock_connect2.return_value.query.call_count, 1) + self.subject.setError.assert_any_call(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_exception_on_connect(self, mock_connect2): + mock_connect2.side_effect = Exception("Simulated error during connection") + + self.subject.checkMixDistPolicy() + + self.assertEqual(mock_connect2.call_count, 1) + self.subject.setError.assert_any_call(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_exception_on_getresult(self, mock_connect2): + mock_connect2.return_value.query.return_value.getresult.side_effect = Exception("Simulated error fetching rows") + + self.subject.checkMixDistPolicy() + + self.assertEqual(mock_connect2.return_value.query.call_count, 1) + self.subject.setError.assert_any_call(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.generateDistPolicyQueryFile') + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_reports_mixed_policy(self, mock_connect2, mock_generate): + # 1 legacy distribution opclass out of 3 in use + mock_connect2.return_value.query.return_value.getresult.return_value = [(1, 3)] + + self.subject.checkMixDistPolicy() + + mock_generate.assert_called_once_with() + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.fetch_guc_value', return_value='off') + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_passes_without_legacy_policy(self, mock_connect2, mock_fetch): + mock_connect2.return_value.query.return_value.getresult.return_value = [(0, 3)] + + self.subject.checkMixDistPolicy() + + mock_fetch.assert_called_once_with('gp_use_legacy_hashops') + self.assertTrue(self.subject.GV.checkStatus) + + @patch('gpcheckcat.connect2') + def test_fetch_guc_value_returns_value(self, mock_connect2): + mock_connect2.return_value.query.return_value.getresult.return_value = [('on',)] + + self.assertEqual(self.subject.fetch_guc_value("gp_use_legacy_hashops"), 'on') + self.assertIn('show gp_use_legacy_hashops', mock_connect2.return_value.query.call_args[0][0]) + self.assertTrue(self.subject.GV.checkStatus) + + @patch('gpcheckcat.connect2') + def test_fetch_guc_value_with_error_on_execution(self, mock_connect2): + mock_connect2.return_value.query.side_effect = Exception("Simulated error during execution") + + self.assertIsNone(self.subject.fetch_guc_value("gp_use_legacy_hashops")) + + self.assertEqual(mock_connect2.return_value.query.call_count, 1) + self.subject.setError.assert_any_call(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.connect2') + def test_fetch_guc_value_exception_on_connect(self, mock_connect2): + mock_connect2.side_effect = Exception("Simulated error during connection") + + self.assertIsNone(self.subject.fetch_guc_value("gp_use_legacy_hashops")) + + self.assertEqual(mock_connect2.call_count, 1) + self.subject.setError.assert_any_call(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.connect2') + def test_fetch_guc_value_exception_on_getresult(self, mock_connect2): + mock_connect2.return_value.query.return_value.getresult.side_effect = Exception("Simulated error fetching rows") + + self.assertIsNone(self.subject.fetch_guc_value("gp_use_legacy_hashops")) + + self.assertEqual(mock_connect2.return_value.query.call_count, 1) + self.subject.setError.assert_any_call(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + def test_generateDistPolicyQueryFile(self): + self.subject.generateDistPolicyQueryFile() + + self.assertTrue(os.path.exists('gpcheckcat.distpolicy.sql')) + with open('gpcheckcat.distpolicy.sql', 'r') as fp: + generated_query = fp.read() + + self.assertIn('as "Legacy Policy"', generated_query) + self.assertIn('as "Non Legacy Policy"', generated_query) + self.assertEqual(generated_query.count(self.subject.LEGACY_OPCLASS_OIDS_CTE), 2) + + # Most of Cloudberry's legacy cdbhash functions have OIDs other than + # Greenplum's, cdblegacyhash_date (7187) and cdblegacyhash_bool (7194) + # among them, so the functions are found by name. + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_finds_legacy_functions_by_name(self, mock_connect2): + mock_connect2.return_value.query.return_value.getresult.return_value = [(0, 0)] + with patch('gpcheckcat.fetch_guc_value', return_value='off'): + self.subject.checkMixDistPolicy() + + qry = mock_connect2.return_value.query.call_args[0][0] + self.assertIn(self.subject.LEGACY_OPCLASS_OIDS_CTE, qry) + self.assertIn("starts_with(proname, 'cdblegacyhash_')", qry) + self.assertNotIn('6140', qry) + + @patch('gpcheckcat.generateDistPolicyQueryFile') + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_mixed_policy_sets_error(self, mock_connect2, mock_generate): + mock_connect2.return_value.query.return_value.getresult.return_value = [(1, 3)] + + self.subject.checkMixDistPolicy() + + self.subject.setError.assert_called_once_with(self.subject.ERROR_NOREPAIR) + + # When the gp_use_legacy_hashops lookup fails, the check must stay failed. + @patch('gpcheckcat.connect2') + def test_checkMixDistPolicy_guc_lookup_fails(self, mock_connect2): + def query(qry): + if 'show gp_use_legacy_hashops' in qry: + raise Exception('lookup failed') + result = Mock() + result.getresult.return_value = [(0, 3)] + return result + mock_connect2.return_value.query.side_effect = query + + self.subject.checkMixDistPolicy() + + self.subject.setError.assert_called_once_with(self.subject.ERROR_NOREPAIR) + self.assertFalse(self.subject.GV.checkStatus) + + # Both GUC mismatches point the user at gpcheckcat.distpolicy.sql, and + # must make gpcheckcat exit with an error. + @patch('gpcheckcat.generateDistPolicyQueryFile') + def test_printDistPolicyMsg_guc_mismatch(self, mock_generate): + for guc, n_legacy, n_total in (('on', 0, 3), ('off', 3, 3)): + with self.subTest(guc=guc): + self.subject.setError.reset_mock() + mock_generate.reset_mock() + + self.subject.printDistPolicyMsg(guc, n_legacy, n_total) + + self.subject.setError.assert_called_once_with(self.subject.ERROR_NOREPAIR) + mock_generate.assert_called_once_with() + self.assertFalse(self.subject.GV.checkStatus) + + @patch('gpcheckcat.generateDistPolicyQueryFile') + def test_printDistPolicyMsg_consistent_guc(self, mock_generate): + for guc, n_legacy, n_total in (('on', 3, 3), ('off', 0, 3)): + with self.subTest(guc=guc): + self.subject.setError.reset_mock() + + self.subject.printDistPolicyMsg(guc, n_legacy, n_total) + + self.subject.setError.assert_not_called() + mock_generate.assert_not_called() + self.assertTrue(self.subject.GV.checkStatus) + class Global(): def __init__(self): self.opt = {} diff --git a/gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_legacy_hash_ops_tables.sql b/gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_legacy_hash_ops_tables.sql new file mode 100644 index 00000000000..4cd9a745a60 --- /dev/null +++ b/gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_legacy_hash_ops_tables.sql @@ -0,0 +1,37 @@ +-- Cherry-picked from Greenplum Database, unmodified. +-- Source: greenplum-db/gpdb-archive @ 482967c1b4 +-- gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_legacy_hash_ops_tables.sql + +set gp_use_legacy_hashops = 1; + +create table t_old(a int, b int, c int) distributed by (a, b); +create table t1_old(a int, b int, c int) distributed by (a, b); +create table t_replicate_old(a int , b int) distributed replicated; +create table t_random_old(a int , b int) distributed randomly; + +CREATE TABLE rank_old (id int, rank int, year int, gender + char(1), count int) +DISTRIBUTED BY (id) +PARTITION BY RANGE (year) +( START (2006) END (2016) EVERY (1), + DEFAULT PARTITION extra ); + + +CREATE OR REPLACE FUNCTION random_between(low INT ,high INT) + RETURNS INT AS +$$ +BEGIN + RETURN floor(random()* (high-low + 1) + low); +END; +$$ language 'plpgsql' STRICT; + +insert into rank_old +select i, i, random_between(2005, 2017), 'g', i +from generate_series(1, 100000)i; + +-- some special characters in column names +create table t_space("a col" int); +create table t_dot("a.col" int); +create table t_dash("a-col" int); +create table t_multispecial("a col" int, "b.col" int, "c-col" int) distributed by ("a col", "b.col", "c-col"); + diff --git a/gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_non_legacy_hashops_tables.sql b/gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_non_legacy_hashops_tables.sql new file mode 100644 index 00000000000..bdd5a23a75e --- /dev/null +++ b/gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_non_legacy_hashops_tables.sql @@ -0,0 +1,30 @@ +-- Cherry-picked from Greenplum Database, unmodified. +-- Source: greenplum-db/gpdb-archive @ 482967c1b4 +-- gpMgmt/test/behave/mgmt_utils/steps/data/gpcheckcat/create_non_legacy_hashops_tables.sql + +set gp_use_legacy_hashops = 0; + +create table t_new(a int, b int, c int) distributed by (a, b); +create table t1_new(a int, b int, c int) distributed by (a, b); +create table t_replicate_new(a int , b int) distributed replicated; +create table t_random_new(a int , b int) distributed randomly; + +CREATE TABLE rank_new (id int, rank int, year int, gender + char(1), count int) +DISTRIBUTED BY (id) +PARTITION BY RANGE (year) +( START (2006) END (2016) EVERY (1), + DEFAULT PARTITION extra ); + + +CREATE OR REPLACE FUNCTION random_between(low INT ,high INT) + RETURNS INT AS +$$ +BEGIN + RETURN floor(random()* (high-low + 1) + low); +END; +$$ language 'plpgsql' STRICT; + +insert into rank_new +select i, i, random_between(2005, 2017), 'g', i +from generate_series(1, 100000)i;