PG17 compatibility: Fix Test Failure in multi_alter_table_add_const

[m3hm3t]

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (release-13.0@2d843cb). Learn more about missing BASE report.

Additional details and impacted files

@@               Coverage Diff               @@
##             release-13.0    #7733   +/-   ##
===============================================
  Coverage                ?   89.65%           
===============================================
  Files                   ?      274           
  Lines                   ?    59611           
  Branches                ?     7437           
===============================================
  Hits                    ?    53443           
  Misses                  ?     4035           
  Partials                ?     2133           

[naisila]

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.

[m3hm3t]

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.

[naisila]

I think that in this test, we only need to remove the line that adds the exclusion constraint, not the rest.

[m3hm3t]

Done

[naisila]

Given that this is now something new that Citus supports, we don't necessarily need to add the exact same test that was in multi_alter_table_add_constraints_without_name.sql. We should include that particular test case for sure, i.e. ALTER TABLE ADD EXCLUDE CONSTRAINT, but we can (should) do more to test this feature.

If we consider this as a new feature in Citus (even though it doesnt need extra code work, apparently), we can create a "simpler/more readable" test, e.g. with the following steps:

Create distributed partitioned table dist_blabla
create partitioned citus local table citus_local_blabla
add exclude constraint with name in dist_blabla
check that its propagated to the workers properly
add exclude constraint with name in citus_local_blabla
check that it was added correctly
add exclusin constraints without name in both tables
verify previous command worked
alter the constraints (if meaningful?)
verify previous command worked
drop constraints
verify previous command worked
something else you can think of?

A test like the above is actually what we run when we want to make sure that something works as it should in Citus. (i.e. something is distributed properly)

[m3hm3t]

@naisila

I’ve prepared a draft of the test based on this plan. Could you review it, please?
Should this test be in the pg17.sql file?

-- Test for exclusion constraints on partitioned and distributed partitioned tables in Citus environment

-- Step 1: Create a distributed partitioned table
CREATE TABLE distributed_partitioned_table (
    id serial PRIMARY KEY,
    partition_col int NOT NULL
) DISTRIBUTED BY (id)
PARTITION BY RANGE (partition_col);

-- Step 2: Create a partitioned Citus local table
CREATE TABLE local_partitioned_table (
    id serial PRIMARY KEY,
    partition_col int NOT NULL
) PARTITION BY RANGE (partition_col);

-- Step 3: Add an exclusion constraint with a name to the distributed partitioned table
ALTER TABLE distributed_partitioned_table ADD CONSTRAINT dist_exclude_named EXCLUDE USING gist (partition_col WITH =);

-- Step 4: Verify propagation of exclusion constraint to worker nodes
\c - - :public_worker_1_host :worker_1_port
SELECT * FROM pg_dist_shard_exclude WHERE constraint_name = dist_exclude_named;

-- Step 5: Add an exclusion constraint with a name to the Citus local partitioned table
\c - - :master_host :master_port
ALTER TABLE local_partitioned_table ADD CONSTRAINT local_exclude_named EXCLUDE USING gist (partition_col WITH =);

-- Step 6: Verify the exclusion constraint on the local partitioned table
SELECT conname FROM pg_constraint WHERE conname = 'local_exclude_named';

-- Step 7: Add exclusion constraints without names to both tables
ALTER TABLE distributed_partitioned_table ADD EXCLUDE USING gist (partition_col WITH =);
ALTER TABLE local_partitioned_table ADD EXCLUDE USING gist (partition_col WITH =);

-- Step 8: Verify the unnamed exclusion constraints were added
SELECT * FROM pg_constraint WHERE conrelid = 'local_partitioned_table'::regclass;
\c - - :public_worker_1_host :worker_1_port
SELECT * FROM pg_constraint WHERE conrelid = 'distributed_partitioned_table'::regclass;

-- Step 9: Drop the exclusion constraints from both tables
\c - - :master_host :master_port
ALTER TABLE distributed_partitioned_table DROP CONSTRAINT dist_exclude_named;
ALTER TABLE local_partitioned_table DROP CONSTRAINT local_exclude_named;

-- Step 10: Verify the constraints were dropped
SELECT * FROM pg_constraint WHERE conname = 'dist_exclude_named';
SELECT * FROM pg_constraint WHERE conname = 'local_exclude_named';

-- Step 11: Clean up - Drop the tables
DROP TABLE IF EXISTS distributed_partitioned_table;
DROP TABLE IF EXISTS local_partitioned_table;

