On 16/07/2024 09:54, Michael Paquier wrote:
> On Mon, Jun 24, 2024 at 11:30:53PM +0300, Heikki Linnakangas wrote:
>> 0002: This is the main patch that fixes the SSL fallback issue
>
> + conn->failed_enc_methods |= conn->allowed_enc_methods &
> (~conn->current_enc_method);
>
> Sounds reasonable to me.
>
> It's a bit annoying to have to guess that current_enc_method is
> tracking only one method at a time (aka these three fields are not
> documented in libpq-int.h), while allowed_enc_methods and
> failed_enc_methods is a bitwise combination of the methods that are
> still allowed or that have already failed.
Yeah. In hindsight I'm still not very happy with the code structure with
"allowed_enc_methods" and "current_enc_methods" and all that. The
fallback logic is still complicated. It's better than in v16, IMHO, but
still not great. This patch seems like the best fix for v17, but I
wouldn't mind another round of refactoring for v18, if anyone's got some
good ideas on how to structure it better. All these new tests are a
great asset when refactoring this again.
> + if (IsInjectionPointAttached("backend-initialize-v2-error"))
> + {
> + FrontendProtocol = PG_PROTOCOL(2,0);
> + elog(FATAL, "protocol version 2 error triggered");
> + }
>
> This is an attempt to do stack manipulation with an injection point
> set. FrontendProtocol is a global variable, so you could have a new
> callback setting up this global variable directly, then FATAL (I
> really don't mind is modules/injection_points finishes with a library
> of callbacks).
>
> Not sure to like much this new IsInjectionPointAttached() that does a
> search in the existing injection point pool, though. This leads to
> more code footprint in the core backend, and I'm trying to minimize
> that. Not everybody agrees with this view, I'd guess, which is also
> fine.
Yeah, I'm also not too excited about the additional code in the backend,
but I'm also not excited about writing another test C module just for
this. I'm inclined to commit this as it is, but we can certainly revisit
this later, since it's just test code.
Here's a new rebased version with some minor cleanup. Notably, I added
docs for the new IS_INJECTION_POINT_ATTACHED() macro.
--
Heikki Linnakangas
Neon (https://neon.tech)