Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PG17 compatibility: Fix Test Failure in multi_alter_table_add_const #7733

Open
wants to merge 2 commits into
base: naisila/pg17_support
Choose a base branch
from

Conversation

m3hm3t
Copy link
Contributor

@m3hm3t m3hm3t commented Nov 11, 2024

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)

@m3hm3t m3hm3t self-assigned this Nov 11, 2024
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@                   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 m3hm3t marked this pull request as ready for review November 11, 2024 20:48
@m3hm3t m3hm3t changed the title Fix Test Failure in multi_alter_table_add_const in PG17 PG17 compatibility: Fix Test Failure in multi_alter_table_add_const in PG17 Nov 12, 2024
Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 naisila changed the title PG17 compatibility: Fix Test Failure in multi_alter_table_add_const in PG17 PG17 compatibility: Fix Test Failure in multi_alter_table_add_const Nov 12, 2024
@m3hm3t m3hm3t marked this pull request as draft November 15, 2024 08:39
@m3hm3t m3hm3t force-pushed the m3hm3t/multi_alter_table_add_const branch from 8e51831 to 304400c Compare November 15, 2024 10:49
update

update

.

move the tests

update

update

update

style
@m3hm3t m3hm3t force-pushed the m3hm3t/multi_alter_table_add_const branch from fa4854a to 765d397 Compare November 15, 2024 15:35
@m3hm3t
Copy link
Contributor Author

m3hm3t commented Nov 15, 2024

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.

@m3hm3t m3hm3t marked this pull request as ready for review November 15, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants