Horton

Thread

  1. Error in PQsetvalue

    Pavel Golub <pavel@microolap.com> — 2011-06-03T19:03:26Z

    Hello.

Reproduced under Windows XP SP3 using Visual C++ 2008 and Delphi. If
PQsetvalue is called with second parameter equals to PQntuples then
memory corruption appears. But it should grow internal tuples array
and populate the last item with provided data. Please see the code:


#include <stdio.h>
#include <stdlib.h>
#include "libpq-fe.h"
#pragma comment(lib,"libpq.lib")

static void
exit_nicely(PGconn *conn)
{
    PQfinish(conn);
    exit(1);
}

int
main(int argc, char **argv)
{
    const char *conninfo;
    PGconn     *conn;
    PGresult   *res;
    if (argc > 1)
        conninfo = argv[1];
    else
        conninfo = "dbname = postgres user = postgres password = password";

    conn = PQconnectdb(conninfo);

    if (PQstatus(conn) != CONNECTION_OK)
    {
	fprintf(stderr, "Connection to database failed: %s", PQerrorMessage(conn));
        exit_nicely(conn);
    }

   res = PQexec(conn, "SELECT generate_series(1, 10)");

   if (!PQsetvalue(res, PQntuples(res), 0, "1", 1))   /* <----- here
we have memory corruption */
	{
		fprintf(stderr, "Shit happens: %s", PQerrorMessage(conn));
		exit_nicely(conn);
	}

    PQclear(res);
    PQfinish(conn);
    return 0;
}

BUT! If we change direct call to:

...
        res = PQexec(conn, "SELECT generate_series(1, 10)");

	res2 = PQcopyResult(res, PG_COPYRES_TUPLES);

	if (!PQsetvalue(res2, PQntuples(res), 0, "1", 1))
	{
		fprintf(stderr, "Shit happens: %s", PQerrorMessage(conn));
		exit_nicely(conn);
	}
...
then all is OK!

As you can see, I copied result first. No error occurs.
Can anybody check this on other platforms?
-- 
Nullus est in vitae sensus ipsa vera est sensus.

  2. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-03T20:06:47Z

    On 6/3/2011 3:03 PM, Pavel Golub wrote:
> Hello.
>
> Reproduced under Windows XP SP3 using Visual C++ 2008 and Delphi. If
> PQsetvalue is called with second parameter equals to PQntuples then
> memory corruption appears. But it should grow internal tuples array
> and populate the last item with provided data. Please see the code:
>
>

At first glance (have not tested this theory), looks like pqAddTuple() 
doesn't zero the newly allocated tuples slots like PQsetvalue() does. 
PQsetvalue is depending on the unassigned tuple table slots to be NULL 
to detect when a tuple must be allocated.  Around line 446 on fe-exec.c. 
  I never tested this case since libpqtypes never tried to call 
PQsetvalue on a PGresult created by the standard libpq library.

The solution I see would be to zero the new table slots within 
pqAddTuple.  Any other ideas?

-- 
Andrew Chernow
eSilo, LLC
global backup
http://www.esilo.com/

  3. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-03T20:08:55Z

    On Fri, Jun 3, 2011 at 2:03 PM, Pavel Golub <pavel@microolap.com> wrote:
