PG17 compatibility: Fix Test Failure in multi_name_lengths multi_create_table_constraints

[m3hm3t]

Relevant PG commit:
e59fcbd712c777eb2987d7c9ad542a7e817954ec

CI link https://github.com/citusdata/citus/actions/runs/11844794788

 \c - - :public_worker_1_host :worker_1_port
 SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.name_lengths_225002'::regclass ORDER BY 1 DESC, 2 DESC;
                            Constraint                            |                                       Definition                                        
 -----------------------------------------------------------------+-----------------------------------------------------------------------------------------
- nl_checky_1234567890123456789012345678901234567_b16df46d_225002 | CHECK (date_col_12345678901234567890123456789012345678901234567890 >= '01-01-2014'::date)
+ nl_checky_1234567890123456789012345678901234567_b16df46d_225002 | CHECK date_col_12345678901234567890123456789012345678901234567890 >= '01-01-2014'::date
 (1 row)

diff -dU10 -w /__w/citus/citus/src/test/regress/expected/multi_create_table_constraints.out /__w/citus/citus/src/test/regress/results/multi_create_table_constraints.out
--- /__w/citus/citus/src/test/regress/expected/multi_create_table_constraints.out.modified	2024-11-05 09:50:21.744669730 +0000
+++ /__w/citus/citus/src/test/regress/results/multi_create_table_constraints.out.modified	2024-11-05 09:50:21.756669718 +0000
@@ -402,22 +402,22 @@
 SELECT "Column", "Type", "Definition" FROM index_attrs WHERE
     relid = 'check_example_partition_col_key_365068'::regclass;
     Column     |  Type   |  Definition   
 ---------------+---------+---------------
  partition_col | integer | partition_col
 (1 row)
 
 SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_example_365068'::regclass;
              Constraint              |            Definition             
 -------------------------------------+-----------------------------------
- check_example_other_col_check       | CHECK (other_col >= 100)
- check_example_other_other_col_check | CHECK (abs(other_other_col) >= 100)
+ check_example_other_col_check       | CHECK other_col >= 100
+ check_example_other_other_col_check | CHECK abs(other_other_col) >= 100
 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@               Coverage Diff               @@
##             release-13.0    #7726   +/-   ##
===============================================
  Coverage                ?   89.64%           
===============================================
  Files                   ?      274           
  Lines                   ?    59583           
  Branches                ?     7436           
===============================================
  Hits                    ?    53412           
  Misses                  ?     4040           
  Partials                ?     2131           

[naisila]

It seems that these two normalization lines are specific to the column name, hence they don't take care of the following failure in multi_create_table_constraints:
https://github.com/citusdata/citus/actions/runs/11775359161/attempts/1#summary-32795798881

SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_example_365068'::regclass;
              Constraint              |            Definition             
 -------------------------------------+-----------------------------------
- check_example_other_col_check       | CHECK (other_col >= 100)
- check_example_other_other_col_check | CHECK (abs(other_other_col) >= 100)
+ check_example_other_col_check       | CHECK other_col >= 100
+ check_example_other_other_col_check | CHECK abs(other_other_col) >= 100
 (2 rows)

So we have date_col, int_col, other_col and abs(other_other_col)
I think we can rename these columns to make the normalization rules apply to them?

Example:
date_col_ to column_date
int_col_ to column_int
other_col to column_other
And then have two normalization lines: one for CHECK column_... and one for CHECK abs(...?
There might be a better way, just thoughts from the top of my head. But definitely in this PR we should fix both multi_create_table_constraints and multi_name_lengths tests

[m3hm3t]

I tried to combine the rules into a single, more comprehensive one, but it didn't work out. A more general rule could potentially work, but it carries the risk of affecting other tests as well.

[naisila]

I understand.

In that case, instead of 4 normalization rules, how about we look at this the other way around: add the parantheses where they are missing. In this case, .out files won't change at all, instead just a single normalization line will take care of it.

Something like the following should work:

# PG 17 Removes outer parentheses from CHECK constraints
# we add them back for pg15,pg16 compatibility
# e.g. change CHECK other_col >= 100 to CHECK (other_col >= 100)
s/\| CHECK ([a-zA-Z])(.*)/| CHECK \(\1\2\)/g

40c34fd

[m3hm3t]

Thanks for the suggestion, @naisila. Adding parentheses for PostgreSQL 15 and 16 compatibility does seem like a straightforward solution that would reduce the number of normalization rules and keep the .out files consistent.

However, I'm wondering if we should align with PostgreSQL 17's standards instead, since it's the latest version and likely reflects the future direction. Adapting the tests to follow PostgreSQL 17's format might help reduce the need for version-specific handling as we move forward.

Let me know your thoughts. In the meantime, I'll try out the suggested normalization and update the PR.

[naisila]

You are right, it would be ideal to go with PG17's output. But normalization lines slow down the tests so in this case it would be more performant to switch 4 normalization lines for 1, especially since the difference between versions doesn't reflect a feature or improvement in PG17 (the difference between versions only reflects a "style" change let's say). Other than performance, 1 normalization line is also cleaner than 4.
When we drop PG16 support, whichever way we choose, we would have to drop the normalization lines anyway. So, in conclusion, I think we can align with pg16/pg15 standards in this case.

[naisila]

Hey @m3hm3t is there something else blocking the update of this PR?

[naisila]

I understand.

In that case, instead of 4 normalization rules, how about we look at this the other way around: add the parantheses where they are missing. In this case, .out files won't change at all, instead just a single normalization line will take care of it.

Something like the following should work:

# PG 17 Removes outer parentheses from CHECK constraints
# we add them back for pg15,pg16 compatibility
# e.g. change CHECK other_col >= 100 to CHECK (other_col >= 100)
s/\| CHECK ([a-zA-Z])(.*)/| CHECK \(\1\2\)/g

40c34fd