Re: Adding support for Default partition in partitioning

On 2017/05/31 9:33, Amit Langote wrote:
> On 2017/05/30 16:38, Jeevan Ladhe wrote:
>> I have rebased the patch on the latest commit.
>> PFA.
>
> Was looking at the patch

I tried creating default partition of a range-partitioned table and got
the following error:

ERROR: invalid bound specification for a range partition

I thought it would give:

ERROR: creating default partition is not supported for range partitioned
tables

Which means transformPartitionBound() should perform this check more
carefully. As I suggested in my previous email, if there were a
is_default field in the PartitionBoundSpec, then one could add the
following block of code at the beginning of transformPartitionBound:

if (spec->is_default && spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("creating default partition is not supported for %s
partitioned tables", get_partition_strategy_name(key->strategy))));

Some more comments on the patch:

+ errmsg("new default partition constraint is
violated by some row"),

"new default partition constraint" may sound a bit confusing to users.
That we recompute the default partition's constraint and check the "new
constraint" against the rows it contains seems to me to be the description
of internal details. How about:

ERROR: default partition contains rows that belong to partition being created

+char *ExecBuildSlotValueDescription(Oid reloid,
+ TupleTableSlot *slot,
+ TupleDesc tupdesc,
+ Bitmapset *modifiedCols,
+ int maxfieldlen);

It seems that you made the above public to use it in
check_default_allows_bound(), which while harmless, I'm not sure if
needed. ATRewriteTable() in tablecmds.c, for example, emits the following
error messages:

errmsg("check constraint \"%s\" is violated by some row",

errmsg("partition constraint is violated by some row")));

but neither outputs the DETAIL part showing exactly what row. I think
it's fine for check_default_allows_bound() not to show the row itself and
hence no need to make ExecBuildSlotValueDescription public.

In get_rule_expr():

case PARTITION_STRATEGY_LIST:
Assert(spec->listdatums != NIL);

+ /*
+ * If the boundspec is of Default partition, it does
+ * not have list of datums, but has only one node to
+ * indicate its a default partition.
+ */
+ if (isDefaultPartitionBound(
+ (Node *) linitial(spec->listdatums)))
+ {
+ appendStringInfoString(buf, "DEFAULT");
+ break;
+ }
+

How about adding this part before the switch (key->strategy)? That way,
we won't have to come back and add this again when we add range default
partitions.

Gotta go; will provide more comments later.

Thanks,
Amit