Re: Declarative partitioning - another take

On Tue, Oct 4, 2016 at 4:02 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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