Horton

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Dmitry Koval <d.koval@postgrespro.ru>, Noah Misch <noah@leadboat.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-08-22T17:25:07Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Adjust errcode in checkPartition()

  2. Fix usage of palloc() in MERGE/SPLIT PARTITION(s) code

  3. Implement ALTER TABLE ... SPLIT PARTITION ... command

  4. Implement ALTER TABLE ... MERGE PARTITIONS ... command

  5. Calculate agglevelsup correctly when Aggref contains a CTE.

  6. Use PqMsg_* macros in applyparallelworker.c.

  7. Restrict psql meta-commands in plain-text dumps.

  8. Ensure we have a snapshot when updating various system catalogs.

  9. Use specific collation where needed in new test

  10. Expand virtual generated columns in the planner

  11. Virtual generated columns

  12. Define PG_LOGICAL_DIR for path pg_logical/ in data folder

  13. Revert support for ALTER TABLE ... MERGE/SPLIT PARTITION(S) commands

  14. Avoid repeated table name lookups in createPartitionTable()

  15. Provide deterministic order for catalog queries in partition_split.sql

  16. Don't copy extended statistics during MERGE/SPLIT partition operations

  17. Fix the name collision detection in MERGE/SPLIT partition operations

  18. Fix regression tests conflict in 3ca43dbbb6

  19. Add permission check for MERGE/SPLIT partition operations

  20. Fix one more portability shortcoming in new test_pg_dump test.

  21. Inherit parent's AM for partition MERGE/SPLIT operations

  22. Add tab completion for partition MERGE/SPLIT operations

  23. Rename tables in tests of partition MERGE/SPLIT operations

  24. Make new partitions with parent's persistence during MERGE/SPLIT

  25. Document the way partition MERGE/SPLIT operations create new partitions

  26. Change the way ATExecMergePartitions() handles the name collision

  27. Grammar fixes for split/merge partitions code

  28. Checks for ALTER TABLE ... SPLIT/MERGE PARTITIONS ... commands

  29. Fix some grammer errors from error messages and codes comments

  30. Support TZ and OF format codes in to_timestamp().

  31. Support identity columns in partitioned tables

  32. Fix indentation in twophase.c

  33. Fix corner-case planner failure for MERGE.

  34. Doc: fix documentation example for bytea hex output format.

  35. Avoid repeated name lookups during table and index DDL.

On Thu, Aug 22, 2024 at 12:43 PM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
> Thank you for your feedback.  Yes, it seems that there is not enough
> time to even carefully analyze all the issues in these features.  The
> rule of thumb I can get from this experience is "think multiple times
> before accessing something already opened by its name".  I'm going to
> revert these features during next couple days.

Thanks, and sorry about that. I would say even "think multiple times"
is possibly not strong enough -- it might almost be "just don't ever
do it". Even if (in some particular case) the invalidation mechanism
seems to protect you from getting wrong answers, there are often holes
in that, specifically around search_path = foo, bar and you're
operating on an object in schema bar and an identically-named object
is created in schema foo at just the wrong time. Sometimes there are
problems even when search_path is not involved, but when it is, there
are more.

Here, aside from the name lookup issues, there are also problems with
expression evaluation: we can't split partitions without reindexing
rows that those partitions contain, and it is critical to think
through which is going to do the evaluation and make sure it's
properly sandboxed. I think we might need
SECURITY_RESTRICTED_OPERATION here.

Another thing I want to highlight if you do have another go at this
patch is that it's really critical to think about where every single
property of the newly-created tables comes from. The original patch
didn't consider relpersistence or tableam, and here I just discovered
that owner is also an issue that probably needs more consideration,
but it goes way beyond that. For example, I was surprised to discover
that if I put per-partition constraints or triggers on a partition and
then split it, they were not duplicated to the new partitions. Now,
maybe that's actually the behavior we want -- I'm not 100% positive --
but it sure wasn't what I was expecting. If we did duplicate them when
splitting, then what's supposed to happen when merging occurs? That is
not at all obvious, at least to me, but it needs careful thought. ACLs
and rules and default values and foreign keys (both outbond and
inbound) all need to be considered too, along with 27 other things
that I'm sure I'm not thinking about right now. Some of this behavior
should probably be explicitly documented, but all of it should be
considered carefully enough before commit to avoid surprises later. I
say that both from a security point of view and also just from a user
experience point of view. Even if things aren't insecure, they can
still be annoying, but it's not uncommon in cases like this for
annoying things to turn out to also be insecure.

Finally, if you do revisit this, I believe it would be a good idea to
think a bit harder about how data is moved around. My impression (and
please correct me if I am mistaken) is that currently, any split or
merge operation rewrites all the data in the source partition(s). If a
large partition is being split nearly equally, I think that has a good
chance of being optimal, but I think that might be the only case. If
we're merging partitions, wouldn't it be better to adjust the
constraints on the first partition -- or perhaps the largest partition
if we want to be clever -- and insert the data from all of the others
into it? Maybe that would even have syntax that puts the user in
control of which partition survives, e.g. ALTER TABLE tab1 MERGE
PARTITION part1 WITH part2, part3, .... That would also make it really
obvious to the user what all of the properties of part1 will be after
the merge: they will be exactly the same as they were before the
merge, except that the partition constraint will have been adjusted.
You basically dodge everything in the previous paragraph in one shot,
and it seems like it would also be faster. Splitting there's no
similar get-out-of-jail free card, at least not that I can see. Even
if you add syntax that splits a partition by using INSERT/DELETE to
move some rows to a newly-created partition, you still have to make at
least one new partition. But possibly that syntax is worth having
anyway, because it would be a lot quicker in the case of a highly
asymmetric split. On the other hand, maybe even splits are much more
likely and we don't really need it. I don't know.

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