Re: [HACKERS] Valgrind-detected bug in partitioning code

On 2017/01/21 9:01, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> The difference is that those other equalBLAH functions call a
>> carefully limited amount of code whereas, in looking over the
>> backtrace you sent, I realized that equalPartitionDescs is calling
>> partition_bounds_equal which does this:
>>                        cmpval =
>> DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[j],
>>                                   key->partcollation[j],
>>                                   b1->datums[i][j],
>>                                   b2->datums[i][j]))
> 
> Ah, gotcha.
> 
>> That's of course opening up a much bigger can of worms.  But apart
>> from the fact that it's unsafe, I think it's also wrong, as I said
>> upthread.  I think calling datumIsEqual() there should be better all
>> around.  Do you think that's unsafe here?
> 
> That sounds like a plausible solution.  It is safe in the sense of
> being a bounded amount of code.  It would return "false" in various
> interesting cases like toast pointer versus detoasted equivalent,
> but I think that would be fine in this application.

Sorry for jumping in late.  Attached patch replaces the call to
partitioning-specific comparison function by the call to datumIsEqual().
I wonder if it is safe to assume that datumIsEqual() would return true for
a datum and copy of it made using datumCopy().  The latter is used to copy
a single datum from a bound's Const node (what is stored in the catalog
for every bound).

> It would probably be a good idea to add something to datumIsEqual's
> comment to the effect that trying to make it smarter would be a bad idea,
> because some callers rely on it being stupid.

I assume "making datumIsEqual() smarter" here means to make it account for
toasting of varlena datums, which is not a good idea because some of its
callers may be working in the context of an aborted transaction.  I tried
to update the header comment along these lines, though please feel to
rewrite it.

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers