Re: Partitioning syntax

On Mon, Jan 18, 2010 at 3:55 AM, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> A couple of preliminary comments on this:
>
> Thanks.
> The attached is rebased on HEAD, with additional documentation.
>
>> 1. If we're thinking that this syntax should eventually result in
>> inserts (and updates?) being redirected to the appropriate partition,
>> then I think we should have that in the initial version.  I don't
>> think we really want to add the syntax with a plan to change its
>> behavior incompatibly down the road.
>
> It's true that we need an alternative method for insert triggers,
> but I'd like to submit it as another patch in the next development cycle.
> I think the syntax proposed here is carefully chosen so that we will
> not frequently modify modify them.

But I think users have the right to expect that the behavior of a
feature will be consistent. If, in 9.0, we add this feature and tell
people to start using it, then when 9.1 comes out and includes
automatic routing of inserts to the proper partition, their
applications will break. I don't think there's enough value in this
by itself to think that we're getting something by committing it now -
it's better to let people do it the way they have been for 9.0, and
then have a more complete implementation for 9.1. Maybe if we were at
the very beginning of the release cycle we could think about
committing this first and then making the other changes before
release, but even that is something we usually shy away from, in case
people get bogged down and don't have time to finish the work.

>> 2. The documentation does not explain what partitioning by list or by
>> range means, or what the difference between the two is.  I think some
>> kind of general introduction to the subject is essential.
>
> Oops, I've forgotten to update ddl.sgml for the new syntax. I rewrote
> ddl-partition section to use the new syntax, with some other changes:
>
>  1. Use PARTITION BY RANGE and CREATE PARTITION for the example.
>
>  2. Recommend to define indexes for the parent table, and copy the
>     definition to each partition with CREATE PARTITION or
>     CREATE TABLE (LIKE INCLUDING ALL).
>
>  3. Use an EXECUTE USING rather than a huge IF-THEN-ELSE block
>     in the insert trigger. The new recommended code is:
>       EXECUTE 'INSERT INTO ' || to_relname(NEW) || ' VALUES ($1.*)' USING NEW;
>     We don't need to update the trigger function if partitions are
>     added or removed with this form.
>
> We could apply changes in 2 and 3 even without the partitioning patch.
> I think we can define trigger functions with EXECUTE USING easliy
> compared to before. I'm willing to split the doc patch if needed.
>
>> 3. This patch is large enough (+1951/-63) that we have to consider
>> whether it makes sense to merge it at this point in the release cycle.
>>  It doesn't change much existing code, which is a point in its favor,
>> but it's still a big patch.  I guess we can wait until we're a little
>> further along to make that decision.
>
> It depends on reviewers :)  But I think the partitioning patch never
> go to waste -- I'll continue to improve it if any feedbacks.

I agree - it will not go to waste. I'm glad you're planning to
continue working on it. I'll try to take a little bit more look at
the code (or perhaps someone else will weigh in) but I don't think we
should expect it to go in this time around.

...Robert