fixing PQsetvalue()

On Jun 6 (http://archives.postgresql.org/pgsql-hackers/2011-06/msg00272.php),
Pavel discovered an issue with PQsetvalue that could cause libpq to
wander off into unallocated memory that was present in 9.0.x. A
fairly uninteresting fix was quickly produced, but Tom indicated
during subsequent review that he was not happy with the behavior of
the function. Everyone was busy with the beta wrap at the time so I
didn't press the issue.

A little history here: PQsetvalue's
(http://www.postgresql.org/docs/9.0/static/libpq-misc.html) main
reason to exist is to allow creating a PGresult out of scratch data on
a result created via PQmakeEmptyResult(). This behavior works as
intended and is not controversial...it was initially done to support
libpqtypes but has apparently found use in other places like ecpg.

PQsetvalue *also* allows you to mess with results returned by the
server using standard query methods for any tuple, not just the one
you are creating as you iterate through. This behavior was
unnecessary in terms of what libpqtypes and friends needed and may (as
Tom suggested) come back to bite us at some point. As it turns out,
PQsetvalue's operation on results that weren't created via
PQmakeEmptyResult was totally busted because of the bug, so we have a
unique opportunity to tinker with libpq here: you could argue that you
have a window of opportunity to change the behavior here since we know
it isn't being usefully used. I think it's too late for that but it's
if it had to be done the time is now.

Pavel actually has been requesting to go further with being able to
mess around with PGresults (see:
http://archives.postgresql.org/pgsql-interfaces/2011-06/msg00000.php),
such that the result would start to have some 'recordset' features you
find in various other libraries. Maybe it's not the job of libpq to
do that, but I happen to think it's pretty cool. Anyways, something
has to be done -- I hate to see an unpatched known 9.0 issue remain in
the wild.

merlin

2011/6/23 Merlin Moncure <mmoncure(at)gmail(dot)com>

> On Jun 6 (
> http://archives.postgresql.org/pgsql-hackers/2011-06/msg00272.php),
> Pavel discovered an issue with PQsetvalue that could cause libpq to
> wander off into unallocated memory that was present in 9.0.x. A
> fairly uninteresting fix was quickly produced, but Tom indicated
> during subsequent review that he was not happy with the behavior of
> the function. Everyone was busy with the beta wrap at the time so I
> didn't press the issue.
>
> A little history here: PQsetvalue's
> (http://www.postgresql.org/docs/9.0/static/libpq-misc.html) main
> reason to exist is to allow creating a PGresult out of scratch data on
> a result created via PQmakeEmptyResult(). This behavior works as
> intended and is not controversial...it was initially done to support
> libpqtypes but has apparently found use in other places like ecpg.
>
> PQsetvalue *also* allows you to mess with results returned by the
> server using standard query methods for any tuple, not just the one
> you are creating as you iterate through. This behavior was
> unnecessary in terms of what libpqtypes and friends needed and may (as
> Tom suggested) come back to bite us at some point. As it turns out,
> PQsetvalue's operation on results that weren't created via
> PQmakeEmptyResult was totally busted because of the bug, so we have a
> unique opportunity to tinker with libpq here: you could argue that you
> have a window of opportunity to change the behavior here since we know
> it isn't being usefully used. I think it's too late for that but it's
> if it had to be done the time is now.
>
> Pavel actually has been requesting to go further with being able to
> mess around with PGresults (see:
> http://archives.postgresql.org/pgsql-interfaces/2011-06/msg00000.php),
> such that the result would start to have some 'recordset' features you
> find in various other libraries. Maybe it's not the job of libpq to
> do that, but I happen to think it's pretty cool. Anyways, something
> has to be done -- I hate to see an unpatched known 9.0 issue remain in
> the wild.
>
+1

Exactly at this moment I am thinking about using modifiable
(via PQsetvalue) PGresult instead of std::map in my C++ library
for store parameters for binding to executing command.
I am already designed how to implement it, and I supposed that
PQsetvalue is intended to work with any PGresult and not only
with those which has been created via PQmakeEmptyResult...
So, I am absolutely sure, that PQsetvalue should works with
any PGresult.

> merlin
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
// Dmitriy.

Hello, Merlin.

You wrote:

MM> On Jun 6
MM> (http://archives.postgresql.org/pgsql-hackers/2011-06/msg00272.php),
MM> Pavel discovered an issue with PQsetvalue that could cause libpq to
MM> wander off into unallocated memory that was present in 9.0.x. A
MM> fairly uninteresting fix was quickly produced, but Tom indicated
MM> during subsequent review that he was not happy with the behavior of
MM> the function. Everyone was busy with the beta wrap at the time so I
MM> didn't press the issue.

MM> A little history here: PQsetvalue's
MM> (http://www.postgresql.org/docs/9.0/static/libpq-misc.html) main
MM> reason to exist is to allow creating a PGresult out of scratch data on
MM> a result created via PQmakeEmptyResult(). This behavior works as
MM> intended and is not controversial...it was initially done to support
MM> libpqtypes but has apparently found use in other places like ecpg.

MM> PQsetvalue *also* allows you to mess with results returned by the
MM> server using standard query methods for any tuple, not just the one
MM> you are creating as you iterate through. This behavior was
MM> unnecessary in terms of what libpqtypes and friends needed and may (as
MM> Tom suggested) come back to bite us at some point. As it turns out,
MM> PQsetvalue's operation on results that weren't created via
MM> PQmakeEmptyResult was totally busted because of the bug, so we have a
MM> unique opportunity to tinker with libpq here: you could argue that you
MM> have a window of opportunity to change the behavior here since we know
MM> it isn't being usefully used. I think it's too late for that but it's
MM> if it had to be done the time is now.

MM> Pavel actually has been requesting to go further with being able to
MM> mess around with PGresults (see:
MM> http://archives.postgresql.org/pgsql-interfaces/2011-06/msg00000.php),
MM> such that the result would start to have some 'recordset' features you
MM> find in various other libraries. Maybe it's not the job of libpq to
MM> do that, but I happen to think it's pretty cool. Anyways, something
MM> has to be done -- I hate to see an unpatched known 9.0 issue remain in
MM> the wild.

MM> merlin

I confirm my desire to have delete tuple routine. And I'm ready to
create\test\modify any sources for this.

--
With best wishes,
Pavel mailto:pavel(at)gf(dot)microolap(dot)com

> you are creating as you iterate through. This behavior was
> unnecessary in terms of what libpqtypes and friends needed and may (as
> Tom suggested) come back to bite us at some point. As it turns out,
> PQsetvalue's operation on results that weren't created via
> PQmakeEmptyResult was totally busted because of the bug, so we have a
> unique opportunity to tinker with libpq here: you could argue that you
>
> +1
>
> Exactly at this moment I am thinking about using modifiable
> (via PQsetvalue) PGresult instead of std::map in my C++ library
> for store parameters for binding to executing command.
> I am already designed how to implement it, and I supposed that
> PQsetvalue is intended to work with any PGresult and not only
> with those which has been created via PQmakeEmptyResult...
> So, I am absolutely sure, that PQsetvalue should works with
> any PGresult.

All PGresults are created via PQmakeEmptyPGresult, including libpqtypes.
Actually, libpqtypes calls PQcopyResult which calls PQmakeEmptyPGresult.

It might be better to say a "server" result vs. a "client" result.
Currently, PQsetvalue is broken when provided a "server" generated result.

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

2011/6/23 Andrew Chernow <ac(at)esilo(dot)com>

> you are creating as you iterate through. This behavior was
>> unnecessary in terms of what libpqtypes and friends needed and may (as
>> Tom suggested) come back to bite us at some point. As it turns out,
>> PQsetvalue's operation on results that weren't created via
>> PQmakeEmptyResult was totally busted because of the bug, so we have a
>> unique opportunity to tinker with libpq here: you could argue that you
>>
>> +1
>>
>> Exactly at this moment I am thinking about using modifiable
>> (via PQsetvalue) PGresult instead of std::map in my C++ library
>> for store parameters for binding to executing command.
>> I am already designed how to implement it, and I supposed that
>> PQsetvalue is intended to work with any PGresult and not only
>> with those which has been created via PQmakeEmptyResult...
>> So, I am absolutely sure, that PQsetvalue should works with
>> any PGresult.
>>
>
> All PGresults are created via PQmakeEmptyPGresult, including libpqtypes.
> Actually, libpqtypes calls PQcopyResult which calls PQmakeEmptyPGresult.
>
> It might be better to say a "server" result vs. a "client" result.
> Currently, PQsetvalue is broken when provided a "server" generated result.
>
Yeah, yeah. Thanks for clarification!
I am not libpq hacker, just user. So I was thinking that server-returned
result creating by libpq's private functions, rather than public
PQmakeEmptyPGresult...

-1 although if this feature (which I personally thought already exists
according to the
documentation) make future optimizations impossible or hard (as
mentioned by Tom). Otherwise - +1.

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

--
// Dmitriy.

On Thu, Jun 23, 2011 at 7:54 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>    you are creating as you iterate through.  This behavior was
>>    unnecessary in terms of what libpqtypes and friends needed and may (as
>>    Tom suggested) come back to bite us at some point. As it turns out,
>>    PQsetvalue's operation on results that weren't created via
>>    PQmakeEmptyResult was totally busted because of the bug, so we have a
>>    unique opportunity to tinker with libpq here: you could argue that you
>>
>> +1
>>
>> Exactly at this moment I am thinking about using modifiable
>> (via PQsetvalue) PGresult instead of std::map in my C++ library
>> for store parameters for binding to executing command.
>> I am already designed how to implement it, and I supposed that
>> PQsetvalue is intended to work with any PGresult and not only
>> with those which has been created via PQmakeEmptyResult...
>> So, I am absolutely sure, that PQsetvalue should works with
>> any PGresult.
>
> All PGresults are created via PQmakeEmptyPGresult, including libpqtypes.
>  Actually, libpqtypes calls PQcopyResult which calls PQmakeEmptyPGresult.
>
> It might be better to say a "server" result vs. a "client" result.
> Currently, PQsetvalue is broken when provided a "server" generated result.

er, right-- the divergence was in how the tuples were getting added --
thanks for the clarification. essentially your attached patch fixes
that divergence.

merlin

Hello.

Any news on these issues? Becuase beta3 is scheduled for July 11th...

You wrote:

MM> On Jun 6
MM> (http://archives.postgresql.org/pgsql-hackers/2011-06/msg00272.php),
MM> Pavel discovered an issue with PQsetvalue that could cause libpq to
MM> wander off into unallocated memory that was present in 9.0.x. A
MM> fairly uninteresting fix was quickly produced, but Tom indicated
MM> during subsequent review that he was not happy with the behavior of
MM> the function. Everyone was busy with the beta wrap at the time so I
MM> didn't press the issue.

MM> A little history here: PQsetvalue's
MM> (http://www.postgresql.org/docs/9.0/static/libpq-misc.html) main
MM> reason to exist is to allow creating a PGresult out of scratch data on
MM> a result created via PQmakeEmptyResult(). This behavior works as
MM> intended and is not controversial...it was initially done to support
MM> libpqtypes but has apparently found use in other places like ecpg.

MM> PQsetvalue *also* allows you to mess with results returned by the
MM> server using standard query methods for any tuple, not just the one
MM> you are creating as you iterate through. This behavior was
MM> unnecessary in terms of what libpqtypes and friends needed and may (as
MM> Tom suggested) come back to bite us at some point. As it turns out,
MM> PQsetvalue's operation on results that weren't created via
MM> PQmakeEmptyResult was totally busted because of the bug, so we have a
MM> unique opportunity to tinker with libpq here: you could argue that you
MM> have a window of opportunity to change the behavior here since we know
MM> it isn't being usefully used. I think it's too late for that but it's
MM> if it had to be done the time is now.

MM> Pavel actually has been requesting to go further with being able to
MM> mess around with PGresults (see:
MM> http://archives.postgresql.org/pgsql-interfaces/2011-06/msg00000.php),
MM> such that the result would start to have some 'recordset' features you
MM> find in various other libraries. Maybe it's not the job of libpq to
MM> do that, but I happen to think it's pretty cool. Anyways, something
MM> has to be done -- I hate to see an unpatched known 9.0 issue remain in
MM> the wild.

MM> merlin

--
With best wishes,
Pavel mailto:pavel(at)gf(dot)microolap(dot)com

Hello, Merlin.

I hope it's OK that I've added Andrew's patch to CommitFest:
https://commitfest.postgresql.org/action/patch_view?id=606

I did this becuase beta3 already released, but nut nothig is done on
this bug.

You wrote:

MM> On Thu, Jun 23, 2011 at 7:54 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>>    you are creating as you iterate through.  This behavior was
>>>    unnecessary in terms of what libpqtypes and friends needed and may (as
>>>    Tom suggested) come back to bite us at some point. As it turns out,
>>>    PQsetvalue's operation on results that weren't created via
>>>    PQmakeEmptyResult was totally busted because of the bug, so we have a
>>>    unique opportunity to tinker with libpq here: you could argue that you
>>>
>>> +1
>>>
>>> Exactly at this moment I am thinking about using modifiable
>>> (via PQsetvalue) PGresult instead of std::map in my C++ library
>>> for store parameters for binding to executing command.
>>> I am already designed how to implement it, and I supposed that
>>> PQsetvalue is intended to work with any PGresult and not only
>>> with those which has been created via PQmakeEmptyResult...
>>> So, I am absolutely sure, that PQsetvalue should works with
>>> any PGresult.
>>
>> All PGresults are created via PQmakeEmptyPGresult, including libpqtypes.
>>  Actually, libpqtypes calls PQcopyResult which calls PQmakeEmptyPGresult.
>>
>> It might be better to say a "server" result vs. a "client" result.
>> Currently, PQsetvalue is broken when provided a "server" generated result.

MM> er, right-- the divergence was in how the tuples were getting added --
MM> thanks for the clarification. essentially your attached patch fixes
MM> that divergence.

MM> merlin

--
With best wishes,
Pavel mailto:pavel(at)gf(dot)microolap(dot)com

On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub <pavel(at)microolap(dot)com> wrote:
> Hello, Merlin.
>
> I hope it's OK that I've added Andrew's patch to CommitFest:
> https://commitfest.postgresql.org/action/patch_view?id=606
>
> I did this becuase beta3 already released, but nut nothig is done on
> this bug.

So I finally got around to taking a look at this patch, and I guess my
basic feeling is that I like it. The existing code is pretty weird
and inconsistent: the logic in PQsetvalue() basically does the same
thing as the logic in pqAddTuple(), but incompatibly and less
efficiently. Unifying them seems sensible, and the fix looks simple
enough to back-patch.

With respect to Tom's concern about boxing ourselves in, I guess it's
hard for me to get worried about that. I've heard no one suggest
changing the internal representation libpq uses for result sets, and
even if we did, presumably the new format would also need to support
an "append a tuple" operation - or the very worst we could cause it to
support that without much difficulty.

So, +1 from me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jul 20, 2011 at 10:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub <pavel(at)microolap(dot)com> wrote:
>> Hello, Merlin.
>>
>> I hope it's OK that I've added Andrew's patch to CommitFest:
>> https://commitfest.postgresql.org/action/patch_view?id=606
>>
>> I did this becuase beta3 already released, but nut nothig is done on
>> this bug.
>
> So I finally got around to taking a look at this patch, and I guess my
> basic feeling is that I like it.  The existing code is pretty weird
> and inconsistent: the logic in PQsetvalue() basically does the same
> thing as the logic in pqAddTuple(), but incompatibly and less
> efficiently.  Unifying them seems sensible, and the fix looks simple
> enough to back-patch.
>
> With respect to Tom's concern about boxing ourselves in, I guess it's
> hard for me to get worried about that.  I've heard no one suggest
> changing the internal representation libpq uses for result sets, and
> even if we did, presumably the new format would also need to support
> an "append a tuple" operation - or the very worst we could cause it to
> support that without much difficulty.
>
> So, +1 from me.

right -- thanks for that. For the record, I think a rework of the
libpq internal representation would be likely to happen concurrently
with a rework of the API -- for example to better support streaming
data. PQsetvalue very well might prove to be a headache -- just too
hard to say. libpq strikes me as a 50 year plus marriage might:
fractious, full of mystery and regrets, but highly functional.

merlin

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> So I finally got around to taking a look at this patch, and I guess my
> basic feeling is that I like it. The existing code is pretty weird
> and inconsistent: the logic in PQsetvalue() basically does the same
> thing as the logic in pqAddTuple(), but incompatibly and less
> efficiently. Unifying them seems sensible, and the fix looks simple
> enough to back-patch.

Yeah, I've been looking at it too. For some reason I had had the
idea that the proposed patch complicated the code, but actually it's
simplifying it by removing almost-duplicate code. So that's good.

The patch as proposed adds back a bug in return for the one it fixes
(you can not free() the result of pqResultAlloc()), but that's easily
fixed.

Will fix and commit.

regards, tom lane

On Thu, Jul 21, 2011 at 12:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> So I finally got around to taking a look at this patch, and I guess my
>> basic feeling is that I like it.  The existing code is pretty weird
>> and inconsistent: the logic in PQsetvalue() basically does the same
>> thing as the logic in pqAddTuple(), but incompatibly and less
>> efficiently.  Unifying them seems sensible, and the fix looks simple
>> enough to back-patch.
>
> Yeah, I've been looking at it too.  For some reason I had had the
> idea that the proposed patch complicated the code, but actually it's
> simplifying it by removing almost-duplicate code.  So that's good.
>
> The patch as proposed adds back a bug in return for the one it fixes
> (you can not free() the result of pqResultAlloc()), but that's easily
> fixed.
>
> Will fix and commit.

Cool. I believe that's the last patch for CommitFest 2011-06.

*bangs gavel*

I believe that makes it time for 9.2alph1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company