Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

On Thu, Apr 18, 2024 at 6:35 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> The revised patchset is attached.
> 1) I've split the fix for the CommandCounterIncrement() issue and the
> fix for relation persistence issue into a separate patch.
> 2) I've validated that the lock on the new partition is held in
> createPartitionTable() after ProcessUtility() as pointed out by
> Robert. So, no need to place the lock again.
> 3) Added fix for problematic error message as a separate patch [1].
> 4) Added rename "salemans" => "salesmen" for tests as a separate patch.
>
> I think these fixes are reaching committable shape, but I'd like
> someone to check it before I push.

Reviewing 0001:

- Seems mostly fine. I think the comment /* Unlock and drop merged
partitions. */ is wrong. I think it should say something like /* Drop
the current partitions before adding the new one. */ because (a) it
doesn't unlock anything, and there's another comment saying that and
(b) we now know that the drop vs. add order matters.

Reviewing 0002:

- Commit message typos: behavious, corresponsing

- Given the change to the header comment of createPartitionTable, it's
rather surprising to me that this patch doesn't touch the
documentation. Isn't that a big change in semantics?

- My previous review comment was really about the code comment, I
believe, rather than the use of AccessExclusiveLock. NoLock is
probably fine, but if it were me I'd be tempted to write
AccessExclusiveLock and just make the comment say something like /* We
should already have the lock, but do it this way just to be certain
*/. But what you have is probably fine, too. Mostly, I want to clarify
the intent of my previous comment.

- Do we, or can we, have a test that if you split a partition that's
not in the search path, the resulting partitions end up in your
creation namespace? And similarly for merge? And maybe also that
schema-qualification works properly?

I haven't exhaustively verified the patch, but these are some things I
noticed when scrolling through it.

Reviewing 0003:

- Are you sure this can't dereference datum when datum is NULL, in
either the upper or lower half? It sure looks strange to have code
that looks like it can make datum a null pointer, and then an
unconditional deference just after.

- In general I think the wording changes are improvements. I'm
slightly suspicious that there might be an even better way to word it,
but I can't think of it right at this very moment.

- I'm kind of unhappy (but not totally unhappy) with the semantics.
Suppose I have a partition that allows values from 0 to 1000, but
actually only contains values that are either between 0 and 99 or
between 901 and 1000. If I try to to split the partition into one that
allows 0..100 and a second that allows 900..1000, it will fail. Maybe
that's good, because that means that if a failure is going to happen,
it will happen right at the beginning, rather than maybe after doing a
lot of work. But on the other hand, it also kind of stinks, because it
feels like I'm being told I can't do something that I know is
perfectly fine.

Reviewing 0004:

- Obviously this is quite trivial and there's no real problem with it,
but if we're changing it anyway, how about a gender-neutral term
(salesperson/salespeople)?

--
Robert Haas
EDB: http://www.enterprisedb.com