pgduck client: include conninfo in start-failure error#361
Open
sfc-gh-dachristensen wants to merge 3 commits into
Open
pgduck client: include conninfo in start-failure error#361sfc-gh-dachristensen wants to merge 3 commits into
sfc-gh-dachristensen wants to merge 3 commits into
Conversation
Add an errdetail() clause to the "could not start query engine" ereport() so users can see which connection string was being used when the connection to pgduck_server failed. Issue: #293 Signed-off-by: David Christensen <david.christensen@snowflake.com>
sfc-gh-mslot
requested changes
May 18, 2026
sfc-gh-dachristensen
added a commit
that referenced
this pull request
May 22, 2026
Per review on PR #361: the connection string may include credentials and is an internal detail not appropriate for the client. Emit a separate LOG ereport() with errmsg + errdetail before the user-facing ERROR so administrators can still debug misconfigured servers (issue #293) via the PostgreSQL server log without leaking the conninfo to the client. Signed-off-by: David Christensen <david.christensen@snowflake.com>
sfc-gh-dachristensen
added a commit
that referenced
this pull request
May 22, 2026
Per review on PR #361: the connection string may include credentials and is an internal detail not appropriate for the client. Emit a separate LOG ereport() with errmsg + errdetail before the user-facing ERROR so administrators can still debug misconfigured servers (issue #293) via the PostgreSQL server log without leaking the conninfo to the client. Signed-off-by: David Christensen <david.christensen@snowflake.com>
sfc-gh-dachristensen
force-pushed
the
pgguru/issue-293-conninfo-errdetail
branch
from
181666a to
c131d1e
Compare
Per review on PR #361: the connection string may include credentials and is an internal detail not appropriate for the client. Emit a separate LOG ereport() with errmsg + errdetail before the user-facing ERROR so administrators can still debug misconfigured servers (issue #293) via the PostgreSQL server log without leaking the conninfo to the client. Signed-off-by: David Christensen <david.christensen@snowflake.com>
sfc-gh-dachristensen
force-pushed
the
pgguru/issue-293-conninfo-errdetail
branch
from
c131d1e to
085dd9b
Compare
| initStringInfo(&buf); | ||
|
|
||
| PGconn *probe = PQconnectStart(PgduckServerConninfo); | ||
|
|
Collaborator
There was a problem hiding this comment.
can we do this when connections are failing?
Collaborator
Author
There was a problem hiding this comment.
Yeah, probably PQconninfoParse() is a better choice here.
Collaborator
Author
There was a problem hiding this comment.
Though that doesn't incorporate defaults; will poke around a little more.
Collaborator
Author
There was a problem hiding this comment.
Looks like a combination of PQconndefaults() and that should work.
Add ResolvePgduckConninfo(), which uses PQconnectStart + PQconninfo to let libpq overlay environment variables (PGHOSTADDR, PGPORT, etc.) and compiled-in defaults onto pg_lake_engine.host, then formats the result as a conninfo string with passwords and debug-only fields stripped. The LOG_SERVER_ONLY errdetail emitted on connection failure now uses this resolved string, so an administrator debugging issue #293-style env-var overrides sees the values libpq actually used rather than just the configured GUC. Signed-off-by: David Christensen <david.christensen@snowflake.com>
sfc-gh-dachristensen
force-pushed
the
pgguru/issue-293-conninfo-errdetail
branch
from
0df0208 to
bba35ff
Compare
|
|
||
| if (defaults == NULL) | ||
| { | ||
| /* OOM inside libpq; fall back to the raw configured string */ |
Collaborator
Author
There was a problem hiding this comment.
Probably fairly useless to do this if we're in OOM situation, but YOLO.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the connection to pgduck_server fails, the user-facing ERROR only says "could not start query engine", which leaves administrators with nothing to
debug a misconfigured server (e.g. environment variables overriding the configured conninfo).
This PR adds a LOG_SERVER_ONLY ereport before the user-facing ERROR with the connection string in errdetail. The conninfo lands in the PostgreSQL server
log where the administrator can see it, but is never sent to the client (which may be a less-trusted role and the conninfo can include credentials).
The user-facing ERROR is unchanged — the libpq error message is still only included in assertion-enabled builds, and the conninfo is never included.
It may still be worth a follow-up to look at the underlying behavior of environment variables superseding the configured connection string; that seems
undesirable.