Thread: partitioning code reorganization
Andres Freund wrote: > It'd be good to adjust the thread topic - this surely isn't about the > crash anymore. And it's good, especially given we're past the feature > freeze etc, if the subject conveyed what's happening? Sure thing. This thread comes from: https://postgr.es/m/20180413182133.euxapzksj3ejxo2h@alap3.anarazel.de Here's my proposed reorganization patch. This cleans up the partitioning header files, and creates new source files. The split was described by Amit in the previous thread. The new source files are: src/backend/partitioning/partbounds.c (2057 lines) src/backend/utils/cache/partcache.c (972 lines) That code comes mostly from partition.c, but there's also some code coming from relcache.c. Two new header files are created: src/include/partitioning/partdefs.h (this one is used by most other header files needing to refer to partitioning features). src/include/utils/partcache.h This compiles with no warnings, and passes cpluspluscheck. Inclusions of catalog/partition.h all over the place has been reduced to a minimum. This is a good thing: that header was causing bleed of unrelated stuff into a lot of seemingly unrelated places, as you can see in a few files that had to gain some completely unrelated #includes: contrib/pageinspect/hashfuncs.c: +#include "utils/rel.h" contrib/pg_stat_statements/pg_stat_statements.c +#include "utils/acl.h" src/backend/catalog/pg_constraint.c +#include "access/tupconvert.h" src/backend/tcop/utility.c +#include "utils/rel.h" src/backend/utils/misc/pg_controldata.c +#include "access/xlog.h" -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Further thought: compute_hash_value and satisfies_hash_partition both belong in partbounds.c. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Here's a final version. The one thing I don't like about this is having put PartitionRangeDatumKind in partdefs.h, which forces us to #include that file in parsenodes.h. I had to do this in order to avoid #including parsenodes.h in partbounds.h. Now maybe that is not so bad, since that file isn't *that* widely used anyway; it wouldn't cause any unnecessary bleeding of parsenodes.h into any other headers. So maybe I'll put the enum back in parsenodes. Any opinions on that? I intend to push this this evening. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hi. Thanks for taking care of few things I left like those PartitionKey accessors in rel.h. On Sat, Apr 14, 2018 at 8:51 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Here's a final version. > > The one thing I don't like about this is having put > PartitionRangeDatumKind in partdefs.h, which forces us to #include that > file in parsenodes.h. I had to do this in order to avoid #including > parsenodes.h in partbounds.h. Now maybe that is not so bad, since that > file isn't *that* widely used anyway; it wouldn't cause any unnecessary > bleeding of parsenodes.h into any other headers. So maybe I'll put the > enum back in parsenodes. Any opinions on that? I'm fine with keeping it where it was, that is, parsenodes.h. I can see that parsenodes.h is pretty heavily included in other headers anyway. Also, +1 to moving compute_hash_value() and satisfies_hash_partition() to partbounds.c. Thanks, Amit
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> The one thing I don't like about this is having put
> PartitionRangeDatumKind in partdefs.h, which forces us to #include that
> file in parsenodes.h. I had to do this in order to avoid #including
> parsenodes.h in partbounds.h. Now maybe that is not so bad, since that
> file isn't *that* widely used anyway; it wouldn't cause any unnecessary
> bleeding of parsenodes.h into any other headers. So maybe I'll put the
> enum back in parsenodes. Any opinions on that?
I'd vote for the latter. Including an arbitrary pile of partition stuff
in parsenodes.h is not much better than making it all flat-out global.
Including one enum is less bad. There are some comparable cases too,
e.g. that some ACL-related defines are in parsenodes.h, but not everything
to do with ACLs.
(Haven't actually read the patch.)
regards, tom lane
On Sat, Apr 14, 2018 at 11:48 PM, Amit Langote <amitlangote09@gmail.com> wrote: > Hi. > > Thanks for taking care of few things I left like those PartitionKey > accessors in rel.h. Forgot to mention -- there are some files that still include catalog/partition.h but no longer need to. Find a delta patch attached that applies on your v6. Thanks, Amit
Attachment
Amit Langote wrote: > On Sat, Apr 14, 2018 at 11:48 PM, Amit Langote <amitlangote09@gmail.com> wrote: > > Hi. > > > > Thanks for taking care of few things I left like those PartitionKey > > accessors in rel.h. > > Forgot to mention -- there are some files that still include > catalog/partition.h but no longer need to. Find a delta patch > attached that applies on your v6. Thanks! I pushed this now, putting back the enum in parsenodes and including this delta patch of yours. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Apr 15, 2018 at 9:17 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Thanks! I pushed this now, putting back the enum in parsenodes and > including this delta patch of yours. Thank you. Do you think the following CF entry is good to be closed? Regards, Amit
On Sun, Apr 15, 2018 at 11:57 AM, Amit Langote <amitlangote09@gmail.com> wrote: > On Sun, Apr 15, 2018 at 9:17 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> Thanks! I pushed this now, putting back the enum in parsenodes and >> including this delta patch of yours. > > Thank you. Do you think the following CF entry is good to be closed? I meant this one: https://commitfest.postgresql.org/17/1520/ Thanks, Amit
On 2018/04/15 9:17, Alvaro Herrera wrote:
> Amit Langote wrote:
>> On Sat, Apr 14, 2018 at 11:48 PM, Amit Langote <amitlangote09@gmail.com> wrote:
>>> Hi.
>>>
>>> Thanks for taking care of few things I left like those PartitionKey
>>> accessors in rel.h.
>>
>> Forgot to mention -- there are some files that still include
>> catalog/partition.h but no longer need to. Find a delta patch
>> attached that applies on your v6.
>
> Thanks! I pushed this now, putting back the enum in parsenodes and
> including this delta patch of yours.
Not sure if you were intending to discuss the remaining portion of the
changes I proposed last week (patch 0002 posted at [1]), but I'm posting
those patches here for discussion. I've divided the patch further.
0001-Make-copying-of-cached-partitioning-info-more-con.patch
Make copying of cached partitioning info more consistent
Currently there are many callers that hold onto pointers that
point into the partitioning related information cached in relcache.
There are others, such as the planner, who copy important information
before using it.
Make everyone copy!
Now because no part of the backend relies on the guarantee that
pointers to partitioning info in relcache points to same memory even
across relcache flushes, we don't need special guards as implemented
in RelationClearRelation() to provide the aforementioned guarantee.
0002-Cache-all-partitioning-info-under-one-memory-cont.patch
Cache all partitioning info under one memory context
Instead of having one for PartitionKey and another for PartitionDesc,
use just one. Also, instead of allocating partition constraint
expression tree directly under CacheMemoryContext, do it under the
aforementioned context.
0003-Cache-partsupfunc-separately-from-PartitionKey.patch
Cache partsupfunc separately from PartitionKey
Callers who want to use partsupfunc now have to copy them separately
from PartitionKey, which makes copying the latter a bit cheaper.
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:
* RelOptInfo -> Relation *
https://www.postgresql.org/message-id/CA%2BTgmoYKToP4-adCFFRNrO21OGuH%3Dphx-fiB1dYoqksNYX6YHQ%40mail.gmail.com
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/CA%2BHiwqFo_NbJfS%2BY%3DtE94Tn5EVHXN02JkmGjwV4xT6fU3oc5OQ%40mail.gmail.com
Attachment
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. > 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. 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). 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. 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. This is all pg12 anyway. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
On 2018/04/15 11:58, Amit Langote wrote: > On Sun, Apr 15, 2018 at 11:57 AM, Amit Langote <amitlangote09@gmail.com> wrote: >> On Sun, Apr 15, 2018 at 9:17 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >>> Thanks! I pushed this now, putting back the enum in parsenodes and >>> including this delta patch of yours. >> >> Thank you. Do you think the following CF entry is good to be closed? > > I meant this one: https://commitfest.postgresql.org/17/1520/ I closed this entry as committed (da6f3e45ddb [1]). Thanks, Amit [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=da6f3e45ddb