Обсуждение: BUG #17872: Dropping an attribute of a composite type breaks indexes over the type silently

The following bug has been logged on the website:

Bug reference:      17872
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

The following script:
CREATE TYPE ctype AS (i int, j int);
CREATE TABLE ctbl(a int, cf ctype);
CREATE UNIQUE INDEX ctbl_idx ON ctbl(cf); 
INSERT INTO ctbl VALUES (1, '(1, 2)'::ctype), (2, '(1, 1)'::ctype);
ALTER TYPE ctype DROP ATTRIBUTE j;

Results in the UNIQUE constraint broken:
SELECT ctid, * FROM ctbl;
 ctid  | a | cf  
-------+---+-----
 (0,1) | 1 | (1)
 (0,2) | 2 | (1)

\d+ ctbl
                                           Table "public.ctbl"
 Column |  Type   | Collation | Nullable | Default | Storage  | Compression
| Stats target | Description 
--------+---------+-----------+----------+---------+----------+-------------+--------------+-------------
 a      | integer |           |          |         | plain    |
|              | 
 cf     | ctype   |           |          |         | extended |
|              | 
Indexes:
    "ctbl_idx" UNIQUE, btree (cf)
Access method: heap

And the index corruption detected by amcheck:
CREATE EXTENSION amcheck;
SELECT bt_index_parent_check(oid, true, true) FROM pg_class
WHERE relname = 'ctbl_idx';
ERROR:  could not find tuple using search from root page in index
"ctbl_idx"
DETAIL:  Index tid=(1,1) points to heap tid=(0,2) page lsn=0/187BF80.

(Interestingly enough, amcheck doesn't detect a corruption if
VALUES (1, '(1, 1)'::ctype), (2, '(1, 2)'::ctype) are inserted.)

The same is observed with a composite type representing a table row,
for example:
CREATE TABLE ijtbl(i int, j int);
CREATE TABLE ttbl(a int, cf ijtbl);
CREATE UNIQUE INDEX ttbl_idx ON ttbl(cf); 
INSERT INTO ttbl VALUES (1, '(1, 2)'::ijtbl), (2, '(1, 1)'::ijtbl);
ALTER TABLE ijtbl DROP COLUMN j;

But if an index created over a set of ordinary columns, it is removed
automatically when a column in the set is dropped.
CREATE TABLE tbl(a int, i int, j int);
CREATE UNIQUE INDEX tbl_idx ON tbl(i, j); 
INSERT INTO tbl VALUES (1, 1, 2), (2, 1, 1);
\d+ tbl
                                           Table "public.tbl"
 Column |  Type   | Collation | Nullable | Default | Storage | Compression |
Stats target | Description 
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | integer |           |          |         | plain   |             |
             | 
 i      | integer |           |          |         | plain   |             |
             | 
 j      | integer |           |          |         | plain   |             |
             | 
Indexes:
    "tbl_idx" UNIQUE, btree (i, j)
Access method: heap

ALTER TABLE tbl DROP COLUMN j;
\d+ tbl
                                           Table "public.tbl"
 Column |  Type   | Collation | Nullable | Default | Storage | Compression |
Stats target | Description 
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | integer |           |          |         | plain   |             |
             | 
 i      | integer |           |          |         | plain   |             |
             | 
Access method: heap

As I can see, in this case pg_depend contains dependencies of the index
on specific table columns, but for composite types there exists only a
dependency on a composite type as a whole.

PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> CREATE TYPE ctype AS (i int, j int);
> CREATE TABLE ctbl(a int, cf ctype);
> CREATE UNIQUE INDEX ctbl_idx ON ctbl(cf); 
> INSERT INTO ctbl VALUES (1, '(1, 2)'::ctype), (2, '(1, 1)'::ctype);
> ALTER TYPE ctype DROP ATTRIBUTE j;

