Re: Declarative partitioning - another take

On Thu, Sep 15, 2016 at 4:53 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Wow, this is bad. What is needed in this case is "canonicalization" of
> the range partition bounds specified in the command.

I think we shouldn't worry about this. It seems like unnecessary
scope creep. All human beings capable of using PostgreSQL will
understand that there are no integers between 4 and 5, so any
practical impact on this would be on someone creating partitions
automatically. But if someone is creating partitions automatically
they are highly likely to be using the same EXCLUSIVE/INCLUSIVE
settings for all of the partitions, in which case this won't arise.
And if they aren't, then I think we should just make them deal with
this limitation in their code instead of dealing with it in our code.
This patch is plenty complicated enough already; introducing a whole
new canonicalization concept that will help practically nobody seems
to me to be going in the wrong direction. If somebody really cares
enough to want to try to fix this, they can submit a followup patch
someday.

> To mitigate this, how about we restrict range partition key to contain
> columns of only those types for which we know we can safely canonicalize a
> range bound (ie, discrete range types)? I don't think we can use, say,
> existing int4range_canonical but will have to write a version of it for
> partitioning usage (range bounds of partitions are different from what
> int4range_canonical is ready to handle). This approach will be very
> limiting as then range partitions will be limited to columns of int,
> bigint and date type only.

-1. That is letting the tail wag the dog. Let's leave it the way you
had it and be happy.

>> -- Observation 2 : able to create sub-partition out of the range set for
>> main table, causing not able to insert data satisfying any of the partition.
>>
>> create table test_subpart (c1 int) partition by range (c1);
>> create table test_subpart_p1 partition of test_subpart for values start (1)
>> end (100) inclusive partition by range (c1);
>> create table test_subpart_p1_sub1 partition of test_subpart_p1 for values
>> start (101) end (200);
>
> It seems that DDL should prevent the same column being used in partition
> key of lower level partitions. I don't know how much sense it would make,
> but being able to use the same column as partition key of lower level
> partitions may be a feature useful to some users if they know what they
> are doing. But this last part doesn't sound like a good thing. I
> modified the patch such that lower level partitions cannot use columns
> used by ancestor tables.

I again disagree. If somebody defines partition bounds that make it
impossible to insert the data that they care about, that's operator
error. The fact that, across multiple levels, you can manage to make
it impossible to insert any data at all does not make it anything
other than operator error. If we take the contrary position that it's
the system's job to prevent this sort of thing, we may disallow some
useful cases, like partitioning by the year portion of a date and then
subpartitioning by the month portion of that same date.

I think we'll probably also find that we're making the code
complicated to no purpose. For example, now you have to check when
attaching a partition that it doesn't violate the rule; otherwise you
end up with a table that can't be created directly (and thus can't
survive dump-and-restore) but can be created indirectly by exploiting
loopholes in the checks. It's tempting to think that we can check
simple cases - e.g. if the parent and the child are partitioning on
the same exact column, the child's range should be contained within
the parent's range - but more complicated cases are tricky. Suppose
the table is range-partitioned on (a, b) and range-subpartitioned on
b. It's not trivial to figure out whether the set of values that the
user can insert into that partition is non-empty. If we allow
partitioning on expressions, then it quickly becomes altogether
impossible to deduce anything useful - unless you can solve the
halting problem.

And, really, why do we care? If the user creates a partitioning
scheme that permits no rows in some or all of the partitions, then
they will have an empty table that can be correctly dumped and
restored but which cannot be used for anything useful unless it is
modified first. They probably don't want that, but it's not any more
broken than a table inheritance setup with mutually exclusive CHECK
constraints, or for that matter a plain table with mutually exclusive
CHECK constraints - and we don't try to prohibit those things. This
patch is supposed to be implementing partitioning, not artificial
intelligence.

>> -- Observation 3 : Getting cache lookup failed, when selecting list
>> partition table containing array.
>>
>> postgres=# SELECT tableoid::regclass,* FROM test_array;
>> ERROR: cache lookup failed for type 0
>
> That's a bug. Fixed in the attached patch.

Now on this one I'm not going to argue with your analysis. :-)

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