Re: proposal: psql: show current user in prompt - Mailing list pgsql-hackers

On Mon, 28 Aug 2023 at 15:00, Pavel Stehule <pavel.stehule@gmail.com> wrote:
+                       minServerMajor = 1600;
+                       serverMajor = PQserverVersion(pset.db) / 100;

Instead of using the server version, we should instead use the
protocol version negotiation that's provided by the
NegotiateProtocolVersion message type. We should bump the requested
protocol version from 3.0 to 3.1 and check that the server supports
3.1. Otherwise proxies or connection poolers might get this new
message type, without knowing what to do with them.

+   <varlistentry id="protocol-message-formats-ReportGUC">
+    <term>ReportGUC (F)</term>

We'll need some docs on the "Message Flow" page too:
https://www.postgresql.org/docs/current/protocol-flow.html
Specifically what response is expected, if any.

Another thing that should be described there is that this falls
outside of the transaction flow, i.e. it's changes are not reverted on
ROLLBACK. But that leaves an important consideration: What happens
when an error occurs on the server during handling of this message
(e.g. the GUC does not exist or an OOM is triggered). Is any open
transaction aborted in that case? If not, we should have a test for
that.

+   if (PQresultStatus(res) != PGRES_COMMAND_OK)
+       pg_fatal("failed to link custom variable: %s", PQerrorMessage(conn));
+   PQclear(res);

The tests should also change the config after running
PQlinkParameterStatus/PQunlinkParameterStatus to show that the guc is
reported then or not reported then.

+   if (!PQsendTypedCommand(conn, PqMsg_ReportGUC, 't', paramName))
+       return NULL;


I think we'll need some bikeshedding on what the protocol message
should look like exactly. I'm not entirely sure what is the most
sensible here, so please treat everything I write next as
suggestions/discussion:
I see that you're piggy-backing on PQsendTypedCommand, which seems
nice to avoid code duplication. It has one downside though: not every
type, is valid for each command anymore.
One way to avoid that would be to not introduce a new command, but
only add a new type that is understood by Describe and Close, e.g. a
'G' (for GUC). Then PqMsg_Describe, G would become the equivalent of
what'the current patch its PqMsg_ReportGUC, 't' and PqMsg_Close, G
would be the same as PqMsg_ReportGUC, 'f'.

The rest of this email assumes that we're keeping your current
proposal for the protocol message, so it might not make sense to
address all of this feedback, in case we're still going to change the
protocol:

+                   if (is_set == 't')
+                   {
+                       SetGUCOptionFlag(name, GUC_REPORT);
+                       status = "SET REPORT_GUC";
+                   }
+                   else
+                   {
+                       UnsetGUCOptionFlag(name, GUC_REPORT);
+                       status = "UNSET REPORT_GUC";
+                   }

I think we should be strict about what we accept here. Any other value
than 'f'/'t' for is_set should result in an error imho.