Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION

I wrote:
>> So I pushed the core bug fix, and I'll take another look at
>> that part later.

> ... or not; the buildfarm didn't like that much. It'll
> have to wait till after these releases, because I'm
> overdue to get to work on the release notes.

The reason the buildfarm found something I'd missed in testing
is that I didn't run check-world with debug_parallel_query set,
which was a bad idea for a patch messing with parallel query
mechanics :-(. Mea culpa.

However, what the farm found is that assign_client_encoding
is flat-out broken. It's ignoring the first commandment
for GUC hooks, which is "Thy assign hooks shalt not fail".
(If we didn't need that, there wouldn't be a separation
between check hooks and assign hooks in the first place.)

Because it's throwing an error at the wrong time, it spits
up if it sees an (irrelevant) rollback of client_encoding
during cleanup of a failed parallel worker. We didn't see
this before because GUCRestoreState was being run in a separate
mini-transaction that (usually at least) doesn't fail.

The attached correction basically just moves that test into
check_client_encoding where it should have been to begin with.
After applying this, I can un-revert f5f30c22e and everything
passes.

However, this episode definitely gives me pause about back-patching
f5f30c22e as I did before. It seems not impossible that there
are extensions with similarly mis-coded assign hooks, and if
so those are going to need to be fixed. (I did check that none
of the other core GUCs have this problem. assign_recovery_target
and friends would, except that they are for PGC_POSTMASTER variables
that won't be getting changed by GUCRestoreState. Anyway they're a
known kluge, and this patch isn't making it worse.) So we'd better
treat this as a minor API change, which means we probably shouldn't
put it in stable branches.

What I'm currently thinking is to apply in HEAD and perhaps v17,
but not further back. Given that this bug has existed since
the beginning of parallel query yet wasn't reported till now,
it's not sufficiently problematic to take any risk for in
stable branches.

Any opinions about whether it's too late to do this in v17?
Post-beta3 is pretty late, for sure, but maybe we could get
away with it. And we are fixing a bug here.

regards, tom lane