[PostgreSQL-Hackers] fixing PQsetvalue()

2011/6/23 Merlin Moncure <[email protected]>
+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.

2011/6/23 Andrew Chernow <[email protected]>
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.

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, 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:
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

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

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

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

*bangs gavel*

I believe that makes it time for 9.2alph1.

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.

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