Thread: Bug in libpq implentation and omission in documentation?

Hello PG hackers. Yesterday I began diagnosing a peculiar bug in some
production code that has been happily running for months. I finally got
to the bottom of it despite the rather misleading error message. Anyway,
within a section of code we are making a DELETE call to the database via
the libpq call PQexecParams(). It failed with this message:

'ERROR:  bind message has 32015 parameter formats but 1 parameters'

This was just plain wrong. In fact, the # of parameters was more like
80,000. The area of code is quite clear. Despite this being a
particularly large number of parameters (as you can imagine this query
is built dynamically based on arbitrarily sized input) the data type for
nParams for is a plain old 4-byte int. Upon further and deeper
inspection I find that this 4 byte int is truncated to two bytes just
before going down the wire.

There is no mention of any restriction in the 9.1.4 documentation:

http://www.postgresql.org/docs/9.1/static/libpq-exec.html

And the interface quite clearly accepts a 4 byte int however, the PQsendQueryGuts()
function on line 1240 of src/interfaces/libpq/fq-exec.c just blatantly truncates the
integer - it's calls pqPutInt() for nParams with a literal 2 rather than 4. It does this
several times, in fact.

Unless I'm barking mad, surely this should either

a) Be fixed and send 4 with nParams for pqPutInt() rather than 2
b) Documented (and the type changed) as only being a 2 byte int  and therefore having a restriction on the number of
parameters permitted in PQexecParams().
 

Could someone either verify or correct me before I submit an official bug report!?

Regards,

Jim Vanns

-- 
Jim Vanns
Systems Programmer
Framestore

Ah ha. Yes, you're correct. It does mention here that an Int16 is used
to specify the number of parameter format codes, values etc. 

I suggest then that the documentation is updated to reflect this? Anf
again, perhaps the 'int' for nParams should be an int16_t or short?

Naturally I have already modified our code to batch or chunk rather
large numbers of parameters to fit within this restriction but I do
think it'd help others if the API interface reflected the same size data
types as the restricted back ends ;)

Thanks Dmitriy,

Jim

On Wed, 2012-08-08 at 13:27 +0400, Dmitriy Igrishin wrote:
> Hey Jim,
> 
> 2012/8/8 Jim Vanns <james.vanns@framestore.com>
>         Hello PG hackers. Yesterday I began diagnosing a peculiar bug
>         in some
>         production code that has been happily running for months. I
>         finally got
>         to the bottom of it despite the rather misleading error
>         message. Anyway,
>         within a section of code we are making a DELETE call to the
>         database via
>         the libpq call PQexecParams(). It failed with this message:
>         
>         'ERROR:  bind message has 32015 parameter formats but 1
>         parameters'
>         
>         This was just plain wrong. In fact, the # of parameters was
>         more like
>         80,000. The area of code is quite clear. Despite this being a
>         particularly large number of parameters (as you can imagine
>         this query
>         is built dynamically based on arbitrarily sized input) the
>         data type for
>         nParams for is a plain old 4-byte int. Upon further and deeper
>         inspection I find that this 4 byte int is truncated to two
>         bytes just
>         before going down the wire.
>         
>         There is no mention of any restriction in the 9.1.4
>         documentation:
>         
>         http://www.postgresql.org/docs/9.1/static/libpq-exec.html
>         
>         And the interface quite clearly accepts a 4 byte int however,
>         the PQsendQueryGuts()
>         function on line 1240 of src/interfaces/libpq/fq-exec.c just
>         blatantly truncates the
>         integer - it's calls pqPutInt() for nParams with a literal 2
>         rather than 4. It does this
>         several times, in fact.
>         
>         Unless I'm barking mad, surely this should either
>         
>         a) Be fixed and send 4 with nParams for pqPutInt() rather than
>         2
>         b) Documented (and the type changed) as only being a 2 byte
>         int
>            and therefore having a restriction on the number of
>         parameters
>            permitted in PQexecParams().
>         
>         Could someone either verify or correct me before I submit an
>         official bug report!?
>         
>         Regards,
>         
>         Jim Vanns
> AFAIK, this is a limitation of the frontend/backend protocol which
> allows
> you to bind no more than 2^16 parameters.
> See the description of the Bind (F) message here
> http://www.postgresql.org/docs/9.2/static/protocol-message-formats.html
> 
> 
> -- 
> // Dmitriy.
> 
> 

-- 
Jim Vanns
Systems Programmer
Framestore

On 08.08.2012 12:36, Jim Vanns wrote:
> Ah ha. Yes, you're correct. It does mention here that an Int16 is used
> to specify the number of parameter format codes, values etc.
>
> I suggest then that the documentation is updated to reflect this? Anf
> again, perhaps the 'int' for nParams should be an int16_t or short?

I don't think we should change the function signature for this, but I 
think a sanity check for "nParams < 32768" in libpq would be in order.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

On Wed, 2012-08-08 at 14:24 +0300, Heikki Linnakangas wrote:
> On 08.08.2012 12:36, Jim Vanns wrote:
> > Ah ha. Yes, you're correct. It does mention here that an Int16 is used
> > to specify the number of parameter format codes, values etc.
> >
> > I suggest then that the documentation is updated to reflect this? Anf
> > again, perhaps the 'int' for nParams should be an int16_t or short?
> 
> I don't think we should change the function signature for this, but I 
> think a sanity check for "nParams < 32768" in libpq would be in order.

While I agree that perhaps changing the function signature is a little
too intrusive considering it's been that way for a long long time (I
would wager) , I do think that yes, there should be a sanity check but
more importantly the documentation should explicitly state the
limitation or restriction despite the parameter type is a 4 byte
integer. Otherwise people like myself will assume that all 4 bytes can
be used ;)

Regards,

Jim

> -- 
>    Heikki Linnakangas
>    EnterpriseDB   http://www.enterprisedb.com
> 

-- 
Jim Vanns
Systems Programmer
Framestore

On Wed, Aug 8, 2012 at 1:24 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> On 08.08.2012 12:36, Jim Vanns wrote:
>>
>> Ah ha. Yes, you're correct. It does mention here that an Int16 is used
>> to specify the number of parameter format codes, values etc.
>>
>> I suggest then that the documentation is updated to reflect this? Anf
>> again, perhaps the 'int' for nParams should be an int16_t or short?
>
>
> I don't think we should change the function signature for this, but I think
> a sanity check for "nParams < 32768" in libpq would be in order.

+1 - and also a clear update to the documentation.

-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> On 08.08.2012 12:36, Jim Vanns wrote:
>> I suggest then that the documentation is updated to reflect this? Anf
>> again, perhaps the 'int' for nParams should be an int16_t or short?

> I don't think we should change the function signature for this, but I 
> think a sanity check for "nParams < 32768" in libpq would be in order.

We *can't* change the function signature like that.  In the first place,
it would be an ABI break necessitating a bump in the .so major version
number, which would cause pain vastly out of proportion to the size of
this problem.  In the second place, if we did that, then if someone made
the same mistake and tried to pass umpteen thousand parameters to a
statement, the same truncation Jim is complaining of would still happen.
Only this way, it would happen silently inside the C function call
mechanism, such that neither the application nor libpq could detect the
problem.

A runtime check for too many parameters seems appropriate.  Assuming
that the error message it throws is well written, I don't think we need
to adjust the documentation.  There are many limitations that are not
spelled out in the docs, and adding every one of them would not amount
to a net improvement.  Considering that Jim is the first to try this in
however many years it's been since 7.4, I don't think that everybody
needs to read about this restriction while they're trying to absorb what
PQexecParams does.
        regards, tom lane

On 08.08.2012 17:35, Tom Lane wrote:
> A runtime check for too many parameters seems appropriate.  Assuming
> that the error message it throws is well written, I don't think we need
> to adjust the documentation.  There are many limitations that are not
> spelled out in the docs, and adding every one of them would not amount
> to a net improvement.  Considering that Jim is the first to try this in
> however many years it's been since 7.4, I don't think that everybody
> needs to read about this restriction while they're trying to absorb what
> PQexecParams does.

Runtime check added to the functions that take an nParams argument.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com