Re: Declarative partitioning - another take - Mailing list pgsql-hackers

On Wed, Aug 10, 2016 at 7:09 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 0003-Catalog-and-DDL-for-partition-bounds.patch
>
> Partition DDL includes both a way to create new partition and "attach" an
> existing table as a partition of parent partitioned table.  Attempt to
> drop a partition using DROP TABLE causes an error. Instead a partition
> needs first to be "detached" from parent partitioned table.  On the other
> hand, dropping the parent drops all the partitions if CASCADE is specified.

Hmm, I don't think I like this.  Why should it be necessary to detach
a partition before dropping it?  That seems like an unnecessary step.
    [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY
IMMEDIATE ]
+</synopsis>

Unnecessary hunk.

+     <para>
+      If this table is a partition, one cannot perform <literal>DROP
NOT NULL</>
+      on a column if it is marked not null in the parent table.
+      not null.
+     </para>

Sentence fragment.

+     <para>
+      Note that unlike the <literal>ATTACH PARTITION</> command, a partition
+      being detached can be itself partitioned.  In that case, it continues
+      to exist as such.
+     </para>

This is another restriction I don't understand.  Why can't I attach a
partitioned table?

+        indicate that descendant tables are included.  Note that whether
+        <literal>ONLY</> or <literal>*</> is specified has no effect in case
+        of a partitioned table; descendant tables (in this case, partitions)
+        are always included.

Ugh, why?  I think this should work exactly the same way for
partitioned tables that it does for any other inheritance hierarchy.
Sure, you'll get no rows, but who cares?

+CREATE FOREIGN TABLE measurement_y2016m07
+    PARTITION OF measurement FOR VALUES START ('2016-07-01') END
('2016-08-01');
+    SERVER server_07;

Extra semicolon?

+      A partition cannot have columns other than those inherited from the
+      parent.  That includes the <structfield>oid</> column, which can be

I think experience suggests that this is a good restriction, but then
why does the syntax synopsis indicate that PARTITION BY can be
specified along with column definitions?  Similarly for CREATE FOREIGN
TABLE.

+      When specifying for a table being created as partition, one needs to
+      use column names from the parent table as part of the key.

This is not very clear.

-       /* Remove NO INHERIT flag if rel is a partitioned table */
-       if (relid_is_partitioned(relid))
+       /* Discard NO INHERIT, if relation is a partitioned table or a
partition */
+       if (relid_is_partitioned(relid) || relid_is_partition(relid))               is_no_inherit = false;

It might be right to disallow NO INHERIT in this case, but I don't
think it can be right to just silently ignore it.

+ * Not flushed from the cache by RelationClearRelation() unless changed because
+ * of addition or removal of partitions.

This seems unlikely to be safe, unless I'm missing something.

+       form = (Form_pg_inherits) GETSTRUCT(tuple);
+
+       systable_endscan(scan);
+       heap_close(catalogRelation, AccessShareLock);
+
+       return form->inhparent;

This is unsafe.  After systable_endscan, it is no longer OK to access
form->inhparent.

Try building with CLOBBER_CACHE_ALWAYS to find other cache flush hazards.

There should probably be a note in the function header comment that it
is unsafe to use this for an inheritance child that is not a
partition, because there could be more than one parent in that case.
Or maybe the whole idea of this function just isn't very sound...

+static List *
+get_partitions(Oid relid, int lockmode)
+{
+       return find_inheritance_children(relid, lockmode);
+}

What's the point?  If we're going to have a wrapper here at all, then
shouldn't it have a name that matches the existing convention - e.g.
find_partitions() or find_child_partitions()?  But I think you might
as well just use find_inheritance_children() directly.

+                * Happens when we have created the pg_inherits entry
but not the
+                * pg_partition entry yet.

Why do we ever allow the flow of control to reach this point while we
are in such an intermediate state?

+free_partition_info(PartitionInfoData **p, int num)

Seems very error-prone.  Isn't this why MemoryContextReset was invented?

+relid_is_partition(Oid relid)
+{
+       return SearchSysCacheExists1(PARTRELID, ObjectIdGetDatum(relid));
+}

This is used in a lot of places, and the overhead of checking it in
all of those places is not necessarily nil.  Syscache lookups aren't
free.  What if we didn't create a new catalog for this and instead
just added a relpartitionbound attribute to pg_class?  It seems a bit
silly to have a whole extra catalog to store one extra column...
       /*
+        * If this foreign table is a partition, check that the FDW supports
+        * insert.
+        */
+       if (stmt->base.partbound != NULL)
+       {
+               FdwRoutine *fdw_routine;
+
+               fdw_routine = GetFdwRoutine(fdw->fdwhandler);
+               if (fdw_routine->ExecForeignInsert == NULL)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_FDW_NO_SCHEMAS),
+                                        errmsg("cannot create foreign
table as partition"),
+                                        errdetail("foreign-data
wrapper \"%s\" does not support insert",
+                                                       fdw->fdwname)));
+       }

Why?  This seems like an entirely arbitrary prohibition.  If inserts
aren't supported, then they'll fail at runtime.  Same for updates or
deletes, for which you have not added checks.  I think you should just
remove this.

+               /* Force inheritance recursion, if partitioned table. */
+               if (recurse || relid_is_partitioned(myrelid))

Disagree with this, too.  There's no reason for partitioned tables to
be special in this way.  Similarly, disagree with all of the places
that do something similar.

-                               errmsg("column \"%s\" in child table
must be marked NOT NULL",
-                                          attributeName)));
+                               errmsg("column \"%s\" in %s table must
be marked NOT NULL",
+                                          attributeName,
+                                          is_attach_partition ?
"source" : "child")));

You have a few of these; they cause problems for translators, because
different languages have different word ordering.  Repeat the entire
message instead: is_attach_partition ? "column \"%s\" in source table
must be marked NOT NULL" : "column \"%s\" in child table must be
marked NOT NULL".

+-- XXX add ownership tests

So do that.  :-)

+ERROR:  column "b" is not null in parent
+HINT:  Please drop not null in the parent instead

Hmm.   That hint doesn't seem like project style, and I'm not sure
that it really makes sense to issue such a hint anyway.  Who knows
whether that is the right thing to do?  I think you should somehow be
complaining about the fact that this is a partition, rather than
complaining about the fact that the column is NOT NULL in the parent.
Are we insisting that the flags match exactly, or only that the child
may not allow nulls unless the parent does?

+ERROR:  new partition's list of values overlaps with partition
"lpart1" of "list_parted"

Maybe just:

ERROR: partitions must not overlap
-or-
ERROR: partition "%s" would overlap partition "%s"

As before, this is just an initial read-through, so apologies for
whatever I may have missed.

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