| From: | Nikhil Sontakke <nikhil(at)planetscale(dot)com> |
|---|---|
| To: | Manuel Reyes Bravo <manuelreyesbravo(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Andres Freund <andres(at)anarazel(dot)de> |
| Subject: | Re: Dropping a composite attribute causes data integrity violations |
| Date: | 2026-11-13 11:16:17 |
| Message-ID: | CA+UBoq0w7=Tua5oHr_ddmvScxj=AwNR48NDYN0O6n1DAheZ1eA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Manuel,
(cc Heikki, Andres for questions since changes are related to recent
CVE-2026-6460 fixes)
>
>
> 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
>
>
Thanks for the review and for the scenario script.
I've rebased onto current master, which now includes the
CVE-2026-6460 commits. That changes the picture enough that I've added
Heikki and Andres. v3 is attached.
On master without the v3 patch, all of your scenarios still go through:
N1/N2 (stored columns), S1-S6 (constants in views, CHECK, DEFAULT,
function parameter default, table row type in a view) and both
controls. The original example still ends with a table that has
duplicates under its own unique index and can't be REINDEXed.
Attribute versioning doesn't catch this, and as far as I can tell
that's by design. README.attver.md says a drop changes no version
because the dropped attribute keeps attlen/attalign, so old values stay
safe to deform.
The good news is that the stricter find_composite_type_dependencies()
from Heikki's commits (0b0f5c5af11, 882eb502595, 1ff5607e95f,
3a98781ba12 and friends) makes the gap you pointed out go away. With
the single call from ATExecDropColumn() that v2 already added, all of
N1, N2 and S1-S6 are now refused with specific errors, so I don't think
your separate patch is needed for these cases anymore, at least on master.
The code change in v3 is the same as in v2. What's new are the tests.
Heikki and Andres, I'd like your opinion on this part. Some of the
tests added with the CVE fix store a value and then drop a column of
its row type, which this v3 patch now refuses.
The README already notes that preventing unsafe deforming doesn't
guarantee stored data stays usable. This is a case where the data stays
readable but silently becomes incorrect. Importantly, it makes DROP
consistent
with ALTER ... TYPE, which is already refused in exactly these situations.
So, the open questions for Heikki and Andres are whether they are okay with
refusing the drop via this v3 patch. Plus, there are questions around
back-patching.
Regards,
Nikhil
---
Nikhil Sontakke
PlanetScale
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Refuse-to-drop-a-column-whose-row-type-is-stored-.patch | application/octet-stream | 33.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Hayato Kuroda (Fujitsu) | 2026-11-13 11:49:11 | RE: Parallel Apply |
| Previous Message | Koshi Shibagaki (Fujitsu) | 2026-11-13 11:15:33 | Re: RE: Injection point: fix race condition between attach and cleanup |