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

Rename foreach_ macros to foreach_declared_ macros #7700

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

naisila
Copy link
Member

@naisila naisila commented Oct 6, 2024

This is prep work for successful compilation with PG17

PG17added 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_

I am separating this into its own PR since it touches many files. The main compilation PR is #7699

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_
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 97.08861% with 23 lines in your changes missing coverage. Please review.

Project coverage is 89.65%. Comparing base (15ecc37) to head (e581885).
Report is 1 commits behind head on release-13.0.

Additional details and impacted files
@@              Coverage Diff              @@
##           release-13.0    #7700   +/-   ##
=============================================
  Coverage         89.64%   89.65%           
=============================================
  Files               274      274           
  Lines             59575    59575           
  Branches           7435     7435           
=============================================
+ Hits              53407    53411    +4     
+ Misses             4036     4033    -3     
+ Partials           2132     2131    -1     

@Green-Chan
Copy link
Contributor

Have you considered rewriting the code to use PG17's macros, and remove existing macros? I understand Citus should work with previous PG versions too, so this approach probably includes defining these PG17-like macros in Citus if PG_VERSION_NUM <= PG_VERSION_16

@onurctirtir onurctirtir self-requested a review October 15, 2024 07:52
@naisila
Copy link
Member Author

naisila commented Oct 16, 2024

Have you considered rewriting the code to use PG17's macros, and remove existing macros? I understand Citus should work with previous PG versions too, so this approach probably includes defining these PG17-like macros in Citus if PG_VERSION_NUM <= PG_VERSION_16

Hey @Green-Chan please see this discussion #7651 (comment)

@naisila naisila merged commit 9d36433 into release-13.0 Oct 16, 2024
119 of 120 checks passed
@naisila naisila deleted the naisila/foreach_macros branch October 16, 2024 14:01
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.

3 participants