Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

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