Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 2:40 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > I think you're overreacting to a behavior that isn't really very surprising.
>
> > If we don't initialize SSL the first time, we don't have a working SSL
> > stack. If we didn't choose to die at that point, we'd be starting up a
> > server that could not accept any SSL connections. I don't think users
> > would like that.
>
> > If we don't *reinitialize* it, we *do* have a working SSL stack. We
> > haven't been able to load the updated configuration, but we still have
> > the old one. We could fall over and die anyway, but I don't think
> > users would like that either. People don't expect 'pg_ctl reload' to
> > kill off a working server, even if the new configuration is bad.
>
> The larger context here is that this is (or at least is supposed to be)
> exactly the same as our reaction to any other misconfiguration: die if
> it's detected at server start, but if it's detected during a later SIGHUP
> reload, soldier on with the known-good previous settings. I believe
> that's fairly well documented already.

This distinction (of server startup vs. reload) is precisely what I
think should be conveyed and addressed in the comments of functions
responsible for (re)initialization of resources. Such a comment,
specifically calling out processing/logging/error-handling differences
between startup and reload, would've definitely helped when I was
trying to understand the code, and trying to figure out the different
contexts these functions may be executed in. The fact that
ProcessStartupPacket() function calls these functions, and then also
calls itself recursively, made understanding the code and intent much
harder.

And since variable/parameter/function names also convey intent, their
naming should also be as explicit as possible, rather than being
vague.

Calling variables/parameters 'isServerStart' leaves so much to
interpretation; how many and what other cases is the code called in
when isServerStart == false? I think a better scheme would've been to
name the parameter as 'reason', and use an enum/constant to convey
that there are exactly 2 higher-level cases that the code is called in
context of: enum InitializationReason { ServerStartup, ServerReload }.

In these functions, it's not important to know the distinction of
whether the server is starting-up vs. already running (or whatever
other states the server may be in) , instead it's important to know
the distinction of whether the server is starting-up or being
reloaded; other states of the server operation, if any, do not matter
here.

Best regards,
Gurjeet
http://Gurje.et