Re: Adding support for Default partition in partitioning

Hi Ashutosh,

I have tried to address your comments in V27 patch series[1].
Please find my comments inlined.

> >>
> >> 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
>

If I understand correctly these comments refer to patch 0001 of V25_rebase
series, which is related to "Fix assumptions that get_qual_from_partbound()"
and not refactoring. This is patch 0001 in V27 now.

* 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.
>

I have changed the wording now.

>
> 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?
>

I have added a comment there.

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

Changed this to:
+ if (and_args == NIL)
+ result = NULL;

> 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."
>

Fixed.

>
> 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.
>

Added in commit message.

> >>
> >> 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.
>

Comments below refer to patch 0002 in V25_rebase(0003 in V25), which
adds basic support for default partition, which is now 0002 in V27.

> 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.
>

Fixed.

> 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.

Fixed.

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

In ATExecDetachPartition() we do not have OID of the relation being
detached
available at the time we lock default partition. Moreover, here we are
taking an
exclusive lock on default OID and an share lock on partition being detached.
As you correctly said in your earlier comment that it will be counted as
locked
twice, which to me also seems harmless. As these locks are to be held till
commit of the transaction nobody else is supposed to be releasing these
locks in
between. I am not able to visualize a problem here, but still I have tried
to
avoid locking the default partition table twice, please review the changes
and
let me know your thoughts.

> + /*
> + * 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 have added a warning here instead of Assert.

> 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()?.
>

The purpose here is to cross check the relid with partdefid stored in
catalog
pg_partitioned_table, though its going to be the same in the parents cache,
I
think its better that we retrieve it from the catalog syscache.
Further, RelationGetPartitionKey() is a macro and not a function, so
modifying
the existing simple macro for this reason does not sound a good idea to me.
Having said this I am open to ideas here.

> + /* 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.
>

Something like following can be done here:
/* A partition cannot be attached if there exists a default partition */
if (partition_bound_has_default(rel->partdesc->boundinfo))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("cannot attach a new partition to table \"%s\"
having a default partition",
RelationGetRelationName(rel))));

But, partition_bound_has_default() is defined in partition.c and not in
partition.h. This is done that way because boundinfo is not available in
partition.h. Further, this piece of code is removed in next patch where we
extend default partition support to add/attach partition even when default
partition exists. So, to me I don’t see much of the correction issue here.

Another way to get around this is, we can define another version of
get_default_partition_oid() something like
get_default_partition_oid_from_parent_rel()
in partition.c which looks around in relcache instead of catalog and
returns the
oid of default partition, or get_default_partition_oid() accepts both
parent OID,
and parent ‘Relation’ rel, if rel is not null look into relcahce and return,
else search from catalog using OID.

>
> + /* 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.
>

I have changed this to:
- if (PQnfields(result) == 3)
+ if (PQnfields(result) == 3 && !PQgetisnull(result,
0, 2))
partconstraintdef = PQgetvalue(result, 0,
2);

Please let me know if the change looks good to you.

> +-- 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".
>

changed the wording in the comment as suggested.

>
> >>
> >> 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.
>
Comments below rather seem to be for the patch that extends default
partition
such that new partition can be added even when default partition exists.
This
is 0003 patch in V27.

>
> 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.
>

Fixed commit message.

>
> - * 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.
>

Agree. Fixed.

> + /*
> + * 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.
>
>
This change is in heap.c, as I said above we would need to have a
different version of get_default_partition_oid() to address this.
Your thoughts?

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?
>

check_default_allows_bound() is called only from DefineRelation(),
and not for alter command. I am not really sure how can we use
work queue for create command.

[1]
https://www.postgresql.org/message-id/CAOgcT0OM_Px6BXp1uDhhArw0bm-q4zCD5YwhDywR3K9ziBNL6A%40mail.gmail.com

Regards,
Jeevan Ladhe