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.

Sign up for GitHub

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

Jump to bottom

fix(postgres): sqlx prepare fails if shared_preload_libraries=pg_stat_statements #2626

Closed
wants to merge 3 commits into from
Closed

fix(postgres): sqlx prepare fails if shared_preload_libraries=pg_stat_statements #2626

wants to merge 3 commits into from

Conversation

mrl5
Copy link
Contributor

@mrl5 mrl5 commented Jul 17, 2023 •
edited

@mrl5

This comment was marked as resolved.

Sign in to view
mrl5
mrl5 commented Jul 17, 2023
View reviewed changes
tests/docker-compose.yml
@@ -183,7 +183,7 @@ services:
volumes:
- "./postgres/setup.sql:/docker-entrypoint-initdb.d/setup.sql"
command: >
-c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key
-c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key -c shared_preload_libraries=pg_stat_statements
Copy link
Contributor Author

@mrl5 mrl5 Jul 17, 2023 •
edited

Choose a reason for hiding this comment

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

I'm not sure if -c shared_preload_libraries=pg_stat_statements should be added to other postgres containers

@mrl5
fix(postgres): regression of launchbadge#1449
f0696ea 
```
error: error occurred while decoding column 0: data did not match any variant of untagged enum Explain at line 3 column 1
Error:    --> tests/postgres/macros.rs:103:15
    |
103 |     let row = sqlx::query!(r#"CALL forty_two(null)"#)
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `$crate::sqlx_macros::expand_query` which comes from the expansion of the macro `sqlx::query` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `sqlx` (test "postgres-macros") due to previous error
```
@mrl5 mrl5 requested a review from tsing July 18, 2023 15:41
@valkum valkum mentioned this pull request Jul 19, 2023
Closed
@valkum
Copy link

valkum commented Jul 20, 2023

It seems enabling JIT in Postgres also adds another field to the explain output. Are we able to come up with a serde data structure which can utilize #[serde(other)]. This would avoid adding fields for every possible setting/extension that adds additional fields to the QueryPlan output.

@mrl5
Copy link
Contributor Author

mrl5 commented Jul 20, 2023 •
edited

It seems enabling JIT in Postgres also adds another field to the explain output. Are we able to come up with a serde data structure which can utilize #[serde(other)]. This would avoid adding fields for every possible setting/extension that adds additional fields to the QueryPlan output.

hi @valkum - thanks for noticing that. Actually with this changes, sqlx will explicitly access Plan property in returned JSON. Any other property should be ignored.

So in fact even defining _query_identifier: Option<u64>, is not needed at all - I'll remove it

jplatte
jplatte reviewed Jul 21, 2023
View reviewed changes
sqlx-postgres/src/connection/describe.rs
/// The string "Utility Statement" -- returned for
/// a CALL statement
#[serde(rename = "Utility Statement")]
UtilityStatement,
UtilityStatement(String),
Copy link
Contributor

Choose a reason for hiding this comment

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

So this enum variant is never used? Its definiton was changed, but I see no other code that had to be updated because of that.

Copy link
Contributor Author

@mrl5 mrl5 Jul 22, 2023 •
edited

Choose a reason for hiding this comment

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

correct, that's my understanding how it was working before my changes as well. I can include "clean up" refactor to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so apparently it's required for compile-time checks agains DB - to match string:

error: error occurred while decoding column 0: data did not match any variant of untagged enum Explain at line 3 column 1
Error:    --> tests/postgres/macros.rs:103:15
    |
103 |     let row = sqlx::query!(r#"CALL forty_two(null)"#)
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `$crate::sqlx_macros::expand_query` which comes from the expansion of the macro `sqlx::query` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `sqlx` (test "postgres-macros") due to previous error
Error: warning: build failed, waiting for other jobs to finish...
Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101

https://github.com/launchbadge/sqlx/actions/runs/5632237064/job/15260004403?pr=2626

@mrl5 mrl5 requested a review from jplatte July 22, 2023 19:10
@mrl5
Copy link
Contributor Author

mrl5 commented Jul 29, 2023

pardon, @jplatte @abonander - is there anything that prevents merging this PR?

@jplatte
Copy link
Contributor

jplatte commented Jul 29, 2023

I'm not a maintainer of sqlx, just a curious contributor 😅

@abonander abonander mentioned this pull request Jul 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsing tsing tsing approved these changes

@jplatte jplatte Awaiting requested review from jplatte

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

postgres: sqlx prepare fails if shared_preload_libraries=pg_stat_statements
4 participants
@mrl5 @valkum @jplatte @tsing