Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Hi, Alexander!

Thank you for notes.

1.
>+ ExecClearTuple(insertslot);
>+
>+ memcpy(insertslot->tts_values, srcslot->tts_values,
>+ sizeof(Datum) * srcslot->tts_nvalid);
>+ memcpy(insertslot->tts_isnull, srcslot->tts_isnull,
>+ sizeof(bool) * srcslot->tts_nvalid);
>+
>+ ExecStoreVirtualTuple(insertslot);
>
> Similar fragments are present in SplitPartitionMoveRows() and
> MergePartitionsMoveRows(). Do you think we could use ExecCopySlot()
> (or similar) instead?

I think ExecCopySlot function is not suitable here because of
tts_virtual_materialize function
(ExecCopySlot -> tts_virtual_copyslot -> tts_virtual_materialize).
It is good to use the same data in insertslot as in srcslot, without
materialization.
Therefore, it seems to me that it is better not to change the existing
code, especially since it is similar to the code of the ATRewriteTable
function [1] and is understandable.

2.
> The presence of the same name in the split partition list is checked
> with equal() (similar concern was already raised about the merge case
> [1]). If one of the names is schema-qualified, the error message is
> different. Could we make them the same?

Corrected. Added namespace comparison for this case.

3.
>+ /* Search partition for current slot srcslot. */
>+ foreach(listptr, partContexts)
>+ {
>+ pc = (SplitPartitionContext *) lfirst(listptr);
>+
>+ /* skip DEFAULT partition */
>+ if (pc->partqualstate && ExecCheck(pc->partqualstate, econtext))
>+ {
>+ found = true;
>+ break;
>+ }
>+ }
> I see we're searching for a partition to place each row using the
> sequential application of partition constraints. I concerned if this
> could be exhausting when the number of new partitions is large.
> Could we use something like binary search here?

I think binary search for this case would make the code much more
complicated. And is binary search really needed for real cases?
I assume that the main use of SPLIT PARTITION will be split into a
small number of partitions, for example:

* for extracting partition from the DEFAULT partition;
* splitting partition for big intreval into partitions for smaller
intervals (for example, splitting a partition for quarter/year into
partitions for months).
* ...

In these cases, we highly likely won't need binary search.

4.
> New split/drop commands do the full reorganization of the involved
> partitions. As Robert previously stated, there are other
> possible strategies. While they are hard to implement, I don't think
> we need them in the initial version. But I think it's worth
> mentioning in the docs that we're completely rewriting the involved
> partitions. And this is not recommended to use for merging very big
> partitions with small ones, and splitting a small fraction of rows out
> of a very big partition.

Added notes to documentation (sorry my poor English).

5.
> Both patches could use pgindent run.

Fixed.

Links
-----
[1]
https://github.com/postgres/postgres/blob/71ea0d6795438f95f4ee6e35867058c44b270d51/src/backend/commands/tablecmds.c#L6361

--
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com