Обсуждение: Error in PQsetvalue
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.
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 nevertried 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/
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
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 onlyone tuple can be allocated at a time, and it always has to be the next one after the current set. merlin
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/
>>
>> 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/
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'swhy 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
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/
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
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/
>> 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/
Вложения
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/
Вложения
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
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
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
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
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
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
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
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
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/
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
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
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