Re: Adding support for Default partition in partitioning

On Mon, Aug 21, 2017 at 4:47 PM, Jeevan Ladhe
<jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
>
> Hi,
>
> On Thu, Aug 17, 2017 at 3:29 PM, Jeevan Ladhe
> <jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
>>
>> Hi,
>>
>> On Tue, Aug 15, 2017 at 7:20 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
>> wrote:
>>>
>>> On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
>>> <jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
>>> > I have rebased the patches on the latest commit.
>>>
>>> This needs another rebase.
>>
>>
>> I have rebased the patch and addressed your and Ashutosh comments on last
>> set of patches.

Thanks for the rebased patches.

>>
>> The current set of patches contains 6 patches as below:
>>
>> 0001:
>> Refactoring existing ATExecAttachPartition code so that it can be used
>> for
>> default partitioning as well

* Returns an expression tree describing the passed-in relation's partition
- * constraint.
+ * constraint. If there are no partition constraints returns NULL e.g. in case
+ * default partition is the only partition.
The first sentence uses singular constraint. The second uses plural. Given that
partition bounds together form a single constraint we should use singular
constraint in the second sentence as well.

Do we want to add a similar comment in the prologue of
generate_partition_qual(). The current wording there seems to cover this case,
but do we want to explicitly mention this case?

+ if (!and_args)
+ result = NULL;
While this is correct, I am increasingly seeing (and_args != NIL) usage.

get_partition_qual_relid() is called from pg_get_partition_constraintdef(),
constr_expr = get_partition_qual_relid(relationId);

/* Quick exit if not a partition */
if (constr_expr == NULL)
PG_RETURN_NULL();
The comment is now wrong since a default partition may have no constraints. May
be rewrite it as simply, "Quick exit if no partition constraint."

generate_partition_qual() has three callers and all of them are capable of
handling NIL partition constraint for default partition. May be it's better to
mention in the commit message that we have checked that the callers of
this function
can handle NIL partition constraint.

>>
>> 0002:
>> This patch teaches the partitioning code to handle the NIL returned by
>> get_qual_for_list().
>> This is needed because a default partition will not have any constraints
>> in case
>> it is the only partition of its parent.

If the partition being dropped is the default partition,
heap_drop_with_catalog() locks default partition twice, once as the default
partition and the second time as the partition being dropped. So, it will be
counted as locked twice. There doesn't seem to be any harm in this, since the
lock will be help till the transaction ends, by when all the locks will be
released.

Same is the case with cache invalidation message. If we are dropping default
partition, the cache invalidation message on "default partition" is not
required. Again this might be harmless, but better to avoid it.

Similar problems exists with ATExecDetachPartition(), default partition will be
locked twice if it's being detached.

+ /*
+ * If this is a default partition, pg_partitioned_table must have it's
+ * OID as value of 'partdefid' for it's parent (i.e. rel) entry.
+ */
+ if (castNode(PartitionBoundSpec, boundspec)->is_default)
+ {
+ Oid partdefid;
+
+ partdefid = get_default_partition_oid(RelationGetRelid(rel));
+ Assert(partdefid == inhrelid);
+ }
Since an accidental change or database corruption may change the default
partition OID in pg_partition_table. An Assert won't help in such a case. May
be we should throw an error or at least report a warning. If we throw an error,
the table will become useless (or even the database will become useless
RelationBuildPartitionDesc is called from RelationCacheInitializePhase3() on
such a corrupted table). To avoid that we may raise a warning.

I am wondering whether we could avoid call to get_default_partition_oid() in
the above block, thus avoiding a sys cache lookup. The sys cache search
shouldn't be expensive since the cache should already have that entry, but
still if we can avoid it, we save some CPU cycles. The default partition OID is
stored in pg_partition_table catalog, which is looked up in
RelationGetPartitionKey(), a function which precedes RelationGetPartitionDesc()
everywhere. What if that RelationGetPartitionKey() also returns the default
partition OID and the common caller passes it to RelationGetPartitionDesc()?.

+ /* A partition cannot be attached if there exists a default partition */
+ defaultPartOid = get_default_partition_oid(RelationGetRelid(rel));
+ if (OidIsValid(defaultPartOid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot attach a new partition to table
\"%s\" having a default partition",
+ RelationGetRelationName(rel))));
get_default_partition_oid() searches the catalogs, which is not needed when we
have relation descriptor of the partitioned table (to which a new partition is
being attached). You should get the default partition OID from partition
descriptor. That will be cheaper.

