Re: Error in PQsetvalue

On 6/3/2011 7:14 PM, Merlin Moncure wrote:
> On Fri, Jun 3, 2011 at 6:22 PM, Andrew Chernow<ac(at)esilo(dot)com> wrote:
>> Actually, the original idea, as I stated UT, was to allow adding tuples in
>> any order as well as overwriting them. Obviously lost that while trying to
>> get libpqtypes working, which only appends.
>
> Well, that may or not be the case, but that's irrelevant. This has to
> be backpatched to 9.0 and we can't use a bug to sneak in a behavior
> change...regardless of what's done, it has to work as documented --
> the current behavior isn't broken. If we want PQsetvalue to support
> creating tuples past the insertion point, that should be proposed for
> 9.2.
>

Well, not really irrelevant since understanding the rationale behind changes is
important. I actually reversed my opinion on this and was preparing a patch
that simply did a memset in pqAddTuple. See below for why....

>> You need to have insertion point zero'd in either case. Look at the code.
>> It only allocates when appending *AND* the tuple at that index is NULL. If
>> pqAddTuple allocated the table, the tuple slots are uninitialized memory,
>> thus it is very unlikely that the next tuple slot is NULL.
>
> I disagree -- I think the fix is a one-liner. line 446:
> if (tup_num == res->ntups&& !res->tuples[tup_num])
>
> should just become
> if (tup_num == res->ntups)
>
> also the memset of the tuple slots when the slot array is expanded can
> be removed. (in addition, the array tuple array expansion should
> really be abstracted, but that isn't strictly necessary here).
>

All true. This is a cleaner fix to something that was in fact broken ;) You
want to apply it?

The fact that the tuples were being zero'd plus the NULL check is evidence of
the original intent; which is what confused me. I found internal libpqtypes
notes related to this change, it actually had to do with producing a result with
dead tuples that would cause PQgetvalue and others to crash. Thus, it seemed
better to only allow creating a result that is always *valid*.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/