Skip to content

Commit 02c359e

Browse files
tglsfdctglsfdc
committed
Apply stopgap fix for bug #15672.
Fix DefineIndex so that it doesn't attempt to pass down a to-be-reused index relfilenode to a child index creation, and fix TryReuseIndex to not think that reuse is sensible for a partitioned index. In v11, this fixes a problem where ALTER TABLE on a partitioned table could assign the same relfilenode to several different child indexes, causing very nasty catalog corruption --- in fact, attempting to DROP the partitioned table then leads not only to a database crash, but to inability to restart because the same crash will recur during WAL replay. Either of these two changes would be enough to prevent the failure, but since neither action could possibly be sane, let's put in both changes for future-proofing. In HEAD, no such bug manifests, but that's just an accidental consequence of having changed the pg_class representation of partitioned indexes to have relfilenode = 0. Both of these changes still seem like smart future-proofing. This is only a stop-gap because the code for ALTER TABLE on a partitioned table with a no-op type change still leaves a great deal to be desired. As the added regression tests show, it gets things wrong for comments on child indexes/constraints, and it is regenerating child indexes it doesn't have to. However, fixing those problems will take more work which may not get back-patched into v11. We need a fix for the corruption problem now. Per bug #15672 from Jianing Yang. Patch by me, regression test cases based on work by Amit Langote, who also did a lot of the investigative work. Discussion: https://postgr.es/m/15672-b9fa7db32698269f@postgresql.org
1 parent 53f48a2 commit 02c359e

File tree

4 files changed

+167
-3
lines changed

4 files changed

+167
-3
lines changed

‎src/backend/commands/indexcmds.c

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,6 +1001,19 @@ DefineIndex(Oid relationId,
10011001
bool found_whole_row;
10021002
ListCell *lc;
10031003

1004+
/*
1005+
* We can't use the same index name for the child index,
1006+
* so clear idxname to let the recursive invocation choose
1007+
* a new name. Likewise, the existing target relation
1008+
* field is wrong, and if indexOid or oldNode are set,
1009+
* they mustn't be applied to the child either.
1010+
*/
1011+
childStmt->idxname = NULL;
1012+
childStmt->relation = NULL;
1013+
childStmt->relationId = childRelid;
1014+
childStmt->indexOid = InvalidOid;
1015+
childStmt->oldNode = InvalidOid;
1016+
10041017
/*
10051018
* Adjust any Vars (both in expressions and in the index's
10061019
* WHERE clause) to match the partition's column numbering
@@ -1032,8 +1045,6 @@ DefineIndex(Oid relationId,
10321045
if (found_whole_row)
10331046
elog(ERROR, "cannot convert whole-row table reference");
10341047

1035-
childStmt->idxname = NULL;
1036-
childStmt->relationId = childRelid;
10371048
DefineIndex(childRelid, childStmt,
10381049
InvalidOid, /* no predefined OID */
10391050
indexRelationId, /* this is our child */

‎src/backend/commands/tablecmds.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10694,7 +10694,9 @@ TryReuseIndex(Oid oldId, IndexStmt *stmt)
1069410694
{
1069510695
Relation irel = index_open(oldId, NoLock);
1069610696

10697-
stmt->oldNode = irel->rd_node.relNode;
10697+
/* If it's a partitioned index, there is no storage to share. */
10698+
if (irel->rd_rel->relkind != RELKIND_PARTITIONED_INDEX)
10699+
stmt->oldNode = irel->rd_node.relNode;
1069810700
index_close(irel, NoLock);
1069910701
}
1070010702
}

‎src/test/regress/expected/alter_table.out

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2081,6 +2081,94 @@ Indexes:
20812081
"at_part_2_a_idx" btree (a)
20822082
"at_part_2_b_idx" btree (b)
20832083

2084+
drop table at_partitioned;
2085+
-- Alter column type when no table rewrite is required
2086+
-- Also check that comments are preserved
2087+
create table at_partitioned(id int, name varchar(64), unique (id, name))
2088+
partition by hash(id);
2089+
comment on constraint at_partitioned_id_name_key on at_partitioned is 'parent constraint';
2090+
comment on index at_partitioned_id_name_key is 'parent index';
2091+
create table at_partitioned_0 partition of at_partitioned
2092+
for values with (modulus 2, remainder 0);
2093+
comment on constraint at_partitioned_0_id_name_key on at_partitioned_0 is 'child 0 constraint';
2094+
comment on index at_partitioned_0_id_name_key is 'child 0 index';
2095+
create table at_partitioned_1 partition of at_partitioned
2096+
for values with (modulus 2, remainder 1);
2097+
comment on constraint at_partitioned_1_id_name_key on at_partitioned_1 is 'child 1 constraint';
2098+
comment on index at_partitioned_1_id_name_key is 'child 1 index';
2099+
insert into at_partitioned values(1, 'foo');
2100+
insert into at_partitioned values(3, 'bar');
2101+
create temp table old_oids as
2102+
select relname, oid as oldoid, relfilenode as oldfilenode
2103+
from pg_class where relname like 'at_partitioned%';
2104+
select relname,
2105+
c.oid = oldoid as orig_oid,
2106+
case relfilenode
2107+
when 0 then 'none'
2108+
when c.oid then 'own'
2109+
when oldfilenode then 'orig'
2110+
else 'OTHER'
2111+
end as storage,
2112+
obj_description(c.oid, 'pg_class') as desc
2113+
from pg_class c left join old_oids using (relname)
2114+
where relname like 'at_partitioned%'
2115+
order by relname;
2116+
relname | orig_oid | storage | desc
2117+
------------------------------+----------+---------+---------------
2118+
at_partitioned | t | own |
2119+
at_partitioned_0 | t | own |
2120+
at_partitioned_0_id_name_key | t | own | child 0 index
2121+
at_partitioned_1 | t | own |
2122+
at_partitioned_1_id_name_key | t | own | child 1 index
2123+
at_partitioned_id_name_key | t | own | parent index
2124+
(6 rows)
2125+
2126+
select conname, obj_description(oid, 'pg_constraint') as desc
2127+
from pg_constraint where conname like 'at_partitioned%'
2128+
order by conname;
2129+
conname | desc
2130+
------------------------------+--------------------
2131+
at_partitioned_0_id_name_key | child 0 constraint
2132+
at_partitioned_1_id_name_key | child 1 constraint
2133+
at_partitioned_id_name_key | parent constraint
2134+
(3 rows)
2135+
2136+
alter table at_partitioned alter column name type varchar(127);
2137+
-- Note: these tests currently show the wrong behavior for comments :-(
2138+
select relname,
2139+
c.oid = oldoid as orig_oid,
2140+
case relfilenode
2141+
when 0 then 'none'
2142+
when c.oid then 'own'
2143+
when oldfilenode then 'orig'
2144+
else 'OTHER'
2145+
end as storage,
2146+
obj_description(c.oid, 'pg_class') as desc
2147+
from pg_class c left join old_oids using (relname)
2148+
where relname like 'at_partitioned%'
2149+
order by relname;
2150+
relname | orig_oid | storage | desc
2151+
------------------------------+----------+---------+--------------
2152+
at_partitioned | t | own |
2153+
at_partitioned_0 | t | own |
2154+
at_partitioned_0_id_name_key | f | own | parent index
2155+
at_partitioned_1 | t | own |
2156+
at_partitioned_1_id_name_key | f | own | parent index
2157+
at_partitioned_id_name_key | f | own | parent index
2158+
(6 rows)
2159+
2160+
select conname, obj_description(oid, 'pg_constraint') as desc
2161+
from pg_constraint where conname like 'at_partitioned%'
2162+
order by conname;
2163+
conname | desc
2164+
------------------------------+-------------------
2165+
at_partitioned_0_id_name_key |
2166+
at_partitioned_1_id_name_key |
2167+
at_partitioned_id_name_key | parent constraint
2168+
(3 rows)
2169+
2170+
-- Don't remove this DROP, it exposes bug #15672
2171+
drop table at_partitioned;
20842172
-- disallow recursive containment of row types
20852173
create temp table recur1 (f1 int);
20862174
alter table recur1 add column f2 recur1; -- fails

‎src/test/regress/sql/alter_table.sql

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1393,6 +1393,69 @@ alter table at_partitioned attach partition at_part_2 for values from (1000) to
13931393
alter table at_partitioned alter column b type numeric using b::numeric;
13941394
\d at_part_1
13951395
\d at_part_2
1396+
drop table at_partitioned;
1397+
1398+
-- Alter column type when no table rewrite is required
1399+
-- Also check that comments are preserved
1400+
create table at_partitioned(id int, name varchar(64), unique (id, name))
1401+
partition by hash(id);
1402+
comment on constraint at_partitioned_id_name_key on at_partitioned is 'parent constraint';
1403+
comment on index at_partitioned_id_name_key is 'parent index';
1404+
create table at_partitioned_0 partition of at_partitioned
1405+
for values with (modulus 2, remainder 0);
1406+
comment on constraint at_partitioned_0_id_name_key on at_partitioned_0 is 'child 0 constraint';
1407+
comment on index at_partitioned_0_id_name_key is 'child 0 index';
1408+
create table at_partitioned_1 partition of at_partitioned
1409+
for values with (modulus 2, remainder 1);
1410+
comment on constraint at_partitioned_1_id_name_key on at_partitioned_1 is 'child 1 constraint';
1411+
comment on index at_partitioned_1_id_name_key is 'child 1 index';
1412+
insert into at_partitioned values(1, 'foo');
1413+
insert into at_partitioned values(3, 'bar');
1414+
1415+
create temp table old_oids as
1416+
select relname, oid as oldoid, relfilenode as oldfilenode
1417+
from pg_class where relname like 'at_partitioned%';
1418+
1419+
select relname,
1420+
c.oid = oldoid as orig_oid,
1421+
case relfilenode
1422+
when 0 then 'none'
1423+
when c.oid then 'own'
1424+
when oldfilenode then 'orig'
1425+
else 'OTHER'
1426+
end as storage,
1427+
obj_description(c.oid, 'pg_class') as desc
1428+
from pg_class c left join old_oids using (relname)
1429+
where relname like 'at_partitioned%'
1430+
order by relname;
1431+
1432+
select conname, obj_description(oid, 'pg_constraint') as desc
1433+
from pg_constraint where conname like 'at_partitioned%'
1434+
order by conname;
1435+
1436+
alter table at_partitioned alter column name type varchar(127);
1437+
1438+
-- Note: these tests currently show the wrong behavior for comments :-(
1439+
1440+
select relname,
1441+
c.oid = oldoid as orig_oid,
1442+
case relfilenode
1443+
when 0 then 'none'
1444+
when c.oid then 'own'
1445+
when oldfilenode then 'orig'
1446+
else 'OTHER'
1447+
end as storage,
1448+
obj_description(c.oid, 'pg_class') as desc
1449+
from pg_class c left join old_oids using (relname)
1450+
where relname like 'at_partitioned%'
1451+
order by relname;
1452+
1453+
select conname, obj_description(oid, 'pg_constraint') as desc
1454+
from pg_constraint where conname like 'at_partitioned%'
1455+
order by conname;
1456+
1457+
-- Don't remove this DROP, it exposes bug #15672
1458+
drop table at_partitioned;
13961459

13971460
-- disallow recursive containment of row types
13981461
create temp table recur1 (f1 int);

0 commit comments

Comments
 (0)