From: | Jacob Champion <jchampion(at)timescale(dot)com> |
---|---|
To: | Jelte Fennema <me(at)jeltef(dot)nl> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz> |
Subject: | Re: [PATCH] Align GSS and TLS error handling in PQconnectPoll() |
Date: | 2023-02-16 17:59:54 |
Message-ID: | CAAWbhmhGwsWdeZSPQSO113qHPR0UDKi4BO0jPX5ra_CPD7wqhQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Feb 16, 2023 at 3:31 AM Jelte Fennema <me(at)jeltef(dot)nl> wrote:
>
> Patch looks good to me. Definitely an improvement over the status quo.
Thanks for the review!
> Looking at the TLS error handling though I see these two lines:
>
> && conn->allow_ssl_try /* redundant? */
> && !conn->wait_ssl_try) /* redundant? */
>
> Are they actually redundant like the comment suggests? If so, we
> should probably remove them (in another patch). If not (or if we don't
> know), should we have these same checks for GSS?
It's a fair point. GSS doesn't have an "allow" encryption mode, so
they can't be the exact same checks. And we're already not checking
the probably-redundant information, so I'd vote against speculatively
adding it back. (try_gss is already based on gssencmode, which we're
using here. So I think rechecking try_gss would only help if we wanted
to clear it manually while in the middle of processing a GSS exchange.
From a quick inspection, I don't think that's happening today -- and
I'm not really sure that it could be useful in the future, because I'd
think prefer-mode is supposed to guarantee a retry on failure.)
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.
Thanks,
--Jacob
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Lakhin | 2023-02-16 18:00:00 | Re: ATTACH PARTITION seems to ignore column generation status |
Previous Message | Alvaro Herrera | 2023-02-16 17:53:05 | Re: Support logical replication of DDLs |