Re: PQclientEncoding() returns -1, resulting in possible assertion failure in psql

On Fri, Mar 21, 2014 at 09:53:25PM -0400, Bruce Momjian wrote:
> On Mon, Dec 9, 2013 at 08:17:35AM +0900, Tatsuo Ishii wrote:
> > > branches. I would argue however that the documentation nowhere
> > > suggests that PQclientEncoding can return a bogus encoding ID,
> > > so this is more likely to be a bug fix than a new bug for other
> > > programs as well. Also, it looks to me like there are probably
> >
> > This sounds like a little bit unfair argument. The libpq documentation
> > is pretty sloppy for the error case for other PQ* as well. For
> > example, look at the PQdb document:
> >
> > PQdb
> > Returns the database name of the connection.
> > char *PQdb(const PGconn *conn);
> >
> > This says nothing about when the connection is bad. Reality is PQdb
> > returns NULL in the case. But are we allowed to change PQdb returns
> > say, "template1" when the connection is bad because the doc says
> > nothing about error case?
>
> So, what did we decide on this? Should we document the -1 return, or
> return SQL_ASCII.

OK, hearing nothing, I dug into this, and I think the solution is
simpler than we thought. Basically, the Assert is checking for the
encoding value to be in a valid range, but the main code is also
checking for an invalid encoding and returning PG_SQL_ASCII:

pg_encoding_mblen(int encoding, const char *mbstr)
{
Assert(PG_VALID_ENCODING(encoding));

return ((encoding >= 0 &&
encoding < sizeof(pg_wchar_table) / sizeof(pg_wchar_tbl)) ?
((*pg_wchar_table[encoding].mblen) ((const unsigned char *) mbstr)) :
((*pg_wchar_table[PG_SQL_ASCII].mblen) ((const unsigned char *) mbstr)));
}

I think the Assert can be removed as it is checking for something that
the main code handles just fine. I assume Asserts are only for checks we
don't want to make in the main code path. Throwing an error for an
assert build and handling the value just fine in a non-assert build
makes no sense.

I have updated the documentation to mention the libpq's
PQclientEncoding() possible return value of -1, and removed the Asserts
from three functions that are already handling invalid encoding values
just fine.

This does fix the reported failure with Asserts enabled:

BEFORE:

test=> \df lkjasdf
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \df lkjasdf
psql: wchar.c:1781: pg_encoding_mblen: Assertion `((encoding) >= 0 && (encoding) < _PG_LAST_ENCODING_)' failed.
/usr/lbin/execargs: line 10: 25883 Aborted "$@"

AFTER:

test=> \df lkjasdf
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \df lkjasdf
You are currently not connected to a database.
!>

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +