Re: POC: Carefully exposing information without authentication

> On 15 Sep 2026, at 19:57, Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:

> ..and a small sleep in the tests to try an overcome an issue with the WIN32 CI tests. Here's the new message:

What sort of issue? Sleeping in tests is generally never the right option but
can of course be useful in debugging.

> Allow specific information to be output directly by Postgres

To be honest, I am absolutely terrified by any feature which does any level of
work based on user input before authentication or authorization. That being
said I've had this on my TODO to review, and while that is still left on the
TODO I threw some cursory looks while waiting for a test-run. Below are a few
comments from skimming.

+ foreach(l, elemlist)
+ {
+ char *tok = (char *) lfirst(l);
+
+ if (pg_strcasecmp(tok, "role") == 0)
+ newexpose |= EXPOSE_INFO_ROLE;
+ else if (pg_strcasecmp(tok, "sysid") == 0)
+ newexpose |= EXPOSE_INFO_SYSID;
+ else if (pg_strcasecmp(tok, "version") == 0)
+ newexpose |= EXPOSE_INFO_VERSION;
+ else
+ {
+ GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+ }
+
+ pfree(rawstring);
+ list_free(elemlist);

This will catch syntax errors and unrecognised keys, but not an empty list.
Should that be handled?

+ if (
+ (expose_information & endpoint_actions[i].require)
+ &&
+ strncmp(buf, endpoint_actions[i].endpoint, endpoint_len) == 0
+ &&
+ (buf[endpoint_len] == ' ' || buf[endpoint_len] == '\r' || buf[endpoint_len] == '\0')
+ )
+ {

While not overly complicated, it's also not particularly readable. Can it be
broken up into a series of conditionals to make it easier to follow?

+ pg_usleep(EXPOSE_SEND_RETRY_SLEEP_US);

pg_usleep is woken up and return before the expected sleep interval, and while
that might be a problem here it should at least be documented in a comment why
that's not a problem.

+ /* The send() call failed in some way we cannot handle */
+ elog(LOG, "failed to send information to client: %m");

Why elog instead of ereport, a syscall failing doesn't seem like an internal
error to me?

+ case EXPOSE_TYPE_GET_VERSION:
+ appendStringInfo(&content, "%d\r\n",
+ PG_VERSION_NUM);
+ break;

Constructing static data for the response dynamically every time seems to
introduce quite asymmetrical cost relationships. The cost for the caller to
perform the request is very low compared to the work done serverside.

+ case EXPOSE_TYPE_GET_PRIMARY:
+ appendStringInfo(&content, "%d\r\n",
+ RecoveryInProgress() ? 0 : 1);

Maybe I'm daft and miss something obvious, but this is executed before shared
memory is available but RecoveryInProgress requires XLOGShmemInit to work?

--
Daniel Gustafsson