> Results in the UNIQUE constraint broken:
> SELECT ctid, * FROM ctbl;
>  ctid  | a | cf  
> -------+---+-----
>  (0,1) | 1 | (1)
>  (0,2) | 2 | (1)

Meh.  I'm happy to classify this as "so don't do that".
The cost of having ALTER TYPE detect such situations seems
vastly out of proportion to the value (and I think it'd
be impossible to prevent race conditions in the detection
anyway).

            regards, tom lane

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> The following script:
>> CREATE TYPE ctype AS (i int, j int);
>> CREATE TABLE ctbl(a int, cf ctype);
>> CREATE UNIQUE INDEX ctbl_idx ON ctbl(cf); 
>> INSERT INTO ctbl VALUES (1, '(1, 2)'::ctype), (2, '(1, 1)'::ctype);
>> ALTER TYPE ctype DROP ATTRIBUTE j;

>> Results in the UNIQUE constraint broken:
>> SELECT ctid, * FROM ctbl;
>> ctid  | a | cf  
>> -------+---+-----
>> (0,1) | 1 | (1)
>> (0,2) | 2 | (1)

> Meh.  I'm happy to classify this as "so don't do that".

On the other hand, this seems considerably more troubling:

regression=# CREATE TYPE ctype AS (i int, j int);
CREATE TYPE
regression=# CREATE TABLE ctbl(a int, b int);
CREATE TABLE
regression=# CREATE UNIQUE INDEX ctbl_idx ON ctbl((row(a,b)::ctype));
CREATE INDEX
regression=# ALTER TYPE ctype ALTER ATTRIBUTE j type numeric;
ALTER TYPE

If we had any data in the index, it'd now be completely broken.

This should be forbidden, but find_composite_type_dependencies
has no idea whatever that indexes might contain expression
columns of the target datatype.  It does find a pg_depend
entry showing the index as dependent on the composite type,
but it ignores it because objsubid = 0 --- and would ignore it
also because of the relkind.

I think what we should do, instead of just ignoring objsubid = 0,
is to look through the index's columns and see if any have
atttypid equal to the target type.  If not, then the composite
type is used in an expression but not stored on disk, so it's
just as safe (or not) as a reference in a view.

            regards, tom lane

Hi Tom,

27.03.2023 19:46, Tom Lane wrote:
> I wrote:
>> PG Bug reporting form <noreply@postgresql.org> writes:
>>> The following script:
>>> CREATE TYPE ctype AS (i int, j int);
>>> CREATE TABLE ctbl(a int, cf ctype);
>>> CREATE UNIQUE INDEX ctbl_idx ON ctbl(cf);
>>> INSERT INTO ctbl VALUES (1, '(1, 2)'::ctype), (2, '(1, 1)'::ctype);
>>> ALTER TYPE ctype DROP ATTRIBUTE j;
>>> Results in the UNIQUE constraint broken:
>>>
>> Meh.  I'm happy to classify this as "so don't do that".
> On the other hand, this seems considerably more troubling:
>
> regression=# CREATE TYPE ctype AS (i int, j int);
> CREATE TYPE
> regression=# CREATE TABLE ctbl(a int, b int);
> CREATE TABLE
> regression=# CREATE UNIQUE INDEX ctbl_idx ON ctbl((row(a,b)::ctype));
> CREATE INDEX
> regression=# ALTER TYPE ctype ALTER ATTRIBUTE j type numeric;
> ALTER TYPE
>
> If we had any data in the index, it'd now be completely broken.

Yes, ALTERing the attribute/column leads to much worse things. In particular,
I've seen a server crash and the "compressed pglz data is corrupt" error.
(I can present the concrete queries if they can be of interest to you
(maybe for including in regression tests).)

