Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> Ashutosh Bapat wrote:
>> On Tue, Apr 10, 2018 at 1:44 PM, Amit Langote
>> <langote_amit...@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.
No, it's correct as written, because rd_partkeycxt hasn't been set yet.
> 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.
Yeah, all of the callers of RelationGetPartitionKey seem to assume that
the pointer they get is guaranteed valid and will stay so forever.
This is pretty dangerous, independently of the bug Amit mentions.
However, I'm not sure that copy-on-read is a good solution here, because
of exactly the point at stake that the FmgrInfos may have infrastructure.
We don't have a way to copy that, and even if we did, copying on every
reference would be really expensive.
We might try to make this work like the relcache infrastructure for
indexes, which also contains FmgrInfos. However, in the index case
we may safely assume that that stuff never changes for the life of the
index. I'm afraid that's not true for the partitioning data is it?
How much does it actually buy us to store FmgrInfos here rather than,
say, function OIDs? If we backed off to that, then the data structure
might be simple enough that copy-on-read would work.
Otherwise we might need some kind of refcount mechanism. We built one
for domain constraints in the typcache, and it's not horrid, but it is a
fair amount of code.
> 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.
It'd only matter if there were a reason to delete/rebuild one but not the
other within a particular relcache entry, which probably there isn't.
So using one context for both would likely be a bit simpler and more
efficient.
BTW, another thing in the same general area that I was planning to get
on the warpath about sooner or later is that the code managing
rd_partcheck is really cruddy. It spews a lot of random data structure
into the CacheMemoryContext with no way to release it at relcache inval,
resulting in a session-lifespan memory leak. (pfree'ing just the List
header, as RelationDestroyRelation does, is laughably inadequate.)
Perhaps that could be fixed by likewise storing it in a sub-context
used for partition information.
regards, tom lane