> Hello.
>
> Reproduced under Windows XP SP3 using Visual C++ 2008 and Delphi. If
> PQsetvalue is called with second parameter equals to PQntuples then
> memory corruption appears. But it should grow internal tuples array
> and populate the last item with provided data. Please see the code:
>
>
> #include <stdio.h>
> #include <stdlib.h>
> #include "libpq-fe.h"
> #pragma comment(lib,"libpq.lib")
>
> static void
> exit_nicely(PGconn *conn)
> {
>    PQfinish(conn);
>    exit(1);
> }
>
> int
> main(int argc, char **argv)
> {
>    const char *conninfo;
>    PGconn     *conn;
>    PGresult   *res;
>    if (argc > 1)
>        conninfo = argv[1];
>    else
>        conninfo = "dbname = postgres user = postgres password = password";
>
>    conn = PQconnectdb(conninfo);
>
>    if (PQstatus(conn) != CONNECTION_OK)
>    {
>        fprintf(stderr, "Connection to database failed: %s", PQerrorMessage(conn));
>        exit_nicely(conn);
>    }
>
>   res = PQexec(conn, "SELECT generate_series(1, 10)");
>
>   if (!PQsetvalue(res, PQntuples(res), 0, "1", 1))   /* <----- here
> we have memory corruption */

hm, what are the exact symptoms you see that is causing you to think
you are having memory corruption? (your example is working for me).

merlin

  4. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-03T20:37:49Z

    On Fri, Jun 3, 2011 at 3:06 PM, Andrew Chernow <ac@esilo.com> wrote:
> On 6/3/2011 3:03 PM, Pavel Golub wrote:
>>
>> Hello.
>>
>> Reproduced under Windows XP SP3 using Visual C++ 2008 and Delphi. If
>> PQsetvalue is called with second parameter equals to PQntuples then
>> memory corruption appears. But it should grow internal tuples array
>> and populate the last item with provided data. Please see the code:
>>
>>
>
> At first glance (have not tested this theory), looks like pqAddTuple()
> doesn't zero the newly allocated tuples slots like PQsetvalue() does.
> PQsetvalue is depending on the unassigned tuple table slots to be NULL to
> detect when a tuple must be allocated.  Around line 446 on fe-exec.c.  I
> never tested this case since libpqtypes never tried to call PQsetvalue on a
> PGresult created by the standard libpq library.
>
> The solution I see would be to zero the new table slots within pqAddTuple.
>  Any other ideas?

It might not be necessary to do that.  AIUI the tuple table slot guard
is there essentially to let setval know if it needs to allocate tuple
attributes, which always has to be done after a new tuple is created
after a set.  It should be enough to keep track of the 'allocation
tuple' as an int (which is incremented after attributes are allocated
for the new tuple).  so if tup# is same is allocation tuple, allocate
the atts and increment the number, otherwise just do a straight 'set'.
 Basically we are taking advantage of the fact only one tuple can be
allocated at a time, and it always has to be the next one after the
current set.

merlin

  5. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-03T20:38:27Z

    On 6/3/2011 4:06 PM, Andrew Chernow wrote:
> On 6/3/2011 3:03 PM, Pavel Golub wrote:
>> Hello.
>>
>> Reproduced under Windows XP SP3 using Visual C++ 2008 and Delphi. If
>> PQsetvalue is called with second parameter equals to PQntuples then
>> memory corruption appears. But it should grow internal tuples array
>> and populate the last item with provided data. Please see the code:
>>
>>
>
> At first glance (have not tested this theory), looks like pqAddTuple()
> doesn't zero the newly allocated tuples slots like PQsetvalue() does.
> PQsetvalue is depending on the unassigned tuple table slots to be NULL
> to detect when a tuple must be allocated. Around line 446 on fe-exec.c.
> I never tested this case since libpqtypes never tried to call PQsetvalue
> on a PGresult created by the standard libpq library.
>
> The solution I see would be to zero the new table slots within
> pqAddTuple. Any other ideas?
>

Eeekks.  Found an additional bug.  PQsetvalue only allocates the actual 
tuple if the provided tup_num equals the number of tuples (append) and 
that slot is NULL.  This is wrong.  The original idea behind PQsetvalue 
was you can add tuples in any order and overwrite existing ones.

Also, doing res->ntups++ whenever a tuple is allocated is incorrect as 
well; since we may first add tuple 3.  In that case, if ntups is 
currently <= 3 we need to set it to 3+1, otherwise do nothing.  In other 
words, while adding tuples via PQsetvalue the ntups should always be 
equal to (max_supplied_tup_num + 1) with all unset slots being NULL.

PQsetvalue(res, 3, 0, "x", 1); // currently sets res->ntups to 1
PQsetvalue(res, 2, 0, "x", 1); // as code is now, this returns FALSE due 
to bounds checking

-- 
Andrew Chernow
eSilo, LLC
global backup
http://www.esilo.com/

  6. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-03T20:48:22Z

    >>
>> At first glance (have not tested this theory), looks like pqAddTuple()
>> doesn't zero the newly allocated tuples slots like PQsetvalue() does.
>> PQsetvalue is depending on the unassigned tuple table slots to be NULL to
>> detect when a tuple must be allocated.  Around line 446 on fe-exec.c.  I
>> never tested this case since libpqtypes never tried to call PQsetvalue on a
>> PGresult created by the standard libpq library.
>>
>> The solution I see would be to zero the new table slots within pqAddTuple.
>>   Any other ideas?
>
> It might not be necessary to do that.  AIUI the tuple table slot guard
> is there essentially to let setval know if it needs to allocate tuple
> attributes, which always has to be done after a new tuple is created
> after a set.

