-
Notifications
You must be signed in to change notification settings - Fork 32
Postgres 18 support #154
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
Postgres 18 support #154
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
- Coverage 90.88% 90.84% -0.05%
==========================================
Files 92 92
Lines 10437 10442 +5
==========================================
Hits 9486 9486
- Misses 951 956 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/parquet_copy_hook/copy_to.rs
Outdated
| ); | ||
| } | ||
|
|
||
| #[cfg(any(feature = "pg14", feature = "pg15", feature = "pg16", feature = "pg17"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoying there is (apparently) no #else-like support here, but I don't know Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional compilation flags might need care in another PR. It is getting more annoying now that we support 5 Postgres versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably creating a derived feature/attribute that we can define relative to others in build.rs, say, then testing that in the code location might be an easier approach to maintain.
src/parquet_copy_hook/pg_compat.rs
Outdated
| } | ||
|
|
||
| #[cfg(any(feature = "pg15", feature = "pg16", feature = "pg17"))] | ||
| #[cfg(any(feature = "pg15", feature = "pg16", feature = "pg17", feature = "pg18"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would #[cfg(not(feature="pg14"))] be legal/more succinct in this and other later cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using cfg_aliases now, made feature flags friendlier
16466c2 to
bb9bc58
Compare
7c68928 to
eed6948
Compare
eed6948 to
59eafcc
Compare
| # install Postgres | ||
| RUN sh -c 'echo "deb https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' | ||
| RUN sh -c 'echo "deb https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' && \ | ||
| sh -c 'echo "deb https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg 18" >> /etc/apt/sources.list.d/pgdg.list' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
18 is not released under main repo yet
| # install pgaudit | ||
| RUN apt-get update && apt-get -y install postgresql-${PG_MAJOR}-pgaudit | ||
| # install pgaudit extension | ||
| RUN if [ ${PG_MAJOR} != "18" ]; then \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pgaudit is not released for 18 yet but has integration branch
pgguru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the exception of the bad comment it looks good.
| pipenv install --dev | ||
| - name: Install PostgreSQL | ||
| - name: Install PostgreSQL and postgis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - name: Install PostgreSQL and postgis | |
| - name: Install PostgreSQL and pgaudit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment seems true. pgaudit is installed below.
Uh oh!
There was an error while loading. Please reload this page.