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 diffs in create_index, privileges vanilla tests #7766

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

colm-mchugh
Copy link
Contributor

@colm-mchugh colm-mchugh commented Nov 25, 2024

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 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.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@               Coverage Diff               @@
##             release-13.0    #7766   +/-   ##
===============================================
  Coverage                ?   89.65%           
===============================================
  Files                   ?      274           
  Lines                   ?    59583           
  Branches                ?     7436           
===============================================
  Hits                    ?    53419           
  Misses                  ?     4032           
  Partials                ?     2132           

@colm-mchugh colm-mchugh changed the title Fix diffs in create_index, privileges vanilla tests PG17 compatibility: fix diffs in create_index, privileges vanilla tests Nov 26, 2024
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.

Unfortunately we cannot disable either of these GUCs.
See #7764 (comment)

@naisila naisila force-pushed the naisila/pg17_support branch from c396ce6 to a12026b Compare December 2, 2024 14:32
@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-vanilla-errormsgs branch from bcc1b97 to 4215663 Compare December 3, 2024 16:29
@colm-mchugh
Copy link
Contributor Author

Unfortunately we cannot disable either of these GUCs. See #7764 (comment)

@naisila The updated PR has a code change to index.c, which is needed because there is a function in there that mirrors a corresponding function in Postgres.

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.

LGTM, nice fix!

Reminder to rebase to release-13.0 and drop the enable configure ... commit before merging.

@colm-mchugh colm-mchugh changed the base branch from naisila/pg17_support to release-13.0 December 5, 2024 09:39
@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-vanilla-errormsgs branch from 4215663 to 693e7aa Compare December 5, 2024 09:43
@colm-mchugh colm-mchugh merged commit 30a75ea into release-13.0 Dec 5, 2024
118 of 119 checks passed
@colm-mchugh colm-mchugh deleted the cmchugh/pg17-vanilla-errormsgs branch December 5, 2024 10:03
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