Skip to content

pgduck client: include conninfo in start-failure error #361

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

Open
sfc-gh-dachristensen wants to merge 3 commits into
base: main
Choose a base branch
from
Open

pgduck client: include conninfo in start-failure error #361

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2d4a28
pgduck client: include conninfo in start-failure error
sfc-gh-dachristensen May 18, 2026
085dd9b
pgduck client: log conninfo to server log instead of errdetail
sfc-gh-dachristensen May 22, 2026
bba35ff
pgduck client: resolve env-supplied conninfo for log
sfc-gh-dachristensen May 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions pg_lake_engine/src/pgduck/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

static void InitializePGDuckClient(void);
static void SetupPgDuckConnectionHash(void);
static char *ResolvePgduckConninfo(void);

static void PGDuckClientTransactionCallback(XactEvent event, void *arg);
static void PGDuckClientSubtransactionCallback(SubXactEvent event,
Expand Down Expand Up @@ -127,6 +128,77 @@ SetupPgDuckConnectionHash(void)
}


/*
* ResolvePgduckConninfo returns a palloc'd connection string showing the
* options libpq would actually use to reach pgduck_server, including values
* supplied via environment variables (e.g. PGHOSTADDR, PGPORT) and libpq's
* compiled-in defaults. This is what an administrator needs to debug a
* misconfigured server -- PgduckServerConninfo alone hides any env-supplied
* overrides.
*
* We resolve options without opening a socket: PQconndefaults() returns the
* options with environment variables and compiled-in defaults applied, and
* PQconninfoParse() returns the values from the configured conninfo string.
* Configured values override defaults. Options whose dispchar starts with
* '*' (passwords) or 'D' (debug-only) are omitted, as are empty values.
*/
static char *
ResolvePgduckConninfo(void)
{
StringInfoData buf;

initStringInfo(&buf);

PQconninfoOption *defaults = PQconndefaults();
PQconninfoOption *configured = PQconninfoParse(PgduckServerConninfo, NULL);

Copy link
Copy Markdown
Collaborator

@sfc-gh-mslot sfc-gh-mslot May 28, 2026

Choose a reason for hiding this comment

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

can we do this when connections are failing?

Copy link
Copy Markdown
Collaborator Author

@sfc-gh-dachristensen sfc-gh-dachristensen May 28, 2026

Choose a reason for hiding this comment

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

Yeah, probably PQconninfoParse() is a better choice here.

Copy link
Copy Markdown
Collaborator Author

@sfc-gh-dachristensen sfc-gh-dachristensen May 28, 2026

Choose a reason for hiding this comment

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

Though that doesn't incorporate defaults; will poke around a little more.

Copy link
Copy Markdown
Collaborator Author

@sfc-gh-dachristensen sfc-gh-dachristensen May 28, 2026

Choose a reason for hiding this comment

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

Looks like a combination of PQconndefaults() and that should work.

if (defaults == NULL)
{
/* OOM inside libpq; fall back to the raw configured string */
Copy link
Copy Markdown
Collaborator Author

@sfc-gh-dachristensen sfc-gh-dachristensen May 28, 2026

Choose a reason for hiding this comment

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

Probably fairly useless to do this if we're in OOM situation, but YOLO.

if (configured != NULL)
PQconninfoFree(configured);
appendStringInfoString(&buf, PgduckServerConninfo);
return buf.data;
}

bool first = true;

for (PQconninfoOption *opt = defaults; opt->keyword != NULL; opt++)
{
const char *val = opt->val;

if (configured != NULL)
{
for (PQconninfoOption *c = configured; c->keyword != NULL; c++)
{
if (strcmp(c->keyword, opt->keyword) == 0)
{
if (c->val != NULL)
val = c->val;
break;
}
}
}

if (val == NULL || val[0] == '\0')
continue;
if (opt->dispchar != NULL &&
(opt->dispchar[0] == '*' || opt->dispchar[0] == 'D'))
continue;

appendStringInfo(&buf, "%s%s='%s'",
first ? "" : " ", opt->keyword, val);
first = false;
}

PQconninfoFree(defaults);
if (configured != NULL)
PQconninfoFree(configured);

return buf.data;
}


/*
* GetPGDuckConnection returns the main PGDuck connection.
*/
Expand All @@ -143,6 +215,20 @@ GetPGDuckConnection(void)

PQfinish(connection);

/*
* Log the conninfo to the server log so an administrator can debug
* misconfigured servers (issue #293) without exposing the connection
Comment thread
sfc-gh-dachristensen marked this conversation as resolved.
* string -- which may include credentials -- to the client. We
* intentionally do not include the libpq error message here; it is
* surfaced only in the assertion-enabled ERROR below.
*/
char *resolved = ResolvePgduckConninfo();

ereport(LOG_SERVER_ONLY,
(errmsg("could not start query engine"),
errdetail("connection string: %s", resolved)));
pfree(resolved);

#ifdef USE_ASSERT_CHECKING
ereport(ERROR, (errmsg("could not start query engine: %s", errorMessage)));
#else
Expand Down
Loading