Re: POC: Carefully exposing information without authentication

Thanks for taking a look at this!

> 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's a very valid concern, and I completely understand your instinct.
I've been taking those concerns seriously, and hardening the patch over
time. While the initial version had a blocking peek, it now sets to
non-blocking and falls back to the usual startup packet handling. Client
input always runs through bounded strncmp calls, and is never parsed or
echoed back. The items that are output are not modifiable by the client,
and consist of low-impact, finite bits of information (version,sysid,role).
I'm happy to have people keep pushing back on this patch, so we can settle
any other concerns.

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

The Win32 CI box was happily working until the test right after the first
reload via $node->reload().

https://github.com/postgres/postgres-cfbot/actions/runs/34537122766/job/103071572936

"No connection could be made because the target machine actively refused
it."

I don't have a working WIN32 to debug this, but perhaps something about the
reload causes connections to temporarily fail? Before I do a better
solution, just wanted to see if a sleep would clear things up in case
that's not the problem (if it is, will probably get a WIN32 system working,
then look into a bounded connection-retry loop in the tests).

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

An empty list is fine to pass through.
This is modeled after log_destination in backend/utils/error/elog.c

> 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?

Sure, will do so in the next patch.

> 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.

Okay, will add a comment. It should not be a problem to leave early from a
signal - it just means more loops before we hit the timeout. I considered
WaitLatch but that seemed overkill as this is already handling a rare
condition and the sleep and timeout are both relatively small.

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

It didn't seem like the right place for ereport to me - we don't have a
real SQL connection, so no need to worry about SQLSTATE and translations.
It seems to fall into the same sort of category as the "could not close
listen socket" failures in postmaster.c

> 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.

I guess we could precompile this version string response, but it seems a
micro-optimization as the fork/exit of each connection dwarfs any string
handling costs.

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

It's always available, since we just forked off of postmaster, and we don't
need to do anything special to access XLogCtl. For Win32, the
PGSharedMemoryReAttach call happens before the code in this patch gets
invoked.

Cheers,
Greg