Re: PQclientEncoding() returns -1, resulting in possible assertion failure in psql - Mailing list pgsql-bugs

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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +