partitioning code reorganization

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

partitioning code reorganization

Álvaro Herrera
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@...

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

v5-0001-Reorganize-partitioning-code-structure.patch (213K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Álvaro Herrera
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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Álvaro Herrera
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

v6-0001-Reorganize-partitioning-code-structure.patch (227K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Amit Langote
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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Tom Lane-2
In reply to this post by Álvaro Herrera
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Amit Langote
In reply to this post by Amit Langote
On Sat, Apr 14, 2018 at 11:48 PM, Amit Langote <[hidden email]> 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

v6_delta-adjust-partition-h-include.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Álvaro Herrera
Amit Langote wrote:
> On Sat, Apr 14, 2018 at 11:48 PM, Amit Langote <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Amit Langote
On Sun, Apr 15, 2018 at 9:17 AM, Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Amit Langote
On Sun, Apr 15, 2018 at 11:57 AM, Amit Langote <[hidden email]> wrote:
> On Sun, Apr 15, 2018 at 9:17 AM, Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Amit Langote-2
In reply to this post by Álvaro Herrera
On 2018/04/15 9:17, Alvaro Herrera wrote:

> Amit Langote wrote:
>> On Sat, Apr 14, 2018 at 11:48 PM, Amit Langote <[hidden email]> 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

v1-0001-Make-copying-of-cached-partitioning-info-more-con.patch (73K) Download Attachment
v1-0002-Cache-all-partitioning-info-under-one-memory-cont.patch (9K) Download Attachment
v1-0003-Cache-partsupfunc-separately-from-PartitionKey.patch (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Álvaro Herrera
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

Reply | Threaded
Open this post in threaded view
|

Re: partitioning code reorganization

Amit Langote-2
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


Previous Thread Next Thread