Thread: crash with sql language partition support function

Hi.

I noticed that RelationBuildPartitionKey() is not doing the right thing
with respect to which context it passes to fmgr.c to set a the (cached)
partition support function's FmgrInfo's fn_mcxt.

I noticed a crash while trying to change partition pruning tests to use
manually created hash operator class per [1].

CREATE OR REPLACE FUNCTION hashint4_noop(int4, int8) RETURNS int8 AS
$$SELECT coalesce($1)::int8$$ LANGUAGE sql IMMUTABLE STRICT;

CREATE OPERATOR CLASS test_int4_ops FOR TYPE int4 USING HASH AS
OPERATOR 1 = , FUNCTION 2 hashint4_noop(int4, int8);

CREATE OR REPLACE FUNCTION hashtext_length(text, int8) RETURNS int8 AS
$$SELECT length(coalesce($1))::int8$$ LANGUAGE sql IMMUTABLE STRICT;

CREATE OPERATOR CLASS test_text_ops FOR TYPE text USING HASH AS
OPERATOR 1 = , FUNCTION 2 hashtext_length(text, int8);

create table hp (a int, b text) partition by hash (a test_int4_ops, b
test_text_ops);
create table hp0 partition of hp for values with (modulus 4, remainder 0);
create table hp3 partition of hp for values with (modulus 4, remainder 3);
create table hp1 partition of hp for values with (modulus 4, remainder 1);
create table hp2 partition of hp for values with (modulus 4, remainder 2);

insert into hp values (1, 'abcde');
INSERT 0 1
Time: 13.604 ms
postgres=# insert into hp values (null, 'abcde');
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Attached fixes it.  It teaches RelationBuildPartitionKey() to use
fmgr_info_cxt and pass rd_partkeycxt to it.

Since this bug also exists in the released PG 10 branch, I also created a
patch for that.  It's slightly different than the one for PG 11dev,
because there were some changes recently to how the memory context is
manipulated in RelationBuildPartitionKey.  That's
v1-PG10-0001-Fix-a-memory-context-bug-in-RelationBuildPartitio.patch.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoZ0D5kJbt8eKXtvVdvTcGGWn6ehWCRSZbWytD-uzH92mQ%40mail.gmail.com

I have added this in the Older Bugs section of open items page.

https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Older_Bugs

Thanks,
Amit

On Tue, Apr 10, 2018 at 1:44 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> Attached fixes it.  It teaches RelationBuildPartitionKey() to use
> fmgr_info_cxt and pass rd_partkeycxt to it.

The patch is using partkeycxt and not rd_partkeycxt. Probably a typo
in the mail. But a wider question, why that context? I guess that
cache context will vanish when that cache entry is thrown away. That's
the reason we have to copy partition key information in
find_partition_scheme() instead of just pointing to it and also use
fmgr_info_copy() there. But if that's the case, buildfarm animal run
with -DCLOBBER_CACHE_ALWAYS should show failure. I am missing
something here.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Ashutosh Bapat wrote:
> On Tue, Apr 10, 2018 at 1:44 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >
> > Attached fixes it.  It teaches RelationBuildPartitionKey() to use
> > fmgr_info_cxt and pass rd_partkeycxt to it.
> 
> The patch is using partkeycxt and not rd_partkeycxt. Probably a typo
> in the mail. But a wider question, why that context? I guess that
> cache context will vanish when that cache entry is thrown away. That's
> the reason we have to copy partition key information in
> find_partition_scheme() instead of just pointing to it and also use
> fmgr_info_copy() there. But if that's the case, buildfarm animal run
> with -DCLOBBER_CACHE_ALWAYS should show failure. I am missing
> something here.

Good point.  Yeah, it looks like it should cause problems.  I think it
would be better to have RelationGetPartitionKey() return a copy in the
caller's context of the data of the relcache data, rather than the
relcache data itself, no?  Seems like this would not fail if there never
is a CCI between the RelationGetPartitionKey call and the last access of
the returned key struct ... but if this is the explanation, then it's a
pretty brittle setup and we should stop relying on that.

I wonder why do we RelationBuildPartitionDesc and
RelationBuildPartitionKey directly in RelationBuildDesc.  Wouldn't it be
better to delay constructing those until the first access to them?

Finally: I don't quite understand why we need two memory contexts there,
one for the partkey and another for the partdesc.  Surely one context
for both is sufficient.  (And while at it, why not have the whole
RelationData inside the same memory context, so that it can be blown
away without so much retail pfree'ing?)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Ashutosh Bapat wrote:
>> On Tue, Apr 10, 2018 at 1:44 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> Attached fixes it.  It teaches RelationBuildPartitionKey() to use
>>> fmgr_info_cxt and pass rd_partkeycxt to it.

>> The patch is using partkeycxt and not rd_partkeycxt. Probably a typo
>> in the mail.

No, it's correct as written, because rd_partkeycxt hasn't been set yet.

> Good point.  Yeah, it looks like it should cause problems.  I think it
> would be better to have RelationGetPartitionKey() return a copy in the
> caller's context of the data of the relcache data, rather than the
> relcache data itself, no?  Seems like this would not fail if there never
> is a CCI between the RelationGetPartitionKey call and the last access of
> the returned key struct ... but if this is the explanation, then it's a
> pretty brittle setup and we should stop relying on that.

Yeah, all of the callers of RelationGetPartitionKey seem to assume that
the pointer they get is guaranteed valid and will stay so forever.
This is pretty dangerous, independently of the bug Amit mentions.

However, I'm not sure that copy-on-read is a good solution here, because
of exactly the point at stake that the FmgrInfos may have infrastructure.
We don't have a way to copy that, and even if we did, copying on every
reference would be really expensive.

We might try to make this work like the relcache infrastructure for
indexes, which also contains FmgrInfos.  However, in the index case
we may safely assume that that stuff never changes for the life of the
index.  I'm afraid that's not true for the partitioning data is it?

How much does it actually buy us to store FmgrInfos here rather than,
say, function OIDs?  If we backed off to that, then the data structure
might be simple enough that copy-on-read would work.

Otherwise we might need some kind of refcount mechanism.  We built one
for domain constraints in the typcache, and it's not horrid, but it is a
fair amount of code.

> Finally: I don't quite understand why we need two memory contexts there,
> one for the partkey and another for the partdesc.  Surely one context
> for both is sufficient.

It'd only matter if there were a reason to delete/rebuild one but not the
other within a particular relcache entry, which probably there isn't.
So using one context for both would likely be a bit simpler and more
efficient.

BTW, another thing in the same general area that I was planning to get
on the warpath about sooner or later is that the code managing
rd_partcheck is really cruddy.  It spews a lot of random data structure
into the CacheMemoryContext with no way to release it at relcache inval,
resulting in a session-lifespan memory leak.  (pfree'ing just the List
header, as RelationDestroyRelation does, is laughably inadequate.)
Perhaps that could be fixed by likewise storing it in a sub-context
used for partition information.

            regards, tom lane

Thanks for the comments.

On 2018/04/11 0:27, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> Ashutosh Bapat wrote:
>>> On Tue, Apr 10, 2018 at 1:44 PM, Amit Langote
>>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>> Attached fixes it.  It teaches RelationBuildPartitionKey() to use
>>>> fmgr_info_cxt and pass rd_partkeycxt to it.
> 
>>> The patch is using partkeycxt and not rd_partkeycxt. Probably a typo
>>> in the mail.
> 
> No, it's correct as written, because rd_partkeycxt hasn't been set yet.

Yeah.  partkeycxt will be assigned to rd_partkecxt later in the function,
but I guess Ashutosh seems to already be aware of that, given this discussion.

>> Good point.  Yeah, it looks like it should cause problems.  I think it
>> would be better to have RelationGetPartitionKey() return a copy in the
>> caller's context of the data of the relcache data, rather than the
>> relcache data itself, no?  Seems like this would not fail if there never
>> is a CCI between the RelationGetPartitionKey call and the last access of
>> the returned key struct ... but if this is the explanation, then it's a
>> pretty brittle setup and we should stop relying on that.
> 
> Yeah, all of the callers of RelationGetPartitionKey seem to assume that
> the pointer they get is guaranteed valid and will stay so forever.
> This is pretty dangerous, independently of the bug Amit mentions.
> 
> However, I'm not sure that copy-on-read is a good solution here, because
> of exactly the point at stake that the FmgrInfos may have infrastructure.
> We don't have a way to copy that, and even if we did, copying on every
> reference would be really expensive.
> 
> We might try to make this work like the relcache infrastructure for
> indexes, which also contains FmgrInfos.  However, in the index case
> we may safely assume that that stuff never changes for the life of the
> index.  I'm afraid that's not true for the partitioning data is it?
> 
> How much does it actually buy us to store FmgrInfos here rather than,
> say, function OIDs?  If we backed off to that, then the data structure
> might be simple enough that copy-on-read would work.

I'm thinking of rearranging this along the lines of rd_supportinfo
handling for indexes, which index AMs use index_procinfo to access.

> Otherwise we might need some kind of refcount mechanism.  We built one
> for domain constraints in the typcache, and it's not horrid, but it is a
> fair amount of code.
> 
>> Finally: I don't quite understand why we need two memory contexts there,
>> one for the partkey and another for the partdesc.  Surely one context
>> for both is sufficient.
> 
> It'd only matter if there were a reason to delete/rebuild one but not the
> other within a particular relcache entry, which probably there isn't.
> So using one context for both would likely be a bit simpler and more
> efficient.

While the partdesc may change due to additiona/removal of partitions,
partkey never changes.  So, while we always "keep" partkey, we may
delete/rebuild partdesc if it has changed.

> BTW, another thing in the same general area that I was planning to get
> on the warpath about sooner or later is that the code managing
> rd_partcheck is really cruddy.  It spews a lot of random data structure
> into the CacheMemoryContext with no way to release it at relcache inval,
> resulting in a session-lifespan memory leak.  (pfree'ing just the List
> header, as RelationDestroyRelation does, is laughably inadequate.)

Indeed it is.  It should've been list_free_deep() I suppose.

> Perhaps that could be fixed by likewise storing it in a sub-context
> used for partition information.

We can do that, but note that the rd_partcheck is set for "partitions",
whereas other things we're talking about are set for partitioned table
parents.  But I guess it doesn't matter much.


Anyway, after reading your replies, I thought of taking a stab at unifying
the partitioning information that's cached by relcache.c.

First thing toward that I did was to unify rd_partkeycxt and rd_pdcxt into
one rd_partcxt.  Instead of having RelationBuildPartitionKey and
RelationBuildPartitionDesc that build rd_partkey and rd_partdesc, resp.,
now there is just RelationBuildPartitionInfo that builds both PartitionKey
and PartitionDesc, all under rd_partcxt.  That means I moved a bunch of
code from catalog/partition.c to cache/relcache.c.

Second, I made all the places that want to hang on to information from
either the PartitionKey or PartitionDesc to make its copy first.  Most
such places already do most of the copying, I just modified the way it's
done.  Before doing that, I replaced the FmgrInfo array in PartitionKey
with one containing OIDs.  FmgrInfo array is now cached outside in
rd_partsupfuncinfo, which is accessed using partition_getprocinfo() (which
functions like index_getprocinfo) by various callers.

Then, because we now have a policy of users making a copy of partitioning
information before it can be hanged on to, the keep_* logic in
RelationClearRelation() to preserve where rd_partkey and rd_partdesc point
to is no longer needed, so removed it.

Because different callers may need only certain information from the
relcache (or really a copy of it), there are different
RelationGetPartition* functions that return a copy of the requested
information.  For example, if a certain caller wants just the number of
partitions, then use RelationGetPartitionCount which returns
rd_partdesc->nparts.  If want OIDs, use RelationGetPartitionOids.  Callers
like the planner, executor that want to copy *all* information use
RelationGetPartitionInfo which returns a PartitionInfo that contains
fields to contain copy of rd_partkey, rd_partdesc->oids, and
rd_partdesc->boundinfo.

Then finally, the expression tree contained in rd_partcheck now uses
memory allocated in the same rd_partcxt.

Attached find the patch.  Since this started as a bugfix that will need to
be back-patched to PG10, I wonder if I need to write a separate patch to
adopt this (kind of big) cleanup/reshuffling for PG 10 code.

Thanks,
Amit

Amit Langote wrote:

> Anyway, after reading your replies, I thought of taking a stab at unifying
> the partitioning information that's cached by relcache.c.

Wow.  Now that's one large patch.  I'm going to run with this for HEAD,
but I think we should do a minimal fix for PG10.  Did you detect any
further bugs, while doing all this rework, apart from the one that
started this thread?  If not, I would prefer to do commit the minimal
fix at start of thread to both branches, then apply the larger
restructuring patch to HEAD only.

For the record, I don't like the amount of code that this is putting in
relcache.c.  I am thinking that most of that code will go to
src/backend/partitioning/partbounds.c instead.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Amit Langote wrote:

> Since this bug also exists in the released PG 10 branch, I also created a
> patch for that.  It's slightly different than the one for PG 11dev,
> because there were some changes recently to how the memory context is
> manipulated in RelationBuildPartitionKey.  That's
> v1-PG10-0001-Fix-a-memory-context-bug-in-RelationBuildPartitio.patch.

Here's a repro for pg10, which doesn't have hash partitioning:

create function my_int4_sort(int4,int4) returns int language sql
  as $$ select case when $1 = $2 then 0 when $1 > $2 then 1 else -1 end; $$;
create operator class test_int4_ops for type int4 using btree as
  operator 1 < (int4,int4), operator 2 <= (int4,int4),
  operator 3 = (int4,int4), operator 4 >= (int4,int4),
  operator 5 > (int4,int4), function 1 my_int4_sort(int4,int4);
create table t (a int4) partition by range (a test_int4_ops);
create table t1 partition of t for values from (0) to (1000);
insert into t values (100);
insert into t values (200); -- *boom*

I'm dealing with this now -- will push shortly.  The sane thing to do is
backpatch my previous memcxt fixes, since your patch introduces a
problem that we discussed with that other patch, namely that you would
leak the whole memory context if there is a problem while running the
function.  I also noticed here that copy_partition_key is doing memcpy()
on the fmgr_info, which is bogus -- it should be using fmgr_info_copy.
Rather than patching that one piece it seems better to replace it
wholesale, since I bet this won't be the last time we'll hear about this
routine, and I would prefer not to deal with differently broken code in
the older branch.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Apr 12, 2018 at 8:55 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Amit Langote wrote:
>> Anyway, after reading your replies, I thought of taking a stab at unifying
>> the partitioning information that's cached by relcache.c.
>
> Wow.  Now that's one large patch.  I'm going to run with this for HEAD,
> but I think we should do a minimal fix for PG10.

Is there really a compelling reason to do more than minimal fixes in
HEAD?  We are (or should be) trying to stabilize this branch so we can
ship it and start the next one, and the chances that heavy hacking on
this will delay that process seem better than average.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Alvaro Herrera wrote:

> I'm dealing with this now -- will push shortly.  The sane thing to do is
> backpatch my previous memcxt fixes, since your patch introduces a
> problem that we discussed with that other patch, namely that you would
> leak the whole memory context if there is a problem while running the
> function.  I also noticed here that copy_partition_key is doing memcpy()
> on the fmgr_info, which is bogus -- it should be using fmgr_info_copy.
> Rather than patching that one piece it seems better to replace it
> wholesale, since I bet this won't be the last time we'll hear about this
> routine, and I would prefer not to deal with differently broken code in
> the older branch.

Pushed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Robert Haas wrote:
> On Thu, Apr 12, 2018 at 8:55 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Amit Langote wrote:
> >> Anyway, after reading your replies, I thought of taking a stab at unifying
> >> the partitioning information that's cached by relcache.c.
> >
> > Wow.  Now that's one large patch.  I'm going to run with this for HEAD,
> > but I think we should do a minimal fix for PG10.
> 
> Is there really a compelling reason to do more than minimal fixes in
> HEAD?

IMO there is ample reason for restructuring the lot of code that currently
lives in catalog/partition.c.  We're going to have to support this code
for a minimum of six years -- and that's only if we get around to
reorganizing it during pg12.  I don't have a lot of faith that I'll be
motivated to reorganize it in pg12, but I do know that I am motivated to
reorganize it now.  If we do happen to reorganize it in pg12, then any
bugs we find afterwards will cost double in terms of backpatching the
fixes than if we reorganize it now.  I don't want my future self to
curse my current self for not doing it when it was appropriate -- i.e.
when it was fresh in my mind and freshly written.

> We are (or should be) trying to stabilize this branch so we can
> ship it and start the next one,

Yes, that is what I am trying to do -- I want us to have a sane base
upon which to do our work for years to come.

Do we need more than minimal fixes in the memory context, FmgrInfo, and
miscellaneous other fixes that Amit is proposing in this patch?  That I
don't know.  I hope to have an answer for this later, and I think this
is the reason for your final comment:

> and the chances that heavy hacking on this will delay that process
> seem better than average.

Maybe if there are no bugs and it's just ugly coding, it is best left
alone.  But as I said, I don't know the answer yet.

I will make sure to propose any functional code changes separately from
code movement.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I wonder what prompted people to #include "catalog/partition.h" in
executor.h.

Amit Langote wrote:

> Anyway, after reading your replies, I thought of taking a stab at unifying
> the partitioning information that's cached by relcache.c.

After going over your patch, I think you went slightly overboard here.
Or maybe not, but this patch is so large that it's hard to form an
opinion about it.  Some of these cleanups we should probably adopt per
discussion upthread, but I think we're at a point where we have to work
in smaller steps to avoid destabilizing the code.

I'm not sure about the new PartitionInfo that you propose.  I see you
had to add partitioning/partbounds.h to rel.h -- not a fan of that.
I was thinking of a simpler fix there, just remove one of the two
memcxts in RelationData and put both structs in the remaining one.
Maybe I'm not ambitious enough.

Here's what I would propose: create partitioning/partbounds.c to deal
with partition bounds (i.e. mostly PartitionBoundInfoData and
PartitionBoundSpec), and have utils/cache/partcache.c (with
corresponding utils/partcache.h) for RelationGetPartitionDesc and
RelationGetPartitionKey (not much else, it seems).

Maybe also move get_partition_for_tuple to execPartition.c?

Preliminarly, I would say that the following functions would be in
partbounds.c (in roughly this order):

Exported:

get_qual_from_partbound
partition_bounds_equal
partition_bounds_copy
check_new_partition_bound
check_default_allows_bound
get_hash_partition_greatest_modulus
make_one_range_bound

qsort_partition_list_value_cmp
partition_rbound_cmp
partition_rbound_datum_cmp
qsort_partition_hbound_cmp
partition_hbound_cmp

partition_list_bsearch
partition_range_bsearch
partition_range_datum_bsearch
partition_hash_bsearch

static:
get_partition_bound_num_indexes
make_partition_op_expr
get_partition_operator
get_qual_for_hash
get_qual_for_list
get_qual_for_range
get_range_key_properties
get_range_nulltest

Unsure yet about compute_hash_value and satisfies_hash_partition.

The rest would remain in catalog/partition.c, which should hopefully not
be a lot.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018/04/13 6:58, Alvaro Herrera wrote:
> I wonder what prompted people to #include "catalog/partition.h" in
> executor.h.

I regret having done it.  Removing it from there is no one-line patch. :-(

> Amit Langote wrote:
> 
>> Anyway, after reading your replies, I thought of taking a stab at unifying
>> the partitioning information that's cached by relcache.c.
> 
> After going over your patch, I think you went slightly overboard here.
> Or maybe not, but this patch is so large that it's hard to form an
> opinion about it.

It's mostly code movement, but there are some other changes as well as
described below.

> Some of these cleanups we should probably adopt per
> discussion upthread, but I think we're at a point where we have to work
> in smaller steps to avoid destabilizing the code.

OK, I agree.  There are a few main things this patch does:

1. Code reorganization (across partition.c, relcache.c, partbounds.c, and
   partcache.c)

2. Allocate all partitioning info in relcache, which include PartitionKey,
   PartitionDesc, and partition constraint expression tree, in the same
   context

3. Make copying of partitioning info in relcache by various callers a bit
   more consistent.  After doing that, the guards in RelationClearRelation
   to preserve unchanged partition info are no longer necessary.

4. Cache FmgrInfo's outside PartitionKey, leaving just OIDs there, and add
   a separate function to copy it to some caller-specified memory (and
   context).

> I'm not sure about the new PartitionInfo that you propose.

I too am no longer sure if there is any point in adding an extra
indirection if callers could instead directly ask for members like
partition key, nparts, oids, and boundinfo.  So, I've gotten rid of it and
RelationGetPartitionInfo.

> I see you
> had to add partitioning/partbounds.h to rel.h -- not a fan of that.
> I was thinking of a simpler fix there, just remove one of the two
> memcxts in RelationData and put both structs in the remaining one.
> Maybe I'm not ambitious enough.

Now, there's just rd_partcxt.  Both RelationBuildPartitionKey and
RelationBuildPartitionDesc and partition constraint expression tree are
allocated in that context.

> Here's what I would propose: create partitioning/partbounds.c to deal
> with partition bounds (i.e. mostly PartitionBoundInfoData and
> PartitionBoundSpec),

OK, I've done that.  It exports:

> get_qual_from_partbound
> partition_bounds_equal
> partition_bounds_copy
> check_new_partition_bound
> check_default_allows_bound
> get_hash_partition_greatest_modulus
> make_one_range_bound
> partition_rbound_cmp
> partition_rbound_datum_cmp
> qsort_partition_hbound_cmp
> partition_hbound_cmp

I have kept qsort_partition_* functions in partcache.c, because they're
only locally used.

and the following are static:

> get_partition_bound_num_indexes
> make_partition_op_expr
> get_partition_operator
> get_qual_for_hash
> get_qual_for_list
> get_qual_for_range
> get_range_key_properties
> get_range_nulltest

This means a bunch of code was migrated from catalog/partition.c to
partitioning/partbounds.c.

> and have utils/cache/partcache.c (with
> corresponding utils/partcache.h) for RelationGetPartitionDesc and
> RelationGetPartitionKey (not much else, it seems).

OK, there is now a utils/cache/partcache.c as well.

Beside RelationGetPartitionKey and RelationGetPartitionDesc you mentioned,
it also has RelationgGetPartitionQual() and get_partition_qual_relid(),
because they touch the relcache (rd_partcheck) data.  Also, there are
following locals:

qsort_partition_hbound_cmp
qsort_partition_list_value_cmp
qsort_partition_rbound_cmp
partition_key_copy
generate_partition_qual

Other than the above mentioned exported functions, I've introduced the
following functions which return copy of the information in relcache.

PartitionKey RelationGetPartitionKey(Relation relation);
FmgrInfo *partition_getprocinfo(Relation rel, PartitionKey key,
                                int partattoff);
int RelationGetPartitionCount(Relation relation);
Oid *RelationGetPartitionOids(Relation relation);
PartitionBoundInfo RelationGetPartitionBounds(Relation relation);
Oid  RelationGetDefaultPartitionOid(Relation rel);

> Maybe also move get_partition_for_tuple to execPartition.c?

Have done that already.

> Unsure yet about compute_hash_value and satisfies_hash_partition.

I've left them in catalog/partition.c for now.

> The rest would remain in catalog/partition.c, which should hopefully not
> be a lot.

Not much after the latest version of the patch.

$ wc -l src/backend/catalog/partition.c
642 src/backend/catalog/partition.c

In master:

$ wc -l src/backend/catalog/partition.c
3497 src/backend/catalog/partition.c

Thanks,
Amit

On 2018/04/13 3:10, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> 
>> I'm dealing with this now -- will push shortly.  The sane thing to do is
>> backpatch my previous memcxt fixes, since your patch introduces a
>> problem that we discussed with that other patch, namely that you would
>> leak the whole memory context if there is a problem while running the
>> function.  I also noticed here that copy_partition_key is doing memcpy()
>> on the fmgr_info, which is bogus -- it should be using fmgr_info_copy.
>> Rather than patching that one piece it seems better to replace it
>> wholesale, since I bet this won't be the last time we'll hear about this
>> routine, and I would prefer not to deal with differently broken code in
>> the older branch.
> 
> Pushed.

Thanks for fixing that.

Regards,
Amit

Amit Langote wrote:
> On 2018/04/13 6:58, Alvaro Herrera wrote:

> > After going over your patch, I think you went slightly overboard here.
> > Or maybe not, but this patch is so large that it's hard to form an
> > opinion about it.
> 
> It's mostly code movement, but there are some other changes as well as
> described below.

Hmm can you please split out the code changes into a separate patch?   I
can commit the code movement quickly, but the other stuff requres more
creful review.



-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Sorry Alvaro for spamming you with multiple replies for the same
message.  I'm failing to use Gmail properly.

On Fri, Apr 13, 2018 at 7:20 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Amit Langote wrote:
>> On 2018/04/13 6:58, Alvaro Herrera wrote:
>
>> > After going over your patch, I think you went slightly overboard here.
>> > Or maybe not, but this patch is so large that it's hard to form an
>> > opinion about it.
>>
>> It's mostly code movement, but there are some other changes as well as
>> described below.
>
> Hmm can you please split out the code changes into a separate patch?   I
> can commit the code movement quickly, but the other stuff requres more
> creful review.

OK, here are two patches.

First one reorganizes and second one implements the other things I
mentioned.  If you think the 2nd one needs to be split further, do let
me know, although I may not be able to respond until Monday.

Thanks,
Amit

I think this is a good improvement.  On top of that, I propose a new
file partitioning/partdefs.h with the following approximate contents.
This reduces cross-inclusion of headers to the minimum.  I'm dealing
with the fallout from this now, will post a complete patch shortly.


/*-------------------------------------------------------------------------
 *
 * partdefs.h
 *        Base definitions for partitioned table handling
 *
 * Copyright (c) 2007-2018, PostgreSQL Global Development Group
 *
 * src/include/partitioning/partdefs.h
 *
 *-------------------------------------------------------------------------
 */
#ifndef PARTDEFS_H
#define PARTDEFS_H

typedef enum PartitionRangeDatumKind
{
    PARTITION_RANGE_DATUM_MINVALUE = -1,    /* less than any other value */
    PARTITION_RANGE_DATUM_VALUE = 0,    /* a specific (bounded) value */
    PARTITION_RANGE_DATUM_MAXVALUE = 1    /* greater than any other value */
} PartitionRangeDatumKind;

typedef struct PartitionBoundInfoData *PartitionBoundInfo;

typedef struct PartitionKeyData *PartitionKey;

typedef struct PartitionBoundSpec PartitionBoundSpec;

typedef struct PartitionDescData *PartitionDesc;

#endif                            /* PARTDEFS_H */


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2018-04-13 15:08:30 -0300, Alvaro Herrera wrote:
> I think this is a good improvement.  On top of that, I propose a new
> file partitioning/partdefs.h with the following approximate contents.
> This reduces cross-inclusion of headers to the minimum.  I'm dealing
> with the fallout from this now, will post a complete patch shortly.

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?

- Andres