Обсуждение: "failed to commit client_encoding" explained

I think I see the reason for the recent report of $SUBJECT.
Starting with a client_encoding different from server_encoding,
change it to something else and then roll back, for example

u8=# show server_encoding ;server_encoding 
-----------------UTF8
(1 row)

u8=# set client_encoding to latin1;
SET
u8=# begin;
BEGIN
u8=# set client_encoding to latin2;
SET
u8=# rollback;
ROLLBACK

and sure enough LOG:  failed to commit client_encoding
pops up in the postmaster log.

The reason is that SetClientEncoding() fails, because it doesn't
want to risk doing catalog lookups, unless IsTransactionState()
is true.  And in the above situation, we try to restore
client_encoding to latin1 in TRANS_ABORT state, for which
IsTransactionState() returns FALSE.

This misbehavior is new in 8.3, because in prior releases
IsTransactionState() would return TRUE for TRANS_ABORT.

I still think that the tightening of IsTransactionState is correct:
it is not a good idea to be trying to do catalog lookups in an
already-failed transaction.  Rather, we need to fix the mbutils
machinery so that it can restore a previously-accepted encoding
combination without doing any fresh catalog lookups.  This is
really pretty analogous to the pushups that assign_role and
assign_session_authorization have done for a long time to ensure
that they can restore state without catalog lookups.

The trick those two functions use (encoding the info they need
into the string saved by GUC) doesn't look like it scales very
well to the fmgr lookup data that SetClientEncoding needs.
What I think we need to do instead is have SetClientEncoding
never throw away lookup data once it's acquired it, but maintain
its own little cache of looked-up conversion functions that it
can use without doing fresh lookups.

A problem with such caching is that it'd fail to respond to changes in
the content of pg_conversion.  Now the code is already pretty
insensitive in that respect, because if you're not doing any fresh "SET
client_encoding" commands it won't ever notice changes in that catalog
anyway.  But this'd make it worse.  We could ameliorate the issue
somewhat by doing fresh lookups (and updating the cache) whenever doing
SetClientEncoding with IsTransactionState() true, and only relying on
the cache when IsTransactionState() is false.

Comments?
        regards, tom lane

Tom Lane wrote:
> A problem with such caching is that it'd fail to respond to changes in
> the content of pg_conversion.  Now the code is already pretty
> insensitive in that respect, because if you're not doing any fresh "SET
> client_encoding" commands it won't ever notice changes in that catalog
> anyway.  But this'd make it worse.  We could ameliorate the issue
> somewhat by doing fresh lookups (and updating the cache) whenever doing
> SetClientEncoding with IsTransactionState() true, and only relying on
> the cache when IsTransactionState() is false.
> 
> Comments?

Certainly seems like a reasonable compromise. From what I understand,
you'll get this "failed to commit..." message *if* you have changedf
things in pg_conversion. I think that's acceptable - it's not like
people modify pg_conversion all the time (at least I hope they don't).

//Magnus

Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> Comments?

> Certainly seems like a reasonable compromise. From what I understand,
> you'll get this "failed to commit..." message *if* you have changedf
> things in pg_conversion. I think that's acceptable - it's not like
> people modify pg_conversion all the time (at least I hope they don't).

Yeah, that's a corner case on a corner case.

A further thought here is that even though the change I propose is
pretty localized, it's still complicated enough to possibly introduce
new bugs.  And it's fixing a case that I think doesn't occur in
practice; people don't really make local-in-transaction changes of
client_encoding.  (Remember the bug was only discovered by accident.)

So my inclination is to fix it now in HEAD so it can go through a
cycle of beta testing, but leave 8.3 alone.  We can back-patch it
after testing if we get any additional complaints.
        regards, tom lane