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: Resolve compilation issues #7651

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

naisila
Copy link
Member

@naisila naisila commented Jul 10, 2024

PG17 compatibility - Part 1

This PR provides successful compilation against PG17Beta2.

Notes to reviewer:

  • Review commit by commit - it's much cleaner that way.
  • When merged, Enable configure and add pg17 build test commits will be reverted. They are part of the PR to make sure that the PG17 build is actually successful - see Build for PG17 in the PR checks. These changes will be merged to the main branch later, when the PG17 support is fully added.

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 97.00240% with 25 lines in your changes missing coverage. Please review.

Project coverage is 89.69%. Comparing base (2a263fe) to head (76f60a7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7651      +/-   ##
==========================================
- Coverage   89.70%   89.69%   -0.01%     
==========================================
  Files         283      283              
  Lines       60507    60512       +5     
  Branches     7539     7539              
==========================================
  Hits        54275    54275              
- Misses       4076     4080       +4     
- Partials     2156     2157       +1     

@naisila naisila force-pushed the pg17_kickoff branch 2 times, most recently from cd85aa0 to e7e64c9 Compare July 25, 2024 10:39
PG17 added foreach_ptr, foreach_int and foreach_oid macros
Relevant PG commit
14dd0f27d7cd56ffae9ecdbe324965073d01a9ff
postgres/postgres@14dd0f2

We already have these macros, but they are different with the
PG17 ones because our macros take a DECLARED variable, whereas
the PG16 macros declare a locally-scoped loop variable themselves.

Hence I am renaming our macros to foreach_declared_
Relevant PG commit
d060e921ea5aa47b6265174c32e1128cebdbc3df
postgres/postgres@d060e92
Relevant PG commit:
89e5ef7e21812916c9cf9fcf56e45f0f74034656
postgres/postgres@89e5ef7
Relevant PG commit:
08e6344fd6423210b339e92c069bb979ba4e7cd6
postgres/postgres@08e6344
Relevant PG commit:
f696c0cd5f299f1b51e214efc55a22a782cc175d
postgres/postgres@f696c0c
Relevant PG commit:
de3600452b61d1bc3967e9e37e86db8956c8f577
postgres/postgres@de36004
Relevant PG commit:
4f622503d6de975ac87448aea5cea7de4bc140d5
postgres/postgres@4f62250
Relevant PG commit:
012460ee93c304fbc7220e5b55d9d0577fc766ab
postgres/postgres@012460e
Relevant PG commit:
50c67c2019ab9ade8aa8768bfe604cd802fe8591
postgres/postgres@50c67c2
Relevant PG commit:
509199587df73f06eda898ae13284292f4ae573a
postgres/postgres@5091995
Relevant PG commit:
75680c3d805e2323cd437ac567f0677fdfc7b680
postgres/postgres@75680c3
Relevant PG commit:
0294df2f1f842dfb0eed79007b21016f486a3c6c
postgres/postgres@0294df2
Relevant PG commit:
5de890e3610d5a12cdaea36413d967cf5c544e20
postgres/postgres@5de890e
…ON_COUNT

Relevant PG commit:
a6be0600ac3b71dda8277ab0fcbe59ee101ac1ce
postgres/postgres@a6be060
Relevant PG commits:
28f3915b73f75bd1b50ba070f56b34241fe53fd1
postgres/postgres@28f3915

ab355e3a88de745607f6dd4c21f0119b5c68f2ad
postgres/postgres@ab355e3

024c521117579a6d356050ad3d78fdc95e44eefa
postgres/postgres@024c521
@naisila naisila marked this pull request as ready for review July 25, 2024 11:53
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider using the macros from pg17 (e.g. removing the local variables)? Would it be feasible?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still have to support PG15 and PG16. One way to do it would be to change Citus's current definition of these macros such that they are identical to PG17's definitions, and then iterate over all uses and remove the local variables.
Do you think that is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would require to add the macro definition for PG < 17. Both (the current / alternative) seem feasible options, and both require quite some editing. You already implemented one of them. It seems OK to me.

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