From 10d62d50eaec9561e0968c57ed5e2c24490ccca3 Mon Sep 17 00:00:00 2001 From: Mehmet YILMAZ Date: Mon, 22 Sep 2025 15:50:32 +0300 Subject: [PATCH] =?UTF-8?q?Stabilize=20table=5Fchecks=20across=20PG15?= =?UTF-8?q?=E2=80=93PG18:=20switch=20to=20pg=5Fconstraint,=20remove=20dupe?= =?UTF-8?q?s,=20exclude=20NOT=20NULL=20(#8140)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DESCRIPTION: Stabilize table_checks across PG15–PG18: switch to pg_constraint, remove dupes, exclude NOT NUL fixes #8138 fixes #8131 **Problem** ```diff 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 2025-08-18 12:26:51.991598284 +0000 +++ /__w/citus/citus/src/test/regress/results/multi_create_table_constraints.out.modified 2025-08-18 12:26:52.004598519 +0000 @@ -403,22 +403,30 @@ 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_col_check | CHECK other_col >= 100 + check_example_other_col_check | CHECK other_col >= 100 + check_example_other_col_check | CHECK 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) + check_example_other_other_col_check | CHECK abs(other_other_col) >= 100 + check_example_other_other_col_check | CHECK abs(other_other_col) >= 100 + check_example_other_other_col_check | CHECK abs(other_other_col) >= 100 + check_example_other_other_col_check | CHECK abs(other_other_col) >= 100 +(10 rows) ``` On PostgreSQL 18, `NOT NULL` is represented as a cataloged constraint and surfaces through `information_schema.check_constraints`. https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f Our helper view `table_checks` (built on `information_schema.check_constraints` + `constraint_column_usage`) started returning: * Extra `…_not_null` rows (noise for our tests) * Duplicate rows for real CHECKs due to the one-to-many join via `constraint_column_usage` * Occasional literal formatting differences (e.g., dates) coming from the information\_schema deparser ### What changed 1. **Rewrite `table_checks` to use system catalogs directly** We now select only expression-based, table-level constraints—excluding NOT NULL—by filtering on `contype <> 'n'` and requiring `conbin IS NOT NULL`. This yields the same effective set as real CHECKs while remaining future-proof against non-CHECK constraint types. ```sql CREATE OR REPLACE VIEW table_checks AS SELECT c.conname AS "Constraint", 'CHECK ' || -- drop a single pair of outer parens if the deparser adds them regexp_replace(pg_get_expr(c.conbin, c.conrelid, true), '^\((.*)\)$', '\1') AS "Definition", c.conrelid AS relid FROM pg_catalog.pg_constraint AS c WHERE c.contype <> 'n' -- drop NOT NULL (PG18) AND c.conbin IS NOT NULL -- only expression-bearing constraints (i.e., CHECKs) AND c.conrelid <> 0 -- table-level only (exclude domains) ORDER BY "Constraint", "Definition"; ``` Why this filter? * `contype <> 'n'` excludes PG18’s NOT NULL rows. * `conbin IS NOT NULL` restricts to expression-backed constraints (CHECKs); PK/UNIQUE/FK/EXCLUSION don’t have `conbin`. * `conrelid <> 0` removes domain constraints. 2. **Add a PG18-specific regression test for `contype = 'n'`** New test (`pg18_not_null_constraints`) verifies: * Coordinator tables have `n` rows for NOT NULL (columns `a`, `c`), * A worker shard has matching `n` rows, * Dropping a NOT NULL on the coordinator propagates to shards (count goes from 2 → 1), * `table_checks` *never* reports NOT NULL, but does report a real CHECK added for the test. --- ### Why this works (PG15–PG18) * **Stable source of truth:** Directly reads `pg_constraint` instead of `information_schema`. * **No duplicates:** Eliminates the `constraint_column_usage` join, removing multiplicity. * **No NOT NULL noise:** PG18’s `contype = 'n'` is filtered out by design. * **Deterministic text:** Uses `pg_get_expr` and strips a single outer set of parentheses for consistent output. --- ### Impact on tests * Removes spurious `…_not_null` entries and duplicate `checky_…` rows (e.g., in `multi_name_lengths` and similar). * Existing expected files stabilize without adding brittle normalizations. * New PG18 test asserts correct catalog behavior and Citus propagation while remaining a no-op on earlier PG versions. --- --- .../expected/multi_test_catalog_views.out | 22 ++- src/test/regress/expected/pg18.out | 163 ++++++++++++++++++ src/test/regress/expected/pg18_0.out | 9 + src/test/regress/multi_schedule | 1 + .../regress/sql/multi_test_catalog_views.sql | 22 ++- src/test/regress/sql/pg18.sql | 129 ++++++++++++++ 6 files changed, 328 insertions(+), 18 deletions(-) create mode 100644 src/test/regress/expected/pg18.out create mode 100644 src/test/regress/expected/pg18_0.out create mode 100644 src/test/regress/sql/pg18.sql diff --git a/src/test/regress/expected/multi_test_catalog_views.out b/src/test/regress/expected/multi_test_catalog_views.out index 8c255f94e..65ca8f637 100644 --- a/src/test/regress/expected/multi_test_catalog_views.out +++ b/src/test/regress/expected/multi_test_catalog_views.out @@ -70,15 +70,19 @@ SELECT "name" AS "Column", "relid" FROM table_attrs; -CREATE VIEW table_checks AS -SELECT cc.constraint_name AS "Constraint", - ('CHECK ' || regexp_replace(check_clause, '^\((.*)\)$', '\1')) AS "Definition", - format('%I.%I', ccu.table_schema, ccu.table_name)::regclass::oid AS relid -FROM information_schema.check_constraints cc, - information_schema.constraint_column_usage ccu -WHERE cc.constraint_schema = ccu.constraint_schema AND - cc.constraint_name = ccu.constraint_name -ORDER BY cc.constraint_name ASC; +CREATE OR REPLACE VIEW table_checks AS +SELECT + c.conname AS "Constraint", + 'CHECK ' || + -- drop a single pair of outer parens if the deparser adds them + regexp_replace(pg_get_expr(c.conbin, c.conrelid, true), '^\((.*)\)$', '\1') + AS "Definition", + c.conrelid AS relid +FROM pg_catalog.pg_constraint AS c +WHERE c.contype <> 'n' -- drop NOT NULL + AND c.conbin IS NOT NULL -- only things with an expression (i.e., CHECKs) + AND c.conrelid <> 0 -- table-level (exclude domain checks) +ORDER BY "Constraint", "Definition"; CREATE VIEW index_attrs AS WITH indexoid AS ( diff --git a/src/test/regress/expected/pg18.out b/src/test/regress/expected/pg18.out new file mode 100644 index 000000000..f5d35a47e --- /dev/null +++ b/src/test/regress/expected/pg18.out @@ -0,0 +1,163 @@ +-- +-- PG18 +-- +SHOW server_version \gset +SELECT substring(:'server_version', '\d+')::int >= 18 AS server_version_ge_18 +\gset +\if :server_version_ge_18 +\else +\q +\endif +-- PG17-specific tests go here. +-- +-- Purpose: Verify PG18 behavior that NOT NULL constraints are materialized +-- as pg_constraint rows with contype = 'n' on both coordinator and +-- worker shards. Also confirm our helper view (table_checks) does +-- NOT surface NOT NULL entries. +-- https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f +CREATE SCHEMA pg18_nn; +SET search_path TO pg18_nn; +-- Local control table +DROP TABLE IF EXISTS nn_local CASCADE; +NOTICE: table "nn_local" does not exist, skipping +CREATE TABLE nn_local( + a int NOT NULL, + b int, + c text NOT NULL +); +-- Distributed table +DROP TABLE IF EXISTS nn_dist CASCADE; +NOTICE: table "nn_dist" does not exist, skipping +CREATE TABLE nn_dist( + a int NOT NULL, + b int, + c text NOT NULL +); +SELECT create_distributed_table('nn_dist', 'a'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +-- Coordinator: count NOT NULL constraint rows +SELECT 'local_n_count' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = 'pg18_nn.nn_local'::regclass +GROUP BY contype +ORDER BY contype; + label | contype | count +--------------------------------------------------------------------- + local_n_count | n | 2 +(1 row) + +SELECT 'dist_n_count' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = 'pg18_nn.nn_dist'::regclass +GROUP BY contype +ORDER BY contype; + label | contype | count +--------------------------------------------------------------------- + dist_n_count | n | 2 +(1 row) + +-- Our helper view should exclude NOT NULL +SELECT 'table_checks_local_count' AS label, count(*) +FROM public.table_checks +WHERE relid = 'pg18_nn.nn_local'::regclass; + label | count +--------------------------------------------------------------------- + table_checks_local_count | 0 +(1 row) + +SELECT 'table_checks_dist_count' AS label, count(*) +FROM public.table_checks +WHERE relid = 'pg18_nn.nn_dist'::regclass; + label | count +--------------------------------------------------------------------- + table_checks_dist_count | 0 +(1 row) + +-- Add a real CHECK to ensure table_checks still reports real checks +ALTER TABLE nn_dist ADD CONSTRAINT nn_dist_check CHECK (b IS DISTINCT FROM 42); +SELECT 'table_checks_dist_with_real_check' AS label, count(*) +FROM public.table_checks +WHERE relid = 'pg18_nn.nn_dist'::regclass; + label | count +--------------------------------------------------------------------- + table_checks_dist_with_real_check | 1 +(1 row) + +-- === Worker checks === +\c - - - :worker_1_port +SET client_min_messages TO WARNING; +SET search_path TO pg18_nn; +-- Pick one heap shard of nn_dist in our schema +SELECT format('%I.%I', n.nspname, c.relname) AS shard_regclass +FROM pg_class c +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'pg18_nn' + AND c.relname LIKE 'nn_dist_%' + AND c.relkind = 'r' +ORDER BY c.relname +LIMIT 1 +\gset +-- Expect: 2 NOT NULL rows (a,c) + 1 CHECK row on the shard +SELECT 'worker_shard_n_count' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = :'shard_regclass'::regclass +GROUP BY contype +ORDER BY contype; + label | contype | count +--------------------------------------------------------------------- + worker_shard_n_count | c | 1 + worker_shard_n_count | n | 2 +(2 rows) + +-- table_checks on shard should hide NOT NULL +SELECT 'table_checks_worker_shard_count' AS label, count(*) +FROM public.table_checks +WHERE relid = :'shard_regclass'::regclass; + label | count +--------------------------------------------------------------------- + table_checks_worker_shard_count | 1 +(1 row) + +-- Drop one NOT NULL on coordinator; verify propagation +\c - - - :master_port +SET search_path TO pg18_nn; +ALTER TABLE nn_dist ALTER COLUMN c DROP NOT NULL; +-- Re-check on worker: NOT NULL count should drop to 1 +\c - - - :worker_1_port +SET search_path TO pg18_nn; +SELECT 'worker_shard_n_after_drop' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = :'shard_regclass'::regclass +GROUP BY contype +ORDER BY contype; + label | contype | count +--------------------------------------------------------------------- + worker_shard_n_after_drop | c | 1 + worker_shard_n_after_drop | n | 1 +(2 rows) + +-- And on coordinator +\c - - - :master_port +SET search_path TO pg18_nn; +SELECT 'dist_n_after_drop' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = 'pg18_nn.nn_dist'::regclass +GROUP BY contype +ORDER BY contype; + label | contype | count +--------------------------------------------------------------------- + dist_n_after_drop | c | 1 + dist_n_after_drop | n | 1 +(2 rows) + +-- cleanup +RESET client_min_messages; +RESET search_path; +DROP SCHEMA pg18_nn CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to table pg18_nn.nn_local +drop cascades to table pg18_nn.nn_dist diff --git a/src/test/regress/expected/pg18_0.out b/src/test/regress/expected/pg18_0.out new file mode 100644 index 000000000..b682ea190 --- /dev/null +++ b/src/test/regress/expected/pg18_0.out @@ -0,0 +1,9 @@ +-- +-- PG18 +-- +SHOW server_version \gset +SELECT substring(:'server_version', '\d+')::int >= 18 AS server_version_ge_18 +\gset +\if :server_version_ge_18 +\else +\q diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 0b1d4ce67..98bc01ac5 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -68,6 +68,7 @@ test: pg14 test: pg15 test: pg15_jsonpath detect_conn_close test: pg17 pg17_json +test: pg18 test: drop_column_partitioned_table test: tableam diff --git a/src/test/regress/sql/multi_test_catalog_views.sql b/src/test/regress/sql/multi_test_catalog_views.sql index bb1442edf..249b2d274 100644 --- a/src/test/regress/sql/multi_test_catalog_views.sql +++ b/src/test/regress/sql/multi_test_catalog_views.sql @@ -71,15 +71,19 @@ SELECT "name" AS "Column", "relid" FROM table_attrs; -CREATE VIEW table_checks AS -SELECT cc.constraint_name AS "Constraint", - ('CHECK ' || regexp_replace(check_clause, '^\((.*)\)$', '\1')) AS "Definition", - format('%I.%I', ccu.table_schema, ccu.table_name)::regclass::oid AS relid -FROM information_schema.check_constraints cc, - information_schema.constraint_column_usage ccu -WHERE cc.constraint_schema = ccu.constraint_schema AND - cc.constraint_name = ccu.constraint_name -ORDER BY cc.constraint_name ASC; +CREATE OR REPLACE VIEW table_checks AS +SELECT + c.conname AS "Constraint", + 'CHECK ' || + -- drop a single pair of outer parens if the deparser adds them + regexp_replace(pg_get_expr(c.conbin, c.conrelid, true), '^\((.*)\)$', '\1') + AS "Definition", + c.conrelid AS relid +FROM pg_catalog.pg_constraint AS c +WHERE c.contype <> 'n' -- drop NOT NULL + AND c.conbin IS NOT NULL -- only things with an expression (i.e., CHECKs) + AND c.conrelid <> 0 -- table-level (exclude domain checks) +ORDER BY "Constraint", "Definition"; CREATE VIEW index_attrs AS WITH indexoid AS ( diff --git a/src/test/regress/sql/pg18.sql b/src/test/regress/sql/pg18.sql new file mode 100644 index 000000000..e18e7455b --- /dev/null +++ b/src/test/regress/sql/pg18.sql @@ -0,0 +1,129 @@ +-- +-- PG18 +-- +SHOW server_version \gset +SELECT substring(:'server_version', '\d+')::int >= 18 AS server_version_ge_18 +\gset + +\if :server_version_ge_18 +\else +\q +\endif + +-- PG17-specific tests go here. +-- + +-- Purpose: Verify PG18 behavior that NOT NULL constraints are materialized +-- as pg_constraint rows with contype = 'n' on both coordinator and +-- worker shards. Also confirm our helper view (table_checks) does +-- NOT surface NOT NULL entries. +-- https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f + +CREATE SCHEMA pg18_nn; +SET search_path TO pg18_nn; + +-- Local control table +DROP TABLE IF EXISTS nn_local CASCADE; +CREATE TABLE nn_local( + a int NOT NULL, + b int, + c text NOT NULL +); + +-- Distributed table +DROP TABLE IF EXISTS nn_dist CASCADE; +CREATE TABLE nn_dist( + a int NOT NULL, + b int, + c text NOT NULL +); + +SELECT create_distributed_table('nn_dist', 'a'); + +-- Coordinator: count NOT NULL constraint rows +SELECT 'local_n_count' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = 'pg18_nn.nn_local'::regclass +GROUP BY contype +ORDER BY contype; + +SELECT 'dist_n_count' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = 'pg18_nn.nn_dist'::regclass +GROUP BY contype +ORDER BY contype; + +-- Our helper view should exclude NOT NULL +SELECT 'table_checks_local_count' AS label, count(*) +FROM public.table_checks +WHERE relid = 'pg18_nn.nn_local'::regclass; + +SELECT 'table_checks_dist_count' AS label, count(*) +FROM public.table_checks +WHERE relid = 'pg18_nn.nn_dist'::regclass; + +-- Add a real CHECK to ensure table_checks still reports real checks +ALTER TABLE nn_dist ADD CONSTRAINT nn_dist_check CHECK (b IS DISTINCT FROM 42); + +SELECT 'table_checks_dist_with_real_check' AS label, count(*) +FROM public.table_checks +WHERE relid = 'pg18_nn.nn_dist'::regclass; + +-- === Worker checks === +\c - - - :worker_1_port +SET client_min_messages TO WARNING; +SET search_path TO pg18_nn; + +-- Pick one heap shard of nn_dist in our schema +SELECT format('%I.%I', n.nspname, c.relname) AS shard_regclass +FROM pg_class c +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE n.nspname = 'pg18_nn' + AND c.relname LIKE 'nn_dist_%' + AND c.relkind = 'r' +ORDER BY c.relname +LIMIT 1 +\gset + +-- Expect: 2 NOT NULL rows (a,c) + 1 CHECK row on the shard +SELECT 'worker_shard_n_count' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = :'shard_regclass'::regclass +GROUP BY contype +ORDER BY contype; + +-- table_checks on shard should hide NOT NULL +SELECT 'table_checks_worker_shard_count' AS label, count(*) +FROM public.table_checks +WHERE relid = :'shard_regclass'::regclass; + +-- Drop one NOT NULL on coordinator; verify propagation +\c - - - :master_port +SET search_path TO pg18_nn; + +ALTER TABLE nn_dist ALTER COLUMN c DROP NOT NULL; + +-- Re-check on worker: NOT NULL count should drop to 1 +\c - - - :worker_1_port +SET search_path TO pg18_nn; + +SELECT 'worker_shard_n_after_drop' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = :'shard_regclass'::regclass +GROUP BY contype +ORDER BY contype; + +-- And on coordinator +\c - - - :master_port +SET search_path TO pg18_nn; + +SELECT 'dist_n_after_drop' AS label, contype, count(*) +FROM pg_constraint +WHERE conrelid = 'pg18_nn.nn_dist'::regclass +GROUP BY contype +ORDER BY contype; + +-- cleanup +RESET client_min_messages; +RESET search_path; +DROP SCHEMA pg18_nn CASCADE;