citusdata / citus

Distributed PostgreSQL as an extension
https://www.citusdata.com
GNU Affero General Public License v3.0
10.63k stars 670 forks source link

PG17 compatibility: Fix Test Failure in multi_alter_table_add_const #7733

Open m3hm3t opened 1 week ago

m3hm3t commented 1 week ago

In earlier versions of PostgreSQL, exclusion constraints were not allowed on partitioned tables. This is why the error in your regression test (ERROR: exclusion constraints are not supported on partitioned tables) was raised in PostgreSQL 16. In PostgreSQL 17, exclusion constraints are now allowed on partitioned tables, which is why the error no longer appears when you attempt to add an exclusion constraint.

The constraint exclusion mechanism, described in the documentation, relies on CHECK constraints to decide which partitions or child tables need to be queried.

CHECK constraints

 -- Check "ADD EXCLUDE" errors out for partitioned table since the postgres does not allow it
 ALTER TABLE AT_AddConstNoName.citus_local_partitioned_table ADD EXCLUDE(partition_col WITH =);
-ERROR:  exclusion constraints are not supported on partitioned tables
 -- Check "ADD CHECK"
 SET client_min_messages TO DEBUG1;
 ALTER TABLE AT_AddConstNoName.citus_local_partitioned_table ADD CHECK (dist_col > 0);
 DEBUG:  the constraint name on the shards of the partition is too long, switching to sequential and local execution mode to prevent self deadlocks: longlonglonglonglonglonglonglonglonglonglonglo_537570f5_5_check
 DEBUG:  verifying table "longlonglonglonglonglonglonglonglonglonglonglonglonglonglongabc"
 DEBUG:  verifying table "p1"
 RESET client_min_messages;
 SELECT con.conname
     FROM pg_catalog.pg_constraint con
       INNER JOIN pg_catalog.pg_class rel ON rel.oid = con.conrelid
       INNER JOIN pg_catalog.pg_namespace nsp ON nsp.oid = connamespace
           WHERE rel.relname = 'citus_local_partitioned_table';
                      conname                      
 --------------------------------------------------
+ citus_local_partitioned_table_partition_col_excl
  citus_local_partitioned_table_check
-(1 row)
+(2 rows)
codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.60%. Comparing base (b29c332) to head (765d397).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## naisila/pg17_support #7733 +/- ## ======================================================== - Coverage 89.61% 89.60% -0.01% ======================================================== Files 274 274 Lines 59689 59689 Branches 7446 7446 ======================================================== - Hits 53490 53486 -4 - Misses 4069 4071 +2 - Partials 2130 2132 +2 ```
m3hm3t commented 6 days ago

Given that we lack in documentation right now, let me introduce this concept here as it's a good PR for it. For new things that PG allows, like an exclusion constraint on partitioned tables, first we make sure that it works as it is supposed to in Citus. If it does, our general practice is to create a new file to test it, namely pg17.sql where we add all such things. We will populate this file with other things as we go on with the support. (For this reason we have pg16.sql pg15.sql tests etc.) I will include this in the documentation for new PG support.

With that in mind, could you adjust this PR accordingly? I don't think we need to add a new output file for multi_alter_table_add_constraints. Instead, we can create pg17.sql file which will have pg17.out and pg17_0.out, and include this particular test case there. In this case, the pg17_0.out file will have the error exclusion constraints are not supported on partitioned tables.

@naisila Can you review the solution? If it is acceptable, I will update the PR to prepare it for merging into the release branch.