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.

Sign up for GitHub

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

Jump to bottom

PG17.1 Support - Regression tests sanity #7653

Closed
45 tasks done
naisila opened this issue Jul 11, 2024 · 1 comment · Fixed by #7661
Closed
45 tasks done

PG17.1 Support - Regression tests sanity #7653

naisila opened this issue Jul 11, 2024 · 1 comment · Fixed by #7661
Assignees
@colm-mchugh @m3hm3t @naisila
Labels
pg17_support

Comments

@naisila
Copy link
Member

naisila commented Jul 11, 2024 •
edited
Loading

Using the PG17 support branch, for now we have the following failing regression tests:

PR to keep track of tests: #7661

See below a list of all current issues. Please put your PR or branch if you are currently working in an issue.

SELECT relname, rolname, relacl FROM pg_class JOIN pg_roles ON (pg_roles.oid = pg_class.relowner) WHERE relname LIKE 'trivial%' ORDER BY relname;
        relname       |   rolname   |                           relacl                           
 ---------------------+-------------+------------------------------------------------------------
  trivial_full_access | full_access | 
- trivial_postgres    | postgres    | {postgres=arwdDxt/postgres,full_access=arwdDxt/postgres}
+ trivial_postgres    | postgres    | {postgres=arwdDxtm/postgres,full_access=arwdDxtm/postgres}
 (2 rows)
 select jsonb_path_query_tz('"12:34:56"', '$.time_tz().string()');
  jsonb_path_query_tz 
 ---------------------
- "12:34:56-07:00"
+ "12:34:56-08:00"
 (1 row)

Fixed by Postgres 17.1

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

MERGE issues:

SET citus.hide_citus_dependent_objects TO on;

CREATE OR REPLACE FUNCTION
    pg_catalog.is_citus_depended_object(oid,oid)
    RETURNS bool
    LANGUAGE C
    AS 'citus', $$is_citus_depended_object$$;

MERGE INTO pg_class c
USING (SELECT 'pg_depend'::regclass AS oid) AS j
ON j.oid = c.oid
WHEN MATCHED THEN
UPDATE SET reltuples = reltuples + 1
RETURNING j.oid;

CREATE VIEW classv AS SELECT * FROM pg_class;

MERGE INTO classv c
USING pg_namespace n
ON n.oid = c.relnamespace
WHEN MATCHED AND c.oid = 'pg_depend'::regclass THEN
UPDATE SET reltuples = reltuples - 1
RETURNING c.oid;

+SSL SYSCALL error: EOF detected
+connection to server was lost
WITH foo AS (
   MERGE INTO tbl1 USING tbl2 ON (true)
   WHEN MATCHED THEN DELETE
 ) SELECT * FROM foo;
-ERROR:  MERGE not supported in WITH query
+ERROR:  WITH query "foo" does not have a RETURNING clause
 COPY (
   MERGE INTO tbl1 USING tbl2 ON (true)
   WHEN MATCHED THEN DELETE
 ) TO stdout;
-ERROR:  MERGE not supported in COPY
+ERROR:  COPY query must have a RETURNING clause

📘 probably originating from the same PG commit
🟪 probably originating from the same PG commit
🟨 probably originating from new MAINTAIN thing

  • installcheck schedule failing, cdc_wal2json test fails because wal2json package is not available yet for pg17

