Re: [HACKERS] fixing PQsetvalue()
On Wed, Jul 20, 2011 at 10:28 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub pa...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
On Thu, Jul 21, 2011 at 12:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub pa...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
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 a...@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:pa...@gf.microolap.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
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/msg0.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:pa...@gf.microolap.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
2011/6/23 Merlin Moncure mmonc...@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/msg0.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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- // Dmitriy.
Re: [HACKERS] fixing PQsetvalue()
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/msg0.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:pa...@gf.microolap.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fixing PQsetvalue()
2011/6/23 Andrew Chernow a...@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. -- Andrew Chernow eSilo, LLC global backup http://www.esilo.com/ -- // Dmitriy.
Re: [HACKERS] fixing PQsetvalue()
On Thu, Jun 23, 2011 at 7:54 AM, Andrew Chernow a...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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/msg0.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
