Re: Valgrind-detected bug in partitioning code

On Fri, Jan 20, 2017 at 4:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> Hmm. That's bad. I kind of wonder how sane it is to think that we
>>>> can invoke SQL-callable functions during a relcache reload,
>
>>> You're doing WHAT?
>
>> Uh. +1.
>
> Now that I've calmed down a bit: the right way to do this sort of thing is
> simply to flush the invalidated data during reload, and recompute it when
> it is next requested, which necessarily will be inside a valid
> transaction. Compare e.g. the handling of the lists of a relation's
> indexes.

The existing handling of partition descriptors is modeled on and very
similar to the existing handling for other types of objects:

keep_tupdesc = equalTupleDescs(relation->rd_att,
newrel->rd_att);
keep_rules = equalRuleLocks(relation->rd_rules,
newrel->rd_rules);
keep_policies = equalRSDesc(relation->rd_rsdesc,
newrel->rd_rsdesc);
keep_partkey = (relation->rd_partkey != NULL);
keep_partdesc = equalPartitionDescs(relation->rd_partkey,

relation->rd_partdesc,

newrel->rd_partdesc);

And I think the reason is the same too, namely, if we've got a pointer
into partition descriptor in the relcache, we don't want that to
suddenly get swapped out and replaced with a pointer to an equivalent
data structure at a different address, because then our pointer will
be dangling. That seems fine as far as it goes.

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]))

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?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company