Trying to append a tuple to a libpq generated PGresult will core dump 
due to the pqAddTuple issue, unless the append operation forces 
PQsetvalue to grow the tuple table; in which case new elements are 
zero'd.  OP attempted to append.

res = PQexec("returns 2 tuples");
PQsetvalue(res, PQntuples(res), ...);

-- 
Andrew Chernow
eSilo, LLC
global backup
http://www.esilo.com/

  7. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-03T21:54:17Z

    On Fri, Jun 3, 2011 at 3:38 PM, Andrew Chernow <ac@esilo.com> wrote:

> Eeekks.  Found an additional bug.  PQsetvalue only allocates the actual
> tuple if the provided tup_num equals the number of tuples (append) and that
> slot is NULL.  This is wrong.  The original idea behind PQsetvalue was you
> can add tuples in any order and overwrite existing ones.

That was by design -- you are only supposed to be able to add a tuple
if you pass in exactly the insertion position (which is the same as
PQntuples()).  If you pass less than that, you will overwrite the
value at that position.  If you pass greater, you should get an error.
 This is also how the function is documented.  That's why you don't
have to zero out the tuple slots at all -- the insertion position is
always known and you just need to make sure the tuple atts are not
allocated more than one time per inserted tuple.  Meaning, PQsetvalue
needs to work more like pqAddTuple (and the insertion code should
probably be abstracted).

merlin

  8. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-03T22:22:06Z

    On 6/3/2011 5:54 PM, Merlin Moncure wrote:
> On Fri, Jun 3, 2011 at 3:38 PM, Andrew Chernow<ac@esilo.com>  wrote:
>
>> Eeekks.  Found an additional bug.  PQsetvalue only allocates the actual
>> tuple if the provided tup_num equals the number of tuples (append) and that
>> slot is NULL.  This is wrong.  The original idea behind PQsetvalue was you
>> can add tuples in any order and overwrite existing ones.
>
> That was by design -- you are only supposed to be able to add a tuple
> if you pass in exactly the insertion position (which is the same as
> PQntuples()).  If you pass less than that, you will overwrite the
> value at that position.  If you pass greater, you should get an error.

Actually, the original idea, as I stated UT, was to allow adding tuples 
in any order as well as overwriting them.  Obviously lost that while 
trying to get libpqtypes working, which only appends.

>   This is also how the function is documented.  That's why you don't
> have to zero out the tuple slots at all -- the insertion position is
> always known and you just need to make sure the tuple atts are not
> allocated more than one time per inserted tuple.  Meaning, PQsetvalue
> needs to work more like pqAddTuple (and the insertion code should
> probably be abstracted).
>

You need to have insertion point zero'd in either case.  Look at the 
code.  It only allocates when appending *AND* the tuple at that index is 
NULL.  If pqAddTuple allocated the table, the tuple slots are 
uninitialized memory, thus it is very unlikely that the next tuple slot 
is NULL.

It is trivial to make this work the way it was initially intended (which 
mimics PQgetvalue in that you can fetch values in any order, that was 
the goal).  Are there any preferences?  I plan to supply a patch that 
allows setting values in any order as well as overwriting unless someone 
speaks up.  I fix the docs as well.

-- 
Andrew Chernow
eSilo, LLC
global backup
http://www.esilo.com/

  9. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-03T23:14:09Z

    On Fri, Jun 3, 2011 at 6:22 PM, Andrew Chernow <ac@esilo.com> wrote:
> Actually, the original idea, as I stated UT, was to allow adding tuples in
> any order as well as overwriting them.  Obviously lost that while trying to
> get libpqtypes working, which only appends.

Well, that may or not be the case, but that's irrelevant.  This has to
be backpatched to 9.0 and we can't use a bug to sneak in a behavior
change...regardless of what's done, it has to work as documented --
the current behavior isn't broken.  If we want PQsetvalue to support
creating tuples past the insertion point, that should be proposed for
9.2.

> You need to have insertion point zero'd in either case.  Look at the code.
>  It only allocates when appending *AND* the tuple at that index is NULL.  If
> pqAddTuple allocated the table, the tuple slots are uninitialized memory,
> thus it is very unlikely that the next tuple slot is NULL.

