Re: Log connection establishment timings

On Mon, Mar 3, 2025 at 11:14 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
>
> On Fri, Feb 28, 2025 at 05:52:35PM -0500, Melanie Plageman wrote:
>
> +bool
> +check_log_connections(char **newval, void **extra, GucSource source)
> +{
>
> This function is pretty close to check_debug_io_direct() for example and its
> overall content, memory management, looks good to me. I just have a few
> following comments about it.

So, I started thinking more about this patch. I wonder if what we
really want to do is add a new GUC type that is somewhere between a
list and an enum. What I'm imagining is basically a set -- you could
specify parameters in a comma separated list like before but without
quotes:

SET log_connections = received,authorized;

It would be for GUCs that can be any combination of a set of
predetermined values.
There are actually a few list gucs that would benefit from this
structure: see log_destination, debug_io_direct,
restrict_nonsystem_relation_kind. And I imagine there are others.

It would reduce code duplication for those (see the boiler plate
string parsing and handling for all of these check functions). If this
was implemented, log_connections wouldn't even need its own check
function (I think).

It also might be easier to do tab completion (depending on how it is
implemented).

And I think it would be even simpler to do your idea of groups and to
have aliases for different combinations of options. And it would let
us keep 'on' and 'off' as backwards compatibility aliases.

Maybe if I tried implementing it, it wouldn't work at all. But, as it
stands, I'm feeling nervous about making two of the most common GUCs
in the world (log_connections and log_disconnections) less structured.

And maybe, instead of 'all', we want '*'? I think using the idea of a
set may open us up to better interface ideas.

All that being said, this is hardly a project for March 3rd. So, in
service of moving 0002 forward, I think I want to walk back to an idea
everyone else on this thread was keen on: making log_connections an
enum with 'on', 'off', and 'timings'. This moves us in the direction
of having more granular control in log_connections without breaking
any existing users and paving the way for a project like what I
outlined above. What do you think?

> I was just wondering why:
>
> "
> + else if (pg_strcasecmp(item, "all") == 0)
> + {
> + GUC_check_errdetail("Must specify \"all\" alone with no additional options, whitespace, or characters.");
> + goto log_connections_error;
> + }
> "
>
> but yeah that probably makes more sense that way.

Well, you can't have other punctuation or trailing commas with other
list GUCs. The whitespace I could add code to support. But I think
this goes back to my feelings about why this whole endeavor might be
sketchy.

- Melanie