As to the race condition possibility, I'm not sure that this consideration
should be applicable only to this kind of DDL. (I've got a collection of
crashes caused by race conditions in the existing code (#17182 and alike).
I had plans to discuss this issue separately.)

> This should be forbidden, but find_composite_type_dependencies
> has no idea whatever that indexes might contain expression
> columns of the target datatype.  It does find a pg_depend
> entry showing the index as dependent on the composite type,
> but it ignores it because objsubid = 0 --- and would ignore it
> also because of the relkind.
>
> I think what we should do, instead of just ignoring objsubid = 0,
> is to look through the index's columns and see if any have
> atttypid equal to the target type.  If not, then the composite
> type is used in an expression but not stored on disk, so it's
> just as safe (or not) as a reference in a view.

Yes, I think something like that can resolve the issue.
But I would also note that the problem is not with indexes only, but also
with "... partition by list(comp_type_value)", for example.

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:
> Yes, I think something like that can resolve the issue.
> But I would also note that the problem is not with indexes only, but also
> with "... partition by list(comp_type_value)", for example.

Hmm ... really?  I'd just concluded that a partitioned table is okay
as long as it doesn't yet have any partitions.  Even if the modified
type is a partitioning column, there's no structure yet that could
depend on the contents of the type.  (If it does have partitions,
we'll fail when we get to one of those.)

[ thinks some more... ] I guess there's the corner case where we
replace, say, a hashable type with a non-hashable one and thereby
break decisions about whether PARTITION BY HASH is allowable.
That's kind of a stretch.  But find_composite_type_dependencies
currently rejects partitioned tables, so we're not taking away any
functionality if we continue to forbid that.

            regards, tom lane

27.03.2023 21:20, Tom Lane wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
>> Yes, I think something like that can resolve the issue.
>> But I would also note that the problem is not with indexes only, but also
>> with "... partition by list(comp_type_value)", for example.
> Hmm ... really?  I'd just concluded that a partitioned table is okay
> as long as it doesn't yet have any partitions.  Even if the modified
> type is a partitioning column, there's no structure yet that could
> depend on the contents of the type.  (If it does have partitions,
> we'll fail when we get to one of those.)

The following query leads to a failure on showing a partition definition:
CREATE TABLE tbl(a int, b int) PARTITION BY LIST ((tbl));
CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
ALTER TABLE tbl ALTER COLUMN a TYPE char(5);
\d+ tbl
(The effect depends on the values specified.)
It's not exactly the dependency issue but still is related to altering
a composite type.

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:
> 27.03.2023 21:20, Tom Lane wrote:
>> Hmm ... really?  I'd just concluded that a partitioned table is okay
>> as long as it doesn't yet have any partitions.  Even if the modified
>> type is a partitioning column, there's no structure yet that could
>> depend on the contents of the type.  (If it does have partitions,
>> we'll fail when we get to one of those.)

> The following query leads to a failure on showing a partition definition:
> CREATE TABLE tbl(a int, b int) PARTITION BY LIST ((tbl));
> CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
> ALTER TABLE tbl ALTER COLUMN a TYPE char(5);

Sure, but there you already have a partition.  If you only had "tbl"
then there would be no stored partition bounds.

            regards, tom lane

Alexander Lakhin <exclusion@gmail.com> writes:

27.03.2023 21:20, Tom Lane wrote:

Hmm ... really?  I'd just concluded that a partitioned table is okay
as long as it doesn't yet have any partitions.  Even if the modified
type is a partitioning column, there's no structure yet that could
depend on the contents of the type.  (If it does have partitions,
we'll fail when we get to one of those.)

The following query leads to a failure on showing a partition definition:
CREATE TABLE tbl(a int, b int) PARTITION BY LIST ((tbl));
CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
ALTER TABLE tbl ALTER COLUMN a TYPE char(5);

Sure, but there you already have a partition.  If you only had "tbl"
then there would be no stored partition bounds.

Yes, that's a separate case when a value of the composite type "tbl" stored
in relpartbound for "tblp", but there is no dependency of "tbl"/"tblp" on the
type "tbl".

The similar situation:
CREATE TYPE ctype AS (a int, b int);
CREATE TABLE tbl(x ctype) PARTITION BY LIST (x);
CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
ALTER TYPE ctype ALTER ATTRIBUTE b TYPE char(5);
is prevented with
ERROR:  cannot alter type "ctype" because column "tbl.x" uses it
because there is dependency {"tbl", objsubid = 1} on the "ctype".

Regarding dropping attributes/columns, I've added the
find_composite_type_dependencies() call inside ATPrepDropColumn() and found
that it breaks cases that were valid before (according to regression tests). E.g.:
 CREATE TYPE test_type3 AS (a int);
 CREATE TABLE test_tbl3 (c) AS SELECT '(1)'::test_type3;
 ALTER TYPE test_type3 DROP ATTRIBUTE a, ADD ATTRIBUTE b int;
+ERROR:  cannot alter type "test_type3" because column "test_tbl3.c" uses it

So I agree, that disabling the drop operation when a composite type has
dependencies is not a harmless change. On the other hand, the code might be
not ready to deal with the uniqueness assumption violated.
For example:
CREATE TYPE ctype AS (a int, b int);
CREATE TABLE lp (t ctype) PARTITION BY RANGE (t);
CREATE TABLE lp_1_0 PARTITION OF lp FOR VALUES FROM (ROW(1, 01)::ctype) TO (ROW(1, 10)::ctype);
CREATE TABLE lp_1_1 PARTITION OF lp FOR VALUES FROM (ROW(1, 11)::ctype) TO (ROW(1, 20)::ctype);
ALTER TYPE ctype DROP ATTRIBUTE b;
INSERT INTO lp values(ROW(1)::ctype);
leads to an assertion failure:
...
#5  0x000055d3131e314f in ExceptionalCondition (
    conditionName=conditionName@entry=0x55d313357b85 "next_index == nparts",
    fileName=fileName@entry=0x55d313357887 "partbounds.c", lineNumber=lineNumber@entry=884) at assert.c:66
#6  0x000055d312fe2e2b in create_range_bounds (boundspecs=boundspecs@entry=0x55d314e4b028,
    nparts=nparts@entry=2, key=key@entry=0x55d314ed7170, mapping=mapping@entry=0x7fffa34bc438)
    at partbounds.c:884
#7  0x000055d312fe46c7 in partition_bounds_create (boundspecs=boundspecs@entry=0x55d314e4b028, nparts=2,
    key=key@entry=0x55d314ed7170, mapping=mapping@entry=0x7fffa34bc438) at partbounds.c:336
#8  0x000055d312fe6a2c in RelationBuildPartitionDesc (rel=rel@entry=0x7f609da02920,
    omit_detached=omit_detached@entry=true) at partdesc.c:271
#9  0x000055d312fe6c99 in RelationGetPartitionDesc (rel=rel@entry=0x7f609da02920,
    omit_detached=<optimized out>) at partdesc.c:110
...
Maybe that exact Assert could be replaced with just elog(ERROR), but I'm
afraid it's not the only such place that exists today and will appear
tomorrow. At least without a comprehensive testing this way seems dangerous
to me. Though maybe postgres should be robust enough to not crash in such
situations (caused by a composite type modification or by something else).

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:
> So I agree, that disabling the drop operation when a composite type has
> dependencies is not a harmless change. On the other hand, the code might be
> not ready to deal with the uniqueness assumption violated.

Well, corrupt indexes are a fact of life and probably always will be,
because collations are such an unpredictable mess.  So if we have anyplace
that is straight out Assert'ing that uniqueness holds, I'd say that that
needs reconsideration.  We need to try to turn it into a helpful error
message.

> #5  0x000055d3131e314f in ExceptionalCondition (
>      conditionName=conditionName@entry=0x55d313357b85 "next_index == nparts",
>      fileName=fileName@entry=0x55d313357887 "partbounds.c", lineNumber=lineNumber@entry=884) at assert.c:66

Hmm.  I did not trace this down, but it doesn't look like it's exactly
connected to corrupt indexes.

            regards, tom lane