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: Check whether table AM is default #7747

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

naisila
Copy link
Member

@naisila naisila commented Nov 14, 2024

PG 17 added support for DEFAULT in ALTER TABLE .. SET ACCESS METHOD

Relevant PG commit:
d61a6cad6418f643a5773352038d0dfe5d3535b8
postgres/postgres@d61a6ca

In that case, name in AlterTableCmd->name would be null.
Add a null check here to avoid crash.

Note to reviewer: I have opened the pull request against naisila/pg17_columnar_issue as the fix from that PR is necessary to see what this PR is fixing. Naturally, this fix will be merged after #7738 has been merged

@naisila naisila force-pushed the naisila/pg17_columnar_crash branch 2 times, most recently from c1373a6 to dea2306 Compare November 14, 2024 13:50
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

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

Additional details and impacted files
@@               Coverage Diff               @@
##             release-13.0    #7747   +/-   ##
===============================================
  Coverage                ?   89.64%           
===============================================
  Files                   ?      274           
  Lines                   ?    59582           
  Branches                ?     7436           
===============================================
  Hits                    ?    53413           
  Misses                  ?     4038           
  Partials                ?     2131           

@naisila naisila requested a review from onurctirtir November 14, 2024 13:55
@naisila naisila force-pushed the naisila/pg17_columnar_issue branch from a162533 to 5ae350e Compare November 18, 2024 13:53
Base automatically changed from naisila/pg17_columnar_issue to release-13.0 November 18, 2024 14:27
PG 17 added support for DEFAULT in ALTER TABLE .. SET ACCESS METHOD

Relevant PG commit:
d61a6cad6418f643a5773352038d0dfe5d3535b8
postgres/postgres@d61a6ca

In that case, name in AlterTableCmd would be null.
Add a null check here to avoid crash.

(cherry picked from commit 71b9974)
(cherry picked from commit 52d3329)
@naisila naisila force-pushed the naisila/pg17_columnar_crash branch from dea2306 to 3121717 Compare November 18, 2024 14:31
@naisila naisila merged commit a93887b into release-13.0 Nov 18, 2024
120 of 121 checks passed
@naisila naisila deleted the naisila/pg17_columnar_crash branch November 18, 2024 15:09
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