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.

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

PG17 compatibility: fix pg16 to pg17 upgrade #7788

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

colm-mchugh
Copy link
Contributor

@colm-mchugh colm-mchugh commented Dec 13, 2024

In citus_prepapre_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:

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.

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (1bee8b0) to head (eef5d84).
Report is 1 commits behind head on naisila/pg17_support.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           naisila/pg17_support    #7788      +/-   ##
========================================================
+ Coverage                 89.59%   89.61%   +0.01%     
========================================================
  Files                       274      274              
  Lines                     59732    59716      -16     
  Branches                   7451     7449       -2     
========================================================
- Hits                      53518    53514       -4     
+ Misses                     4079     4066      -13     
- Partials                   2135     2136       +1     

Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, I remember I have implemented this so sorry for that mistake :)

However we can't merge as it is as we need to create a new .sql file for the new citus version. We don't modify old .sql files. I don't remember where the ocumentation for this part is, but basically we need to add a new 13.0-1.sql file with the modified script, and also an upgrade and downgrade path for Citus https://github.com/citusdata/citus/tree/release-13.0/src/backend/distributed/sql

@colm-mchugh
Copy link
Contributor Author

However we can't merge as it is as we need to create a new .sql file for the new citus version. We don't modify old .sql files. I don't remember where the ocumentation for this part is, but basically we need to add a new 13.0-1.sql file with the modified script, and also an upgrade and downgrade path for Citus https://github.com/citusdata/citus/tree/release-13.0/src/backend/distributed/sql

The latest push reverts the modification to the existing citus sql file and adds a new one; however it probably needs #7792 to enable the pg16 -> pg17 upgrade to pass (?)

@naisila naisila force-pushed the naisila/pg17_support branch 9 times, most recently from a53615e to 1bee8b0 Compare December 23, 2024 12:49
In citus_prepapre_upgrade(), don't drop any_value when upgrading from
PG16+, because PG16+ has its own any_value function. Attempting to do
so results in an error. When 16 becomes the minimum supported Postgres
version, the drop statements can be removed.
@naisila naisila force-pushed the cmchugh/pg17-16-17-upgrade branch from b0d624f to eef5d84 Compare December 23, 2024 12:52
@naisila naisila merged commit 4f6a2cd into naisila/pg17_support Dec 23, 2024
138 of 156 checks passed
@naisila naisila deleted the cmchugh/pg17-16-17-upgrade branch December 23, 2024 13:00
naisila pushed a commit that referenced this pull request Dec 23, 2024
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.
naisila pushed a commit that referenced this pull request Dec 23, 2024
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.
naisila pushed a commit that referenced this pull request Dec 24, 2024
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.
naisila added a commit that referenced this pull request Dec 24, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants