Re: crash with sql language partition support function - Mailing list pgsql-hackers

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