Horton

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Robert Haas <robertmhaas@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-22T16:43:35Z
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.

Hi!

On Thu, Aug 22, 2024 at 7:33 PM Robert Haas <robertmhaas@gmail.com> wrote:
> In response to some concerns raised about this fix on the
> pgsql-release list today, I spent some time investigating this patch.
> Unfortunately, I think there are too many problems here to be
> reasonably fixed before release, and I think all of SPLIT/MERGE
> PARTITION needs to be reverted.
>
> I focused my investigation on createPartitionTable(), which is a
> helper for both SPLIT PARTITION and MERGE PARTITION, and it works by
> consing up a CREATE TABLE AS statement and then feeding that back
> through
> ProcessUtility. I think it's bad design to use such a high-level
> facility here; it is unlike what we do elsewhere in tablecmds.c and
> opens us up to a variety of problems. The first thing that I
> discovered is that this patch does not fix all of the repeated name
> lookup problems. There is still this:
>
>     tlc->relation =
> makeRangeVar(get_namespace_name(RelationGetNamespace(modelRel)),
>                                  RelationGetRelationName(modelRel), -1);
>
> And also this:
>
>     createStmt->tablespacename =
> get_tablespace_name(modelRel->rd_rel->reltablespace);
>
> In both cases, we do a reverse lookup on an OID to get a name which
> the CREATE TABLE code will later turn back into an OID. If we don't
> get the same value, that's at least a bug and probably a security
> vulnerability, and there is no way to be certain that we will get the
> same value. The only remedy is to not repeat the lookup in the first
> place.
>
> Then I got to looking at this:
>
>     tlc->options = CREATE_TABLE_LIKE_ALL &
>         ~(CREATE_TABLE_LIKE_INDEXES | CREATE_TABLE_LIKE_IDENTITY |
> CREATE_TABLE_LIKE_STATISTICS);
>
> It's not obvious at first glance that there is a critical problem
> here, but there are reasons to be nervous. We're deploying a lot of
> machinery here to copy a lot of stuff and, while that's efficient from
> a coding perspective, it means that stuff you might not expect can
> just kind of happen. For instance:
>
> robert.haas=# \d+
>                                             List of relations
>  Schema | Name |       Type        |    Owner    | Persistence |
> Access method |    Size    | Description
> --------+------+-------------------+-------------+-------------+---------------+------------+-------------
>  public | foo  | partitioned table | robert.haas | permanent   |
>         | 0 bytes    |
>  public | foo1 | table             | robert.haas | permanent   | heap
>         | 8192 bytes |
>  public | foo2 | table             | bob         | permanent   | heap
>         | 8192 bytes |
> (3 rows)
> robert.haas=# alter table foo split partition foo2 into (partition
> foo3 for values from (10) to (15), partition foo4 for values from (15)
> to (20));
> ALTER TABLE
> robert.haas=# \d+
>                                             List of relations
>  Schema | Name |       Type        |    Owner    | Persistence |
> Access method |    Size    | Description
> --------+------+-------------------+-------------+-------------+---------------+------------+-------------
>  public | foo  | partitioned table | robert.haas | permanent   |
>         | 0 bytes    |
>  public | foo1 | table             | robert.haas | permanent   | heap
>         | 8192 bytes |
>  public | foo3 | table             | robert.haas | permanent   | heap
>         | 8192 bytes |
>  public | foo4 | table             | robert.haas | permanent   | heap
>         | 8192 bytes |
> (4 rows)
>
> I've split a partition owned by bob into two partitions owned by
> robert.haas. That's rather surprising. It doesn't work to split a
> partition that I don't own (and thus gain access to it) but if the
> superuser splits a non-superuser's partition, the superuser ends
> upowning the new partitions. I don't know if that's a vulnerability or
> just unexpected. However, then I found this, which I'm pretty well
> certain is a vulnerability:
>
> robert.haas=# set role bob;
> SET
> robert.haas=> create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> robert.haas=> create table foo1 partition of foo for values from (0) to (10);
> CREATE TABLE
> robert.haas=> create table foo2 partition of foo for values from (10) to (20);
> CREATE TABLE
> robert.haas=> insert into foo values (11, 'carrots'), (16, 'pineapple');
> INSERT 0 2
> robert.haas=> create or replace function run_me(integer) returns
> integer as $$begin raise notice 'you are running me as %',
> current_user; return $1; end$$ language plpgsql immutable;
> CREATE FUNCTION
> robert.haas=> create index on foo (run_me(a));
> NOTICE:  you are running me as bob
> NOTICE:  you are running me as bob
> CREATE INDEX
> robert.haas=> reset role;
> RESET
> robert.haas=# alter table foo split partition foo2 into (partition
> foo3 for values from (10) to (15), partition foo4 for values from (15)
> to (20));
> NOTICE:  you are running me as robert.haas
> NOTICE:  you are running me as robert.haas
> ALTER TABLE
>
> I think it is very unlikely that the problems mentioned above are the
> only ones. They're just what I found in an hour or two of testing.
> Even if they were, we're probably too close to release to be rushing
> out last minute fixes to multiple unanticipated security problems. But
> because of the design that was chosen here, I think there is probably
> more stuff here that is not right, some of which is security relevant
> and some of which is just a question of whether we're really getting
> the behavior that we want. And I don't think we can fix all that
> without either a very large number of grotty hacks similar to the one
> installed by 04158e7fa37c2dda9c3421ca922d02807b86df19, or a complete
> redesign of the feature. I believe the latter is probably a wiser
> course of action.

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.

------
Regards,
Alexander Korotkov
Supabase