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