[naisila]

Very nice tests!

Should this test be in the pg17.sql file?

Yes

[m3hm3t]

tests are added

[naisila]

Mostly looks good, thanks! Added some small comments.

[naisila]

Almost forgot: let's also add partitions to the partitioned tables so that they become more meaningful partitioned tables.

You can see this example https://github.com/citusdata/citus/blob/main/src/test/regress/sql/multi_alter_table_add_constraints_without_name.sql#L730-L734

[m3hm3t]

Altered the test

[naisila]

Sometimes I reach a deadlock when the partition name is long. Steps to reproduce:

-- create partitioned table
CREATE TABLE distributed_partitioned_table (
    id serial NOT NULL,
    partition_col int NOT NULL,
    PRIMARY KEY (id, partition_col)
) PARTITION BY RANGE (partition_col);

-- add normal length name partition
CREATE TABLE distributed_partitioned_table_p1 PARTITION OF distributed_partitioned_table 
FOR VALUES FROM (1) TO (100);

-- add long name partition
CREATE TABLE distributed_partitioned_table_p2_longlonglonglonglonglonglong_name PARTITION OF distributed_partitioned_table 
FOR VALUES FROM (100) TO (200);

-- distribute
SELECT create_distributed_table('distributed_partitioned_table', 'id');

-- Add an exclusion constraint with a name to the distributed partitioned table
-- this seems to deadlock sometimes for the long name partition
SET client_min_messages TO DEBUG1;
ALTER TABLE distributed_partitioned_table ADD CONSTRAINT dist_exclude_named EXCLUDE USING btree (id WITH =, partition_col WITH =);
DEBUG:  ALTER TABLE / ADD EXCLUDE will create implicit index "dist_exclude_named" for table "distributed_partitioned_table"
DEBUG:  ALTER TABLE / ADD EXCLUDE will create implicit index "distributed_partitioned_table_p1_id_partition_col_excl" for table "distributed_partitioned_table_p1"
DEBUG:  building index "distributed_partitioned_table_p1_id_partition_col_excl" on table "distributed_partitioned_table_p1" serially
DEBUG:  index "distributed_partitioned_table_p1_id_partition_col_excl" can safely use deduplication
DEBUG:  ALTER TABLE / ADD EXCLUDE will create implicit index "distributed_partitioned_table_p2_longlong_id_partition_col_excl" for table "distributed_partitioned_table_p2_longlonglonglonglonglonglong_n"
DEBUG:  building index "distributed_partitioned_table_p2_longlong_id_partition_col_excl" on table "distributed_partitioned_table_p2_longlonglonglonglonglonglong_n" serially
DEBUG:  index "distributed_partitioned_table_p2_longlong_id_partition_col_excl" can safely use deduplication

ERROR:  canceling the transaction since it was involved in a distributed deadlock

I am still investigating this. In some local tests I did, we switch to local execution for many cases, I couldn't understand why we don't switch in this case.
Meanwhile I suggest you also add some long name tests as in https://github.com/citusdata/citus/blob/main/src/test/regress/sql/multi_alter_table_add_constraints_without_name.sql#L489-L490
Also, for you to get familiar with this deadlock issue, we have a number of functions in the codebase starting with SwitchToSequential....

[m3hm3t]

I am still investigating this. In some local tests I did, we switch to local execution for many cases, I couldn't understand why we don't switch in this case. Meanwhile I suggest you also add some long name tests as in https://github.com/citusdata/citus/blob/main/src/test/regress/sql/multi_alter_table_add_constraints_without_name.sql#L489-L490 Also, for you to get familiar with this deadlock issue, we have a number of functions in the codebase starting with SwitchToSequential....

@naisila
I added long name tests. Could you check them? Thanks.

[naisila]

We need to also address the code for the deadlock error I encountered above.

[naisila]

It seems like this is a general constraint issue, and not related to the EXCLUDE constraint, so we can merge only tests in this PR.

-- this also fails
ALTER TABLE distributed_partitioned_table ADD CONSTRAINT dist_unique_named UNIQUE(id, partition_col);

ERROR:  canceling the transaction since it was involved in a distributed deadlock

[naisila]

The test output is full of these errors of non-existing schema

[naisila]

No need to worry about the deadlock issue, it is not related to only EXCLUDE constraint, but all constraints #7799

So, only updating the test output in this PR is enough

[naisila]

LGTM