FAQ
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

Search Discussions

  • Dmitriy Igrishin at Jun 23, 2011 at 12:27 pm
    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.

    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.
  • Andrew Chernow at Jun 23, 2011 at 12:54 pm

    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/
  • Dmitriy Igrishin at Jun 23, 2011 at 1:41 pm
    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.

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


    --
    // Dmitriy.
  • Merlin Moncure at Jun 23, 2011 at 1:41 pm

    On Thu, Jun 23, 2011 at 7:54 AM, Andrew Chernow 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
  • Pavel Golub at Jul 18, 2011 at 10:38 am
    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 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
  • Robert Haas at Jul 21, 2011 at 3:29 am

    On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub 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
  • Merlin Moncure at Jul 21, 2011 at 4:11 pm

    On Wed, Jul 20, 2011 at 10:28 PM, Robert Haas wrote:
    On Mon, Jul 18, 2011 at 6:38 AM, Pavel Golub 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
  • Tom Lane at Jul 21, 2011 at 4:20 pm

    Robert Haas 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
  • Robert Haas at Jul 21, 2011 at 5:58 pm

    On Thu, Jul 21, 2011 at 12:19 PM, Tom Lane 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
  • Pavel Golub at Jun 23, 2011 at 12:33 pm
    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
  • Pavel Golub at Jul 6, 2011 at 7:54 am
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJun 22, '11 at 8:52p
activeJul 21, '11 at 5:58p
posts12
users6
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase