On Tue, Oct 4, 2016 at 4:02 AM, Amit Langote <langote_amit...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
