Re: [HACKERS] Adding support for Default partition in partitioning

On Thu, Jun 1, 2017 at 3:35 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Please let me know if I have missed anything and any further comments.

+                     errmsg("a default partition \"%s\" already exists",

I suggest: partition \"%s\" conflicts with existing default partition \"%s\"

The point is that's more similar to the message you get when overlap
&& !spec->is_default.

+     * If the default partition exists, it's partition constraint will change

it's -> its

+                         errmsg("default partition contains row(s)
that would overlap with partition being created")));

It doesn't really sound right to talk about rows overlapping with a
partition.  Partitions can overlap with each other, but not rows.
Also, it's not really project style to use ambiguously plural forms
like "row(s)" in error messages.  Maybe something like:

new partition constraint for default partition \"%s\" would be
violated by some row

+/*
+ * InvalidateDefaultPartitionRelcache
+ *
+ * Given a parent oid, this function checks if there exists a default partition
+ * and invalidates it's relcache if it exists.
+ */
+void
+InvalidateDefaultPartitionRelcache(Oid parentOid)
+{
+    Relation parent = heap_open(parentOid, AccessShareLock);
+    Oid default_relid =
parent->rd_partdesc->oids[DEFAULT_PARTITION_INDEX(parent)];
+
+    if (partition_bound_has_default(parent->rd_partdesc->boundinfo))
+        CacheInvalidateRelcacheByRelid(default_relid);
+
+    heap_close(parent, AccessShareLock);
+}

It does not seem like a good idea to put the heap_open() call inside
this function.  One of the two callers already *has* the Relation, and
we definitely want to avoid pulling the Oid out of the Relation only
to reopen it to get the Relation back.  And I think
heap_drop_with_catalog could open the parent relation instead of
calling LockRelationOid().

If DETACH PARTITION and DROP PARTITION require this, why not ATTACH
PARTITION and CREATE TABLE .. PARTITION OF?

The indentation of the changes in gram.y doesn't appear to match the
nearby code.  I'd remove this comment:

+             * Currently this is supported only for LIST partition.

Since nothing here is dependent on this working only for LIST
partitions, and since this will probably change, I think it would be
more future-proof to leave this out, lest somebody forget to update it
later.

-                switch (spec->strategy)
+                if (spec->is_default && (strategy == PARTITION_STRATEGY_LIST ||
+                                         strategy == PARTITION_STRATEGY_RANGE))

Checking strategy here appears pointless.

This is not a full review, but I'm out of time for today.

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