On Thu, Feb 16, 2023 at 09:59:54AM -0800, Jacob Champion wrote:
> On Thu, Feb 16, 2023 at 3:31 AM Jelte Fennema <me@jeltef.nl> wrote:
>> Patch looks good to me. Definitely an improvement over the status quo.
>
> Thanks for the review!
I was looking at that a second time, and with fresh eyes I can see
that we would miss to mark conn->status with CONNECTION_BAD when using
gssencmode=require when the polling fails in pqsecure_open_gss(),
which is just wrong IMO. This code has been introduced by b0b39f7,
that has added support for GSS encryption. I am adding Stephen Frost
in CC to see if he has any comments about all this part of the logic
with gssencmode.
> I suspect this is a much deeper rabbit hole; I think it's work that
> needs to be done, but I can't sign myself up for it at the moment. The
> complexity of this function is off the charts (for instance, why do we
> recheck conn->try_gss above, if the only apparent way to get to
> CONNECTION_GSS_STARTUP is by having try_gss = true to begin with? is
> there a goto/retry path I'm missing?). I think it either needs heavy
> assistance from a committer who already has intimate knowledge of this
> state machine and all of its possible branches, or from a static
> analysis tool that can help with a step-by-step simplification.
The first one of these is from 57c0879, the second from bcd713a, which
I assume is a copy-paste of the first one. I agree that
PQconnectPoll() has grown beyond the point of making it easy to
maintain. I am wondering which approach we could take when it comes
to simplify something like that. Attempting to reduce the number of
flags stored in PGconn would be one. The second may be to split the
internal logic into more functions, for each state we are going
through? The first may lead to an even cleaner logic for the second
point.
--
Michael