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-22T16:33:27Z
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,

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.

...Robert