Re: Error in PQsetvalue

On 6/8/2011 12:03 PM, Tom Lane wrote:
> Merlin Moncure<mmoncure(at)gmail(dot)com> writes:
>> On Wed, Jun 8, 2011 at 10:18 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Merlin Moncure<mmoncure(at)gmail(dot)com> writes:
>>>> I went ahead and tested andrew's second patch -- can we get this
>>>> reviewed and committed?
>
>>> Add it to the upcoming commitfest.
>
>> It's a client crashing bug in PQsetvalue that goes back to 9.0 :(.
>
> I was under the impression that this was extending PQsetvalue to let it
> be used in previously unsupported ways, ie, to modify a server-returned

Well, it was supposed to support that but the implementation is busted
(sorry) and core dumps instead.

> PGresult. That's a feature addition, not a bug fix. I'm not even sure
> it's a feature addition I approve of. I think serious consideration
> ought to be given to locking down returned results so PQsetvalue refuses

I don't disagree at all, but I do think this is a bit more involved of a
change. Several functions like PQmakeEmptyPGresult, PQsetvalue,
PQcopyResult (one used by libpqtypes), the PGresult object needs a bool
member and probably others things all must be aware of the distinction
bwteen client and server generated results. That is a little tricky
because both use PQmakeEmptyPGresult that has no argument to indicate that.

Since libpqtypes only calls PQcopyResult, you could just set a flag on
the result in that function that PQsetvalue uses as a guard. However,
this hard codes knowledge about the libpqtypes calling pattern which is
rather weak.

Maybe it would be better to just apply the patch (since it also removes
duplicate code from pqAddTuple in addition to fixing a crash) and update
the docs to say this is an unsupported feature, don't do it. If it
happens to work forever or just for a while, than so be it.

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