Ashutosh Bapat wrote:
> On Tue, Apr 10, 2018 at 1:44 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >
> > Attached fixes it. It teaches RelationBuildPartitionKey() to use
> > fmgr_info_cxt and pass rd_partkeycxt to it.
>
> The patch is using partkeycxt and not rd_partkeycxt. Probably a typo
> in the mail. But a wider question, why that context? I guess that
> cache context will vanish when that cache entry is thrown away. That's
> the reason we have to copy partition key information in
> find_partition_scheme() instead of just pointing to it and also use
> fmgr_info_copy() there. But if that's the case, buildfarm animal run
> with -DCLOBBER_CACHE_ALWAYS should show failure. I am missing
> something here.
Good point. Yeah, it looks like it should cause problems. I think it
would be better to have RelationGetPartitionKey() return a copy in the
caller's context of the data of the relcache data, rather than the
relcache data itself, no? Seems like this would not fail if there never
is a CCI between the RelationGetPartitionKey call and the last access of
the returned key struct ... but if this is the explanation, then it's a
pretty brittle setup and we should stop relying on that.
I wonder why do we RelationBuildPartitionDesc and
RelationBuildPartitionKey directly in RelationBuildDesc. Wouldn't it be
better to delay constructing those until the first access to them?
Finally: I don't quite understand why we need two memory contexts there,
one for the partkey and another for the partdesc. Surely one context
for both is sufficient. (And while at it, why not have the whole
RelationData inside the same memory context, so that it can be blown
away without so much retail pfree'ing?)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services