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

On Tue, Oct 4, 2016 at 4:02 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Even if we leave the empty relfilenode around for now -- in the long
>> run I think it should die -- I think we should prohibit the creation
>> of subsidiary object on the parent which is only sensible if it has
>> rows - e.g. indexes.  It makes no sense to disallow non-inheritable
>> constraints while allowing indexes, and it could box us into a corner
>> later.
>
> I agree.  So we must prevent from the get-go the creation of following
> objects on parent tables (aka RELKIND_PARTITIONED_TABLE relations):
>
> * Indexes
> * Row triggers (?)

Hmm, do we ever fire triggers on the parent for operations on a child
table?  Note this thread, which seems possibly relevant:

https://www.postgresql.org/message-id/flat/cd282adde5b70b20c57f53bb9ab75e27%40biglumber.com

We certainly won't fire the parent's per-row triggers ever, so those
should be prohibited.  But I'm not sure about per-statement triggers.

> In addition to preventing creation of these objects, we must also teach
> commands that directly invoke heapam.c to skip such relations.

I'm don't think that's required if we're not actually getting rid of
the relfilenode.

> In this case, relkind refers to the command viz. DROP <ObjectType/relkind>
> <relname>.  It can never be RELKIND_PARTITIONED_TABLE, so the above will
> be equivalent to leaving things unchanged.  Anyway I agree the suggested
> style is better, but I assume you meant:
>
>     if (classform->relkind == RELKIND_PARTITIONED_TABLE)
>         expected_relkind = RELKIND_RELATION;
>     else
>         expected_relkind = classform->relkind;
>
>     if (relkind != expected_relkind)
>         DropErrorMsgWrongType(rel->relname, classform->relkind, relkind);

Uh, yeah, probably that's what I meant. :-)

> Thanks for the explanation. I added a new field to the catalog called
> partcollation.
>
> That means a couple of things - when comparing input key values (consider
> a new tuple being routed) to partition bounds using the btree compare
> function, we use the corresponding key column's partcollation.  The same
> is also used as inputcollid of the OpExpr (or ScalarArrayOpExpr) in the
> implicitly generated CHECK constraints.

Check.

> However, varcollid of any Var nodes in the above expressions is still the
> corresponding attributes' attcollation.

I think that's OK.

> Needless to say, a query-specified qualification clause must now include
> the same collate clause as used for individual partition columns (or
> expressions) for the constraint exclusion to work as intended.

Hopefully this is only required if the default choice of collation
wouldn't be correct anyway.

>> +      <entry><structfield>partexprbin</structfield></entry>
>>
>> Is there any good reason not to do partexprbin -> partexpr throughout the patch?
>
> Agreed. Though I used partexprs (like indexprs).

Oh, good idea.

>>          case EXPR_KIND_TRIGGER_WHEN:
>>              return "WHEN";
>> +        case EXPR_KIND_PARTITION_KEY:
>> +            return "partition key expression";
>>
>> I think you should say "PARTITION BY" here.  See the function header
>> comment for ParseExprKindName.
>
> Used "PARTITION BY".  I was trying to mimic EXPR_KIND_INDEX_EXPRESSION,
> but this seems to work better.  Also, I renamed EXPR_KIND_PARTITION_KEY to
> EXPR_KIND_PARTITION_EXPRESSION.
>
> By the way, a bunch of error messages say "partition key expression".  I'm
> assuming it's better to leave them alone, that is, not rewrite them as
> "PARTITION BY expressions".

I think that is fine.  ParseExprKindName is something of a special case.

Reviewing 0002:

+    prettyFlags = PRETTYFLAG_INDENT;
+    PG_RETURN_TEXT_P(string_to_text(pg_get_partkeydef_worker(relid,
+                                    prettyFlags)));

Why bother with the variable?

+    /* Must get partclass, and partexprs the hard way */
+    datum = SysCacheGetAttr(PARTEDRELID, tuple,
+                            Anum_pg_partitioned_table_partclass, &isnull);
+    Assert(!isnull);
+    partclass = (oidvector *) DatumGetPointer(datum);

Comment mentions getting two things, but code only gets one.

+        partexprs = (List *) stringToNode(exprsString);

if (!IsA(partexprs, List)) elog(ERROR, ...); so as to guard against
corrupt catalog contents.

+    switch (form->partstrat)
+    {
+        case 'l':
+            appendStringInfo(&buf, "LIST");
+            break;
+        case 'r':
+            appendStringInfo(&buf, "RANGE");
+            break;
+    }

default: elog(ERROR, "unexpected partition strategy: %d", (int)
form->partstrat);

Also, why not case PARTITION_STRATEGY_LIST: instead of case 'l': and
similarly for 'r'?

@@ -5296,7 +5301,8 @@ getTables(Archive *fout, int *numTables)                          "OR %s IS NOT NULL "
             "OR %s IS NOT NULL"                          "))"
 
-                          "AS changed_acl "
+                          "AS changed_acl, "
+                          "CASE WHEN c.relkind = 'P' THEN
pg_catalog.pg_get_partkeydef(c.oid) ELSE NULL END AS partkeydef "                          "FROM pg_class c "
              "LEFT JOIN pg_depend d ON "                          "(c.relkind = '%c' AND "
 

I think if you test it you'll find this breaks dumps from older server
versions.  You've got to add a new version of the SQL query for 10+,
and then update the 9.6, 9.5, 9.4, 9.3, 9.1-9.2, 9.0, 8.4, 8.2-8.3,
8.0-8.1, 7.3-7.4, 7.2, 7.1, and pre-7.1 queries to return a dummy NULL
value for that column. Alternatively, you can add the new version for
10.0, leave the older queries alone, and then adjust the code further
down to cope with i_partkeydef == -1 (see i_checkoption for an
example).
                      gettext_noop("table"),
+                      gettext_noop("table"),

Add a comment like /* partitioned table */ on the same line.

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