for now, fixed by citusdata/the-process@16d4616
EDIT: now wal2json package is available for pg17.
@naisila naisila mentioned this issue Jul 25, 2024
Merged
@naisila naisila mentioned this issue Oct 16, 2024
Merged
@naisila naisila changed the title PG17Beta2 Support - Regression tests sanity PG17.0 Support - Regression tests sanity Nov 10, 2024
@naisila naisila modified the milestone: multi_name_len Nov 13, 2024
@colm-mchugh colm-mchugh self-assigned this Nov 14, 2024
@colm-mchugh colm-mchugh mentioned this issue Nov 25, 2024
Merged
colm-mchugh added a commit that referenced this issue Nov 25, 2024
@colm-mchugh
PG17 regress sanity (#7653): Fix test diffs in columnar schedule
8bcee73
colm-mchugh added a commit that referenced this issue Nov 25, 2024
@colm-mchugh
PG17 regress sanity (#7653): Fix test diffs in columnar schedule
15b3bf0 
Add an alternative file for columnar_paths. The diff is because of PG17 commit:
postgres/postgres@f7816ae
which enabled the planner to use statistics to estimate the cardinality
of CTE scans.

Add a helper function for columnnar_chunk_filtering. The diff is due to:
postgres/postgres@fd0398fc
which changed how subquery outputs appear in expressions.
colm-mchugh added a commit that referenced this issue Dec 3, 2024
@colm-mchugh
PG17 regress sanity (#7653): Fix test diffs in columnar schedule
1d11d3d 
Add an alternative file for columnar_paths. The diff is because of PG17 commit:
postgres/postgres@f7816ae
which enabled the planner to use statistics to estimate the cardinality
of CTE scans.

Add a helper function for columnnar_chunk_filtering. The diff is due to:
postgres/postgres@fd0398fc
which changed how subquery outputs appear in expressions.
colm-mchugh added a commit that referenced this issue Dec 3, 2024
@colm-mchugh
PG17 compatibility (#7653): Fix test diffs in columnar schedule (#7768)
698699d 
This PR fixes diffs in `columnnar_chunk_filtering` and `columnar_paths`
tests.

In `columnnar_chunk_filtering` an expression `(NOT (SubPlan 1))` changed
to `(NOT (ANY (a = (SubPlan 1).col1)))`. This is due to [aPG17
commit](postgres/postgres@fd0398fc) that
improved how scalar subqueries (InitPlans) and ANY subqueries (SubPlans)
are EXPLAINed in expressions. The fix uses a helper function which
converts the PG17 format to the pre-PG17 format. It is done this way
because pre-PG17 EXPLAIN does not provide enough context to convert to
the PG17 format. The helper function can (and should) be retired when 17
becomes the minimum supported PG.

In `columnar_paths`, a merge join changed to a hash join. This is due to
[this PG17
commit](postgres/postgres@f7816ae),
which improved the PG optimizer's ability to estimate the size of a CTE
scan. The impacted query involves a CTE scan with a point predicate
`(a=123)` and before the change the CTE size was estimated to be 5000,
but with the change it is correctly (given the data in the table)
estimated to be 1, making hash join a more attractive join method. The
fix is to have an alternative goldfile for pre-PG17. I tried, but was
unable, to force a specific kind of join method using the GUCs
(`enable_nestloop`, `enable_hashjoin`, `enable_mergejoin`), but it was
not possible to obtain a consistent plan across all supported PG
versions (in some cases the join inputs switched sides).
colm-mchugh added a commit that referenced this issue Dec 5, 2024
@colm-mchugh
PG17 compatibility: fix diffs in create_index, privileges vanilla tes…
30a75ea 
…ts (#7766)

PG17 regress sanity (#7653) fix; address diffs in vanilla tests
`create_index` and `privileges`. There is a change from `permission
denied` to `must be owner of`, seen in create_index:
```
@@ -2970,21 +2970,21 @@
 REINDEX TABLE pg_toast.pg_toast_1260;
 ERROR:  permission denied for table pg_toast_1260
 REINDEX INDEX pg_toast.pg_toast_1260_index;
-ERROR:  permission denied for index pg_toast_1260_index
+ERROR:  must be owner of index pg_toast_1260_index
```
and privileges:
```
@@ -2945,41 +2945,43 @@
ERROR:  permission denied for table maintain_test
 REINDEX INDEX maintain_test_a_idx;
-ERROR:  permission denied for index maintain_test_a_idx
+ERROR:  must be owner of index maintain_test_a_idx
 REINDEX SCHEMA reindex_test;

 REINDEX INDEX maintain_test_a_idx;
+ERROR:  must be owner of index maintain_test_a_idx
 REINDEX SCHEMA reindex_test;
```

The fix updates function `RangeVarCallbackForReindexIndex()` in
`index.c` with changes made by the introduction of the [MAINTAIN
privilege in
PG17](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ecb0fd337)
to the function `RangeVarCallbackForReindexIndex()` in `indexcmds.c`.
The code is under a Postgres 17 version directive, which can be removed
when 17 becomes the oldest supported Postgres version.
@naisila naisila changed the title PG17.0 Support - Regression tests sanity PG17.1 Support - Regression tests sanity Dec 23, 2024
@naisila naisila changed the title PG17.1 Support - Regression tests sanity PG17 Support - Regression tests sanity Dec 23, 2024
@naisila naisila changed the title PG17 Support - Regression tests sanity PG17.1 Support - Regression tests sanity Dec 23, 2024
@naisila
Copy link
Member Author

naisila commented Dec 23, 2024

All these issues have been fixed in https://github.com/citusdata/citus/tree/naisila/pg17_support branch. That branch will soon be merged into release-13.0 branch, after figuring out some versioning problems. Closing this issue.

@naisila naisila closed this as completed Dec 23, 2024
naisila added a commit that referenced this issue Dec 24, 2024
@naisila @colm-mchugh
Adds PG17.1 support - Regression tests sanity (#7661)
6fed917 
This is the final commit that adds
PG17 compatibility with Citus's current capabilities.

You can use Citus community, release-13.0 branch, with PG17.1.

---------

Specifically, this commit:

- Enables PG17 in the configure script.

- Adds PG17 tests to CI using test images that have 17.1

- Fixes an upgrade test: see below for details
In `citus_prepare_upgrade()`, don't drop any_value when upgrading from
PG16+, because PG16+ has its own any_value function. Attempting to do so
results in the error seen in [pg16-pg17
upgrade](https://github.com/citusdata/citus/actions/runs/11768444117/job/32778340003?pr=7661):
```
ERROR:  cannot drop function any_value(anyelement) because it is required by the database system
CONTEXT:  SQL statement "DROP AGGREGATE IF EXISTS pg_catalog.any_value(anyelement)"
```
When 16 becomes the minimum supported Postgres version, the drop
statements can be removed.

---------

Several PG17 Compatibility commits have been merged before this final one.
All these subtasks are done #7653

See the list below:

Compilation PR: #7699
Ruleutils PR: #7725
Sister PR for tests: citusdata/the-process#159

Helpful smaller PRs:
- #7714
- #7726
- #7731
- #7732
- #7733
- #7738
- #7745
- #7747
- #7748
- #7749
- #7752
- #7755
- #7757
- #7759
- #7760
- #7761
- #7762
- #7765
- #7766
- #7768
- #7769
- #7771
- #7774
- #7776
- #7780
- #7781
- #7785
- #7788
- #7793
- #7796

---------

Co-authored-by: Colm <colmmchugh@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@colm-mchugh colm-mchugh

@m3hm3t m3hm3t

@naisila naisila

Labels
pg17_support
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adds PG17.1 support - Regression tests sanity citusdata/citus
3 participants
@colm-mchugh @m3hm3t @naisila