Horton

Thread

Commits

  1. Reorganize partitioning code

  1. partitioning code reorganization

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-13T19:35:03Z

    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

  2. Re: partitioning code reorganization

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-13T20:48:06Z

    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

  3. Re: partitioning code reorganization

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-14T11:51:40Z

    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

  4. Re: partitioning code reorganization

    Amit Langote <amitlangote09@gmail.com> — 2018-04-14T14:48:05Z

    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

  5. Re: partitioning code reorganization

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-14T15:30:08Z

    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

  6. Re: partitioning code reorganization

    Amit Langote <amitlangote09@gmail.com> — 2018-04-14T15:55:50Z

    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

  7. Re: partitioning code reorganization

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-15T00:17:06Z

    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

  8. Re: partitioning code reorganization

    Amit Langote <amitlangote09@gmail.com> — 2018-04-15T02:57:41Z

    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

  9. Re: partitioning code reorganization

    Amit Langote <amitlangote09@gmail.com> — 2018-04-15T02:58:03Z

    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

  10. Re: partitioning code reorganization

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-04-16T07:46:34Z

    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

  11. Re: partitioning code reorganization

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-17T20:18:50Z

    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

  12. Re: partitioning code reorganization

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-04-19T09:58:34Z

    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

  13. Re: partitioning code reorganization

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-04-24T01:12:42Z

    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