-
Notifications
You must be signed in to change notification settings - Fork 176
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
Replace PQconnectdb()
by PQconnectdbParams()
#384
Conversation
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.
This would fix my issue--left a suggestion for covering all cases.
bin/pgut/pgut-fe.c
Outdated
@@ -62,7 +62,7 @@ setup_workers(int num_workers) | |||
if (username && username[0]) | |||
appendStringInfo(&buf, "user=%s ", username); | |||
if (password && password[0]) | |||
appendStringInfo(&buf, "password=%s ", password); | |||
appendStringInfo(&buf, "password='%s' ", password); |
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.
That'll work for most passwords (and mine), but this approach won't work for passwords that contain single quotes.
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING-KEYWORD-VALUE
To make it bulletproof: when wrapping with single quotes, we'd need to scan the string and:
- Prefix any backslash with a backslash.
- Prefix any single quote with a backslash.
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.
That makes sense 👍. I see couple of ways of resolving other cases:
- The simpler approach is to copy the function appendConnStrVal().
libpgfeutils
started sharing it only after Postgres 9.6. - IMHO the proper approach would be to use PQconnectdbParams(), which doesn't require connection parameters values and which is used by most of the PostgreSQL client applications.
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 commit 135df8b I replaced call of PQconnectdb()
by PQconnectdbParams()
. This should be more solid approach to pass password and other connection options, which doesn't require escaping value strings.
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.
Much appreciated! I don't see anything glaringly wrong, but I have very little C experience.
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.
Looks good. Thanks for keeping older versions alive.
Thanks for the approval. I'm merging the PR. |
PQconnectdb()
by PQconnectdbParams()
A value in a connection string should be quoted if it can contain spaces:
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING-KEYWORD-VALUE
Alternative approach is to use
PQconnectdbParams()
instead ofPQconnectdb()
. This should be more solid approach to pass password and other connection options, which doesn't require escaping value strings.Issue: #382