Re: Valgrind-detected bug in partitioning code

On 2017/01/21 9:01, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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