From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Amit Langote <amitlangote09(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: partitioning code reorganization |
Date: | 2018-04-19 09:58:34 |
Message-ID: | e9f294b9-6b32-a2c5-344a-10fcd439aac4@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro HORIGUCHI | 2018-04-19 10:03:02 | Re: Oddity in tuple routing for foreign partitions |
Previous Message | John Naylor | 2018-04-19 09:50:39 | Re: Documentation for bootstrap data conversion |