Re: Dropping a composite attribute causes data integrity violations

Hi Nikhil,

Nikhil Sontakke <nikhil(at)planetscale(dot)com> wrote:
> The attached patch calls the existing function from the drop path, after
> the column is checked for droppability and before recursion to
> inheritance children, so both calls and every level are covered.

I reviewed v2 on master at 9e17d25e79d. It applies cleanly, builds
without warnings, and make check and contrib/test_decoding pass. The
new tests in alter_table fail on master without the tablecmds.c change,
so they do exercise the fix. Both of your cases, ALTER TYPE ... DROP
ATTRIBUTE with a column of the type and ALTER TABLE ... DROP COLUMN with
a column of the table's row type, are refused, and the stored values
stay distinct.

One gap remains: find_composite_type_dependencies() only looks for
columns of the row type, so the drop still goes through when a value of
the type is stored as a constant inside an expression, with the same
effect you describe for columns. On v2:

create type s1 as (a int, b int);
create view vs1 as select '(1,2)'::s1 = '(1,3)'::s1 as eq; -- f
alter type s1 drop attribute b; -- accepted
select eq from vs1; -- t

A CHECK constraint is worse, because the table stops accepting rows:

create type s3 as (a int, b int);
create table ts3 (i int check ('(1,2)'::s3 <> '(1,3)'::s3));
alter type s3 drop attribute b; -- accepted
insert into ts3 values (1);
ERROR: new row for relation "ts3" violates check constraint "ts3_check"

The same happens with a column DEFAULT, a function parameter default,
and with ALTER TABLE ... DROP COLUMN when the table's row type is used
the same way.

On pgsql-bugs, in a thread about a related problem [1], I posted a
patch that makes find_composite_type_dependencies() look inside stored
expressions for constants of the row type (pg_attrdef, pg_constraint,
pg_policy, pg_proc, pg_rewrite, pg_statistic_ext, pg_trigger, and index
and partition key expressions). It was written for ALTER COLUMN TYPE,
but because your patch calls the same function from the drop path, the
two compose: with both applied, all six stored-expression cases are
refused and the stored values keep their meaning, while a function that
only takes the type as a parameter, or a view that selects NULL::s1,
is still allowed. The two patches apply cleanly together, and make
check and test_decoding pass with both. Neither covers all of this on
its own.

Testing them together also showed a problem in mine rather than in
yours: for a stand-alone composite type my new messages said

cannot alter table "s1" because rule _RETURN on view vs1 stores a
constant of its row type

where the existing ones, which your patch reaches, correctly say
"cannot alter type". That was already wrong in my v1 on its own (ALTER
TYPE ... ALTER ATTRIBUTE ... TYPE shows it too), and v2, posted on that
thread, fixes it. With your v2 and mine, the drops above report
"cannot alter type".

Heikki, I've added you because d78040a469b touched this call site "to
make backpatching future patches a little easier", in case this
overlaps with what you have in mind.

The SQL for all the cases above, with controls, is attached.

[1] https://postgr.es/m/CA+bCEdDvsVApd+AADX=5uyj6Q5C3r5aruWNzOFaYqaFqLwE_5g@mail.gmail.com

Regards,
Manu