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@esilo.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/