gpcheckcat: fix the defects the Behave suite exposes

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']

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 = {}