Re: Declarative partitioning - another take

Hi Jaime,

On 2016/11/08 2:15, Jaime Casanova wrote:
> On 28 October 2016 at 02:53, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> I started to review the functionality of this patch, so i applied all
> 9 patches. After that i found this warning, which i guess is because
> it needs a cast.

Thanks a ton for reviewing!

> After that, i tried a case that i think is important: to partition an
> already existing table. Because there is no ALTER TABL SET PARTITION
> or something similar (which i think makes sense because such a command
> would need to create the partitions and move the rows to follow the
> rule that there is no rows in a parent table).
>
> So, what i tried was:
>
> 1) rename original table
> 2) create a new partitioned table with old name
> 3) attach old table as a partition with bounds outside normal bounds
> and no validate
>
> the idea is to start attaching valid partitions and delete and insert
> rows from the invalid one (is there a better way of doing that?), that
> will allow to partition a table easily.

So, step 3 creates a partition that is basically unbounded. From there,
it seems you want to create partitions with proper bounds, moving data
into them as they are created. It seems like a job of some redistribution
command (split partition?) which is currently unsupported.

> So far so good, until i decided points 1 to 3 should happen inside a
> transaction to make things transparent to the user.
>
> Attached is script that shows the failure when trying it:
>
> script 1 (failing_test_1.sql) fails the assert
> "Assert(RelationGetPartitionKey(parentRel) != NULL);" in
> transformAttachPartition() at src/backend/parser/parse_utilcmd.c:3164

Thanks for the test. This test uncovered a bug which I have fixed in my
local repository. Given the fix the above will work, although I see that
it's not the best way to do what you want to do.

> After that i tried the same but with an already partitioned (via
> inheritance) table and got this (i did this first without a
> transaction, doing it with a transaction will show the same failure as
> before):
>
> script 2 (failing_test_2.sql) fails the assert
> "Assert(childrte->relkind == RELKIND_PARTITIONED_TABLE);" in
> expand_inherited_rte_internal() at
> src/backend/optimizer/prep/prepunion.c:1551

This again was an oversight/bug in the patch. It's not supported to
combine old-style inheritance partitioning with the new partitioned
tables. In fact, ATTACH PARTITION prevents adding a regular inheritance
parent as partition. After fixing the bug, you would instead get this error:

alter table prueba attach partition prueba_old for values start
(unbounded, unbounded) end (2008,1) no validate;
ERROR: cannot attach regular inheritance parent as partition

In this case, you should instead create prueba_old partition hierarchy
using the new partitioning commands and then attach the same.

> PS: i don't like the START END syntax but i don't have any ideas
> there... except, there was a reason not to use expressions (like a
> CHECK constraint?)

Expression syntax is too unrestricted. What's to prevent users from using
completely different check constraint expressions from partition to
partition (not that any users deliberately try do that)? With the new
partitioning syntax, you specify the partitioning columns once when
creating the partitioned parent and then specify, using partitioning
method specific syntax, the bounds for every partition of the parent.
That allows to capture the partition metadata in a form that is more
suitable to implement other features related to partitioning.

That said, I think it's always a challenge to come up with a syntax that
is universally acceptable. For example, the recent discussion about
whether to allow inclusive/exclusive to be specified for START and END
bounds of range partitions or always assume inclusive start and exclusive
end [1].

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoaKOycHcVoed%2BF3fk-z6xUOeysQFG6HT%3Doucw76bSMHCQ%40mail.gmail.com