+ /* If there isn't any constraint, show that explicitly */
+ if (partconstraintdef[0] == '\0')
+ printfPQExpBuffer(&tmpbuf, _("No partition constraint"));
I think we need to change the way we set partconstraintdef at
if (PQnfields(result) == 3)
partconstraintdef = PQgetvalue(result, 0, 2);
Before this commit, constraints will never be NULL so this code works, but now
that the cosntraints could be NULL, we need to check whether 3rd value is NULL
or not using PQgetisnull() and assigning a value only when it's not NULL.

+-- test adding default partition as first partition accepts any value including
grammar, reword as "test that a default partition added as the first
partition accepts any
value including".

>>
>> 0003:
>> Support for default partition with the restriction of preventing addition
>> of any
>> new partition after default partition. This is a merge of 0003 and 0004 in
>> V24 series.

The commit message of this patch has following line, which no more applies to
patch 0001. May be you want to remove this line or update the patch number.
3. This patch uses the refactored functions created in patch 0001
in this series.
Similarly the credit line refers to patch 0001. That too needs correction.

- * Also, invalidate the parent's relcache, so that the next rebuild will load
- * the new partition's info into its partition descriptor.
+ * Also, invalidate the parent's relcache entry, so that the next rebuild will
+ * load he new partition's info into its partition descriptor. If there is a
+ * default partition, we must invalidate its relcache entry as well.
Replacing "relcache" with "relcache entry" in the first sentence may be a good
idea, but is unrelated to this patch. I would leave that change aside and just
add comment about default partition.

+ /*
+ * The partition constraint for the default partition depends on the
+ * partition bounds of every other partition, so we must invalidate the
+ * relcache entry for that partition every time a partition is added or
+ * removed.
+ */
+ defaultPartOid = get_default_partition_oid(RelationGetRelid(parent));
+ if (OidIsValid(defaultPartOid))
+ CacheInvalidateRelcacheByRelid(defaultPartOid);
Again, since we have access to the parent's relcache, we should get the default
partition OID from relcache rather than catalogs.

The commit message of this patch has following line, which no more applies to
patch 0001. May be you want to remove this line or update the patch number.
3. This patch uses the refactored functions created in patch 0001
in this series.
Similarly the credit line refers to patch 0001. That too needs correction.

- * Also, invalidate the parent's relcache, so that the next rebuild will load
- * the new partition's info into its partition descriptor.
+ * Also, invalidate the parent's relcache entry, so that the next rebuild will
+ * load he new partition's info into its partition descriptor. If there is a
+ * default partition, we must invalidate its relcache entry as well.
Replacing "relcache" with "relcache entry" in the first sentence may be a good
idea, but is unrelated to this patch. I would leave that change aside and just
add comment about default partition.

+ /*
+ * The partition constraint for the default partition depends on the
+ * partition bounds of every other partition, so we must invalidate the
+ * relcache entry for that partition every time a partition is added or
+ * removed.
+ */
+ defaultPartOid = get_default_partition_oid(RelationGetRelid(parent));
+ if (OidIsValid(defaultPartOid))
+ CacheInvalidateRelcacheByRelid(defaultPartOid);
Again, since we have access to the parent's relcache, we should get the default
partition OID from relcache rather than catalogs.

I haven't gone through the full patch yet, so there may be more
comments here. There is some duplication of code in
check_default_allows_bound() and ValidatePartitionConstraints() to
scan the children of partition being validated. The difference is that
the first one scans the relations in the same function and the second
adds them to work queue. May be we could use
ValidatePartitionConstraints() to scan the relation or add to the
queue based on some input flag may be wqueue argument itself. But I
haven't thought through this completely. Any thoughts?

>>
>> 0004:
>> Extend default partitioning support to allow addition of new partitions
>> after
>> default partition is created/attached. This patch is a merge of patches
>> 0005 and 0006 in V24 series to simplify the review process. The
>> commit message has more details regarding what all is included.
>>
>> 0005:
>> This patch introduces code to check if the scanning of default partition
>> child
>> can be skipped if it's constraints are proven.
>>
>> 0006:
>> Documentation.
>>
>

I will get to these patches in a short while.

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