Re: Adding support for Default partition in partitioning

On Wed, Jul 12, 2017 at 3:31 PM, Jeevan Ladhe
<jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
> 0001:
> Refactoring existing ATExecAttachPartition code so that it can be used for
> default partitioning as well

Boring refactoring. Seems fine.

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

Perhaps it would be better to make validatePartConstraint() a no-op
when the constraint is empty rather than putting the logic in the
caller. Otherwise, every place that calls validatePartConstraint()
has to think about whether or not the constraint-is-NULL case needs to
be handled.

> 0003:
> Support for default partition with the restriction of preventing addition of
> any
> new partition after default partition.

This looks generally reasonable, but can't really be committed without
the later patches, because it might break pg_dump, which won't know
that the DEFAULT partition must be dumped last and might therefore get
the dump ordering wrong, and of course also because it reverts commit
c1e0e7e1d790bf18c913e6a452dea811e858b554.

> 0004:
> Store the default partition OID in pg_partition_table, this will help us to
> retrieve the OID of default relation when we don't have the relation cache
> available. This was also suggested by Amit Langote here[1].

I looked this over and I think this is the right approach. An
alternative way to avoid needing a relcache entry in
heap_drop_with_catalog() would be for get_default_partition_oid() to
call find_inheritance_children() here and then use a syscache lookup
to get the partition bound for each one, but that's still going to
cause some syscache bloat.

> 0005:
> Extend default partitioning support to allow addition of new partitions.

+ if (spec->is_default)
+ {
+ /* Default partition cannot be added if there already
exists one. */
+ if (partdesc->nparts > 0 &&
partition_bound_has_default(boundinfo))
+ {
+ with = boundinfo->default_index;
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("partition \"%s\"
conflicts with existing default partition \"%s\"",
+ relname,
get_rel_name(partdesc->oids[with])),
+ parser_errposition(pstate,
spec->location)));
+ }
+
+ return;
+ }

I generally think it's good to structure the code so as to minimize
the indentation level. In this case, if you did if (partdesc->nparts
== 0 || !partition_bound_has_default(boundinfo)) return; first, then
the rest of it could be one level less indented. Also, perhaps it
would be clearer to test boundinfo == NULL rather than
partdesc->nparts == 0, assuming they are equivalent.

- * We must also lock the default partition, for the same
reasons explained
- * in heap_drop_with_catalog().
+ * We must lock the default partition, for the same reasons explained in
+ * DefineRelation().

I don't really see the point of this change. Whichever earlier patch
adds this code could include or omit the word "also" as appropriate,
and then this patch wouldn't need to change it.

> 0006:
> Extend default partitioning validation code to reuse the refactored code in
> patch 0001.

I'm having a very hard time understanding what's going on with this
patch. It certainly doesn't seem to be just refactoring things to use
the code from 0001. For example:

- if (ExecCheck(partqualstate, econtext))
+ if (!ExecCheck(partqualstate, econtext))

It seems hard to believe that refactoring things to use the code from
0001 would involve inverting the value of this test.

+ * derived from the bounds(the partition constraint
never evaluates to
+ * NULL, so negating it like this is safe).

I don't see it being negated.

I think this patch needs a better explanation of what it's trying to
do, and better comments. I gather that at least part of the point
here is to skip validation scans on default partitions if the default
partition has been constrained not to contain any values that would
fall in the new partition, but neither the commit message for 0006 nor
your description here make that very clear.

> 0007:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.

If I understand correctly, this is actually a completely separate
feature not intrinsically related to default partitioning.

> [0008 documentation]

- attached is marked <literal>NO INHERIT</literal>, the command will fail;
- such a constraint must be recreated without the <literal>NO
INHERIT</literal>
- clause.
+ target table.
+ </para>

I don't favor inserting a paragraph break here.

+ then the default partition(if it is a regular table) is scanned to check

The sort-of-trivial problem with this is that an open parenthesis
should be proceeded by a space. But I think this won't be clear. I
think you should move this below the following paragraph, which
describes what happens for foreign tables, and then add a new
paragraph like this:

When a table has a default partition, defining a new partition changes
the partition constraint for the default partition. The default
partition can't contain any rows that would need to be moved to the
new partition, and will be scanned to verify that none are present.
This scan, like the scan of the new partition, can be avoided if an
appropriate <literal>CHECK</literal> constraint is present. Also like
the scan of the new partition, it is always skipped when the default
partition is a foreign table.

-) ] FOR VALUES <replaceable
class="PARAMETER">partition_bound_spec</replaceable>
+) ] { DEFAULT | FOR VALUES <replaceable
class="PARAMETER">partition_bound_spec</replaceable> }

I recommend writing FOR VALUES | DEFAULT both here and in the ATTACH
PARTITION syntax summary.

+ If <literal>DEFAULT</literal> is specified the table will be created as a
+ default partition of the parent table. The parent can either be a list or
+ range partitioned table. A partition key value not fitting into any other
+ partition of the given parent will be routed to the default partition.
+ There can be only one default partition for a given parent table.
+ </para>
+
+ <para>
+ If the given parent is already having a default partition then adding a
+ new partition would result in an error if the default partition contains a
+ record that would fit in the new partition being added. This check is not
+ performed if the default partition is a foreign table.
+ </para>

The indentation isn't correct here - it doesn't match the surrounding
paragraphs. The bit about list or range partitioning doesn't match
the actual behavior of the other patches, but maybe you intended this
to document both this feature and what Beena's doing.

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