Support PostgreSQL 17 MERGE features in Citus with clean exceptions #7781

tejeswarm · 2024-12-06T21:43:25Z

Adapted pgmerge.sql tests from PostgreSQL community's merge.sql to Citus by converting tables into Citus local tables.
Identified two new PostgreSQL 17 MERGE features (RETURNING support and MERGE on updatable views) not yet supported by Citus.
Implemented changes to detect unsupported features and raise clean exceptions, ensuring pgmerge tests pass without diffs.
Addressed breaking changes caused by MERGE ... WHEN NOT MATCHED BY SOURCE restructuring, reducing diffs in pgmerge tests.
Segregated unsupported test cases into merge_unsupported.sql to maintain clarity and avoid large diffs in test files.
Prepared the Citus MERGE planner to handle new PostgreSQL changes, reducing remaining test discrepancies.

All merge tests now pass cleanly, with unsupported cases clearly isolated.

codecov · 2024-12-06T22:16:20Z

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

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

Additional details and impacted files

@@               Coverage Diff               @@
##             release-13.0    #7781   +/-   ##
===============================================
  Coverage                ?   89.61%           
===============================================
  Files                   ?      274           
  Lines                   ?    59727           
  Branches                ?     7450           
===============================================
  Hits                    ?    53524           
  Misses                  ?     4070           
  Partials                ?     2133

src/backend/distributed/commands/multi_copy.c

src/backend/distributed/planner/merge_planner.c

src/test/regress/expected/merge_unsupported.out

src/backend/distributed/planner/merge_planner.c

src/test/regress/expected/merge_unsupported_1.out

src/test/regress/expected/merge_unsupported_0.out

colm-mchugh

The latest update looks good, thanks for accommodating the review comments. The codecov checks are passing now, it looks like.

check-multi-1 is failing on PG14 with one diff, which looks unrelated; it does not fail when I run locally, so the test may be flappy.
https://github.com/citusdata/citus/actions/runs/12320873515/attempts/1#summary-34391423540

naisila

Great work, thank you! I don't have any major comments, just some PG17 readability ones.

src/backend/distributed/planner/merge_planner.c

src/backend/distributed/commands/multi_copy.c

src/backend/distributed/planner/merge_planner.c

src/test/regress/sql/merge_unsupported.sql

src/test/regress/expected/merge_unsupported_0.out

pgmerge.sql: This is a clone of the PostgreSQL community's merge.sql test, adapted for Citus by converting tables into Citus local tables. The expectation is that any MERGE syntax that works on PostgreSQL should work on Citus as-is, utilizing our MERGE deparser. Diffs, which primarily seem to stem fromtwo major features in MERGE introduced by the community: RETURNING support for MERGE MERGE support for updatable views Currently, Citus code does not support these features. For now, I have implemented changes to catch these cases and raise clean exceptions. With these adjustments, the pgmerge tests now pass without diffs.

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 <[email protected]>

tejeswarm force-pushed the mergediffs branch 2 times, most recently from d12ed14 to e5e1455 Compare December 6, 2024 22:12

naisila force-pushed the naisila/pg17_support branch from 1e55f40 to 1c39f95 Compare December 8, 2024 08:33

tejeswarm force-pushed the mergediffs branch 3 times, most recently from 567be8b to a229e76 Compare December 10, 2024 05:24

tejeswarm requested review from naisila and colm-mchugh December 10, 2024 15:57