I disagree -- I think the fix is a one-liner.  line 446:
if (tup_num == res->ntups && !res->tuples[tup_num])

should just become
if (tup_num == res->ntups)

also the memset of the tuple slots when the slot array is expanded can
be removed. (in addition, the array tuple array expansion should
really be abstracted, but that isn't strictly necessary here).

merlin

  10. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-03T23:46:08Z

    On 6/3/2011 7:14 PM, Merlin Moncure wrote:
> On Fri, Jun 3, 2011 at 6:22 PM, Andrew Chernow<ac@esilo.com>  wrote:
>> Actually, the original idea, as I stated UT, was to allow adding tuples in
>> any order as well as overwriting them.  Obviously lost that while trying to
>> get libpqtypes working, which only appends.
>
> Well, that may or not be the case, but that's irrelevant.  This has to
> be backpatched to 9.0 and we can't use a bug to sneak in a behavior
> change...regardless of what's done, it has to work as documented --
> the current behavior isn't broken.  If we want PQsetvalue to support
> creating tuples past the insertion point, that should be proposed for
> 9.2.
>

Well, not really irrelevant since understanding the rationale behind changes is 
important.  I actually reversed my opinion on this and was preparing a patch 
that simply did a memset in pqAddTuple.  See below for why....

>> You need to have insertion point zero'd in either case.  Look at the code.
>>   It only allocates when appending *AND* the tuple at that index is NULL.  If
>> pqAddTuple allocated the table, the tuple slots are uninitialized memory,
>> thus it is very unlikely that the next tuple slot is NULL.
>
> I disagree -- I think the fix is a one-liner.  line 446:
> if (tup_num == res->ntups&&  !res->tuples[tup_num])
>
> should just become
> if (tup_num == res->ntups)
>
> also the memset of the tuple slots when the slot array is expanded can
> be removed. (in addition, the array tuple array expansion should
> really be abstracted, but that isn't strictly necessary here).
>

All true.  This is a cleaner fix to something that was in fact broken ;)  You 
want to apply it?

The fact that the tuples were being zero'd plus the NULL check is evidence of 
the original intent; which is what confused me.  I found internal libpqtypes 
notes related to this change, it actually had to do with producing a result with 
dead tuples that would cause PQgetvalue and others to crash.  Thus, it seemed 
better to only allow creating a result that is always *valid*.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

  11. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-04T02:26:26Z

    >> I disagree -- I think the fix is a one-liner. line 446:
>> if (tup_num == res->ntups&& !res->tuples[tup_num])
>>
>> should just become
>> if (tup_num == res->ntups)
>>
>> also the memset of the tuple slots when the slot array is expanded can
>> be removed. (in addition, the array tuple array expansion should
>> really be abstracted, but that isn't strictly necessary here).
>>
>
> All true. This is a cleaner fix to something that was in fact broken ;) You want

Attached a patch that fixes the OP's issue.  PQsetvalue now uses pqAddTuple to 
grow the tuple table and has removed the remnants of an older idea that caused 
the bug.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

  12. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-04T02:36:37Z

    On 6/3/2011 10:26 PM, Andrew Chernow wrote:
>
>>> I disagree -- I think the fix is a one-liner. line 446:
>>> if (tup_num == res->ntups&& !res->tuples[tup_num])
>>>
>>> should just become
>>> if (tup_num == res->ntups)
>>>
>>> also the memset of the tuple slots when the slot array is expanded can
>>> be removed. (in addition, the array tuple array expansion should
>>> really be abstracted, but that isn't strictly necessary here).
>>>
>>
>> All true. This is a cleaner fix to something that was in fact broken ;) You want
>
> Attached a patch that fixes the OP's issue. PQsetvalue now uses pqAddTuple to
> grow the tuple table and has removed the remnants of an older idea that caused
> the bug.
>

Sorry, I attached the wrong patch.  Here is the correct one.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

  13. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-04T12:45:50Z

    On Fri, Jun 3, 2011 at 10:36 PM, Andrew Chernow <ac@esilo.com> wrote:
> On 6/3/2011 10:26 PM, Andrew Chernow wrote:
>>
>>>> I disagree -- I think the fix is a one-liner. line 446:
>>>> if (tup_num == res->ntups&& !res->tuples[tup_num])
>>>>
>>>> should just become
>>>> if (tup_num == res->ntups)
>>>>
>>>> also the memset of the tuple slots when the slot array is expanded can
>>>> be removed. (in addition, the array tuple array expansion should
>>>> really be abstracted, but that isn't strictly necessary here).
>>>>
>>>
>>> All true. This is a cleaner fix to something that was in fact broken ;)
>>> You want
>>
>> Attached a patch that fixes the OP's issue. PQsetvalue now uses pqAddTuple
>> to
>> grow the tuple table and has removed the remnants of an older idea that
>> caused
>> the bug.
>>
>
> Sorry, I attached the wrong patch.  Here is the correct one.


This looks good.  Pavel, want to test it?

merlin

  14. Re: Error in PQsetvalue

    Pavel Golub <pavel@microolap.com> — 2011-06-06T12:09:15Z

    Hello, guys.

You wrote:

MM> On Fri, Jun 3, 2011 at 10:36 PM, Andrew Chernow <ac@esilo.com> wrote:
>> On 6/3/2011 10:26 PM, Andrew Chernow wrote:
>>>
>>>>> I disagree -- I think the fix is a one-liner. line 446:
>>>>> if (tup_num == res->ntups&& !res->tuples[tup_num])
>>>>>
>>>>> should just become
>>>>> if (tup_num == res->ntups)
>>>>>
>>>>> also the memset of the tuple slots when the slot array is expanded can
>>>>> be removed. (in addition, the array tuple array expansion should
>>>>> really be abstracted, but that isn't strictly necessary here).
>>>>>
>>>>
>>>> All true. This is a cleaner fix to something that was in fact broken ;)
>>>> You want
>>>
>>> Attached a patch that fixes the OP's issue. PQsetvalue now uses pqAddTuple
>>> to
>>> grow the tuple table and has removed the remnants of an older idea that
>>> caused
>>> the bug.
>>>
>>
>> Sorry, I attached the wrong patch.  Here is the correct one.


MM> This looks good.  Pavel, want to test it?

Sorry for delay in answer. Yeah, I'm glad to. Should I apply this
patch by myself?

MM> merlin



-- 
With best wishes,
 Pavel                          mailto:pavel@gf.microolap.com

  15. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-06T18:42:06Z

    On Mon, Jun 6, 2011 at 7:09 AM, Pavel Golub <pavel@microolap.com> wrote:
> Sorry for delay in answer. Yeah, I'm glad to. Should I apply this
> patch by myself?

sure, run it against your test case and make sure it works. if so, we
can pass it up the chain of command (hopefully as context diff).

merlin

  16. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-08T15:13:47Z

    On Mon, Jun 6, 2011 at 1:42 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Mon, Jun 6, 2011 at 7:09 AM, Pavel Golub <pavel@microolap.com> wrote:
>> Sorry for delay in answer. Yeah, I'm glad to. Should I apply this
>> patch by myself?
>
> sure, run it against your test case and make sure it works. if so, we
> can pass it up the chain of command (hopefully as context diff).

I went ahead and tested andrew's second patch -- can we get this
reviewed and committed?

merlin

  17. Re: Error in PQsetvalue

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-08T15:18:56Z

    Merlin Moncure <mmoncure@gmail.com> writes:
> I went ahead and tested andrew's second patch -- can we get this
> reviewed and committed?

Add it to the upcoming commitfest.

			regards, tom lane

  18. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-08T15:46:38Z

    On Wed, Jun 8, 2011 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> I went ahead and tested andrew's second patch -- can we get this
>> reviewed and committed?
>
> Add it to the upcoming commitfest.

It's a client crashing bug in PQsetvalue that goes back to 9.0 :(.  In
short (apologies for the non-context diff), PQsetvalue was
inappropriately using libpq tuple slots as a check to see if it should
allocate the per row tuple datums, and borked when setting values on a
non copied result (libpqtypes always copies results) because the
regular pqAddTuple does not null out the slots like copy result does.
The whole mechanism was basically unnecessary, so it was removed,
fixing the bug.

merlin

  19. Re: Error in PQsetvalue

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-08T16:03:28Z

    Merlin Moncure <mmoncure@gmail.com> writes:
> On Wed, Jun 8, 2011 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Merlin Moncure <mmoncure@gmail.com> writes:
>>> I went ahead and tested andrew's second patch -- can we get this
>>> reviewed and committed?

>> Add it to the upcoming commitfest.

> It's a client crashing bug in PQsetvalue that goes back to 9.0 :(.

I was under the impression that this was extending PQsetvalue to let it
be used in previously unsupported ways, ie, to modify a server-returned
PGresult.  That's a feature addition, not a bug fix.  I'm not even sure
it's a feature addition I approve of.  I think serious consideration
ought to be given to locking down returned results so PQsetvalue refuses
to touch them, instead.  Otherwise we're likely to find ourselves unable
to make future optimizations because we have to support this
barely-used-by-anybody corner case.

			regards, tom lane

  20. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-08T16:39:12Z

    On Wed, Jun 8, 2011 at 11:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> On Wed, Jun 8, 2011 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Merlin Moncure <mmoncure@gmail.com> writes:
>>>> I went ahead and tested andrew's second patch -- can we get this
>>>> reviewed and committed?
>
>>> Add it to the upcoming commitfest.
>
>> It's a client crashing bug in PQsetvalue that goes back to 9.0 :(.
>
> I was under the impression that this was extending PQsetvalue to let it
> be used in previously unsupported ways, ie, to modify a server-returned
> PGresult.  That's a feature addition, not a bug fix.

It's neither -- it's documented libpq behavior: "The function will
automatically grow the result's internal tuples array as needed.
However, the tup_num argument must be less than or equal to PQntuples,
meaning this function can only grow the tuples array one tuple at a
time. But any field of any existing tuple can be modified in any
order. "

Andrew was briefly flirting with a proposal to tweak this behavior,
but withdrew the idea.

> it's a feature addition I approve of.  I think serious consideration
> ought to be given to locking down returned results so PQsetvalue refuses
> to touch them, instead.  Otherwise we're likely to find ourselves unable
> to make future optimizations because we have to support this
> barely-used-by-anybody corner case.

I think that's debatable, but I'm not going to argue this yea or nea.
But I will say that maybe we shouldn't confuse behavior issues with
bug fix either way...patch the bug, and we can work up a patch to lock
down the behavior and the docs if you want it that way, but maybe we
could bikeshed a bit on that point.

merlin

  21. Re: Error in PQsetvalue

    Andrew Chernow <ac@esilo.com> — 2011-06-08T16:44:37Z

    On 6/8/2011 12:03 PM, Tom Lane wrote:
> Merlin Moncure<mmoncure@gmail.com>  writes:
>> On Wed, Jun 8, 2011 at 10:18 AM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>>> Merlin Moncure<mmoncure@gmail.com>  writes:
>>>> I went ahead and tested andrew's second patch -- can we get this
>>>> reviewed and committed?
>
>>> Add it to the upcoming commitfest.
>
>> It's a client crashing bug in PQsetvalue that goes back to 9.0 :(.
>
> I was under the impression that this was extending PQsetvalue to let it
> be used in previously unsupported ways, ie, to modify a server-returned

Well, it was supposed to support that but the implementation is busted 
(sorry) and core dumps instead.

> PGresult.  That's a feature addition, not a bug fix.  I'm not even sure
> it's a feature addition I approve of.  I think serious consideration
> ought to be given to locking down returned results so PQsetvalue refuses

I don't disagree at all, but I do think this is a bit more involved of a 
change.  Several functions like PQmakeEmptyPGresult, PQsetvalue, 
PQcopyResult (one used by libpqtypes), the PGresult object needs a bool 
member and probably others things all must be aware of the distinction 
bwteen client and server generated results.  That is a little tricky 
because both use PQmakeEmptyPGresult that has no argument to indicate that.

Since libpqtypes only calls PQcopyResult, you could just set a flag on 
the result in that function that PQsetvalue uses as a guard.  However, 
this hard codes knowledge about the libpqtypes calling pattern which is 
rather weak.

Maybe it would be better to just apply the patch (since it also removes 
duplicate code from pqAddTuple in addition to fixing a crash) and update 
the docs to say this is an unsupported feature, don't do it.  If it 
happens to work forever or just for a while, than so be it.

-- 
Andrew Chernow
eSilo, LLC
global backup
http://www.esilo.com/

  22. Re: Error in PQsetvalue

    Pavel Golub <pavel@microolap.com> — 2011-06-09T05:48:30Z

    Hello, Merlin.

You wrote:

MM> On Wed, Jun 8, 2011 at 11:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Merlin Moncure <mmoncure@gmail.com> writes:
>>> On Wed, Jun 8, 2011 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Merlin Moncure <mmoncure@gmail.com> writes:
>>>>> I went ahead and tested andrew's second patch -- can we get this
>>>>> reviewed and committed?
>>
>>>> Add it to the upcoming commitfest.
>>
>>> It's a client crashing bug in PQsetvalue that goes back to 9.0 :(.
>>
>> I was under the impression that this was extending PQsetvalue to let it
>> be used in previously unsupported ways, ie, to modify a server-returned
>> PGresult.  That's a feature addition, not a bug fix.

MM> It's neither -- it's documented libpq behavior: "The function will
MM> automatically grow the result's internal tuples array as needed.
MM> However, the tup_num argument must be less than or equal to PQntuples,
MM> meaning this function can only grow the tuples array one tuple at a
MM> time. But any field of any existing tuple can be modified in any
MM> order. "

MM> Andrew was briefly flirting with a proposal to tweak this behavior,
MM> but withdrew the idea.

>> it's a feature addition I approve of.  I think serious consideration
>> ought to be given to locking down returned results so PQsetvalue refuses
>> to touch them, instead.  Otherwise we're likely to find ourselves unable
>> to make future optimizations because we have to support this
>> barely-used-by-anybody corner case.

Do I understand correctly that there is no any chance at all to have function
like PQdeleteTuple in libpq? (see my message "PQdeleteTuple
function in libpq" on Wed, 1 Jun 2011)

MM> I think that's debatable, but I'm not going to argue this yea or nea.
MM> But I will say that maybe we shouldn't confuse behavior issues with
MM> bug fix either way...patch the bug, and we can work up a patch to lock
MM> down the behavior and the docs if you want it that way, but maybe we
MM> could bikeshed a bit on that point.

MM> merlin



-- 
With best wishes,
 Pavel                          mailto:pavel@gf.microolap.com

  23. Re: Error in PQsetvalue

    Merlin Moncure <mmoncure@gmail.com> — 2011-06-09T20:40:08Z

    On Thu, Jun 9, 2011 at 12:48 AM, Pavel Golub <pavel@microolap.com> wrote:
>>> it's a feature addition I approve of.  I think serious consideration
>>> ought to be given to locking down returned results so PQsetvalue refuses
>>> to touch them, instead.  Otherwise we're likely to find ourselves unable
>>> to make future optimizations because we have to support this
>>> barely-used-by-anybody corner case.
>
> Do I understand correctly that there is no any chance at all to have function
> like PQdeleteTuple in libpq? (see my message "PQdeleteTuple
> function in libpq" on Wed, 1 Jun 2011)

It means the feature is on thin ice.  I'm personally in favor of
keeping it, and perhaps cautiously exploring ways to go further.  I'm
waiting for Tom to make a call...I see three options:

1) patch bug and leave behavior alone (apply andrew C's patch)
2) discuss behavior change in -hackers
3) patch bug and behavior immediately (get a new patch with doc change as well)

merlin

  24. Re: Error in PQsetvalue

    Pavel Golub <pavel@microolap.com> — 2011-07-18T10:24:49Z

    Hello, Andrew.

I hope you don't mind I've added this patch to CommitFest:
https://commitfest.postgresql.org/action/patch_view?id=606

You wrote:

AC> On 6/3/2011 10:26 PM, Andrew Chernow wrote:
>>
>>>> I disagree -- I think the fix is a one-liner. line 446:
>>>> if (tup_num == res->ntups&& !res->tuples[tup_num])
>>>>
>>>> should just become
>>>> if (tup_num == res->ntups)
>>>>
>>>> also the memset of the tuple slots when the slot array is expanded can
>>>> be removed. (in addition, the array tuple array expansion should
>>>> really be abstracted, but that isn't strictly necessary here).
>>>>
>>>
>>> All true. This is a cleaner fix to something that was in fact broken ;) You want
>>
>> Attached a patch that fixes the OP's issue. PQsetvalue now uses pqAddTuple to
>> grow the tuple table and has removed the remnants of an older idea that caused
>> the bug.
>>

AC> Sorry, I attached the wrong patch.  Here is the correct one.




-- 
With best wishes,
 Pavel                          mailto:pavel@gf.microolap.com