Обсуждение: 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@gmail.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.
(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@postgresql.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@gf.microolap.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 callsPQmakeEmptyPGresult. 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@esilo.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.
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@esilo.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@gf.microolap.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@esilo.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@gf.microolap.com
On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub <pavel@microolap.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@gmail.com> wrote: > On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub <pavel@microolap.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@gmail.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.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