Re: partitioning code reorganization

On 2018/04/18 5:18, Alvaro Herrera wrote:
> Amit Langote wrote:
> 
>> 0001-Make-copying-of-cached-partitioning-info-more-con.patch
>> 0002-Cache-all-partitioning-info-under-one-memory-cont.patch
>> 0003-Cache-partsupfunc-separately-from-PartitionKey.patch
> 
> I'd rather not do these patches now, unless there is some pressing
> reason to (eg. some bug crops up).  I know Tom dislikes some of the code
> being used for relcache, but I don't know of any actual bugs; my
> proposal is put these patches to sleep until pg12 opens.

Sure, that might be a good idea.

>> I think going the way of 0001 might seem counter to what we may *really*
>> want to do in this regard, which is to make it so that we can use (keep
>> around) the pointer to partition info in relcache, instead of copying the
>> information in it piece by piece for every query.  Robert's email from a
>> couple of months ago (that he also recently mentioned) brought this up wrt
>> to relcache data usage within the planner:
> 
> I hadn't seen that discussion, thanks.
> 
> Some braindump follows.
> 
> Re 0002 though I didn't carefully review the patch, I worry that an
> error partway some of these routines would leave you with either a
> long-lived memory leak (or permanent!), or dangling pointers in the
> relcache entry.

The latter seems unlikely because we don't attach anything to relcache
until we're done building the whole thing.

> If that is so, you can probably fix it by having a
> memcxt callback that resets the other partition-related members from the
> relcache entry being processed, so that they are rebuilt; then reset the
> memcxt. (make the memcxt always exist, for simplicity).

Thanks, I'll go look at if and what memory context callbacks can do to
help here.

> The other thing I was interested in was to build the partitioning info
> on demand rather than immediately when the relcache entry is created.
> I think there must be tons of places where we want the relcache entry
> for other reasons and we don't want to waste time building partition
> desc/key uselessly.

I agree.  I had tried to do that back when I wrote the current code, but
don't remember why I gave up.  I quickly prototyped it today and didn't
run into any problems along the way.

> Another line of thought.  Instead of copying anything, did you consider
> the idea of using refcounted structs?  Rough sketch: in
> RelationGetPartitionDesc, build struct if relcache doesn't have one,
> otherwise just increment pincount in existing struct; create new
> ReleasePartitionDesc which decrements pincount, and if pincount is 0 and
> not referenced from relcache entry, free the struct (memcxt delete?); if
> relcache inval is received, detach partitiondesc from relcache entry and
> build a new one.  So old pointers continue to work, until the release.
> Maybe need to get ResourceOwner involved to make sure we don't
> accidentally lose track of a PartitionDesc with pincout != 0.

Will look into that.  Not having to copy all the time seems like it'd be nice.

> This is all pg12 anyway.

I will start a new thread sometime later.

Thanks,
Amit