From: Tom Lane <tgl@sss.pgh.pa.us>
To: jian he <jian.universality@gmail.com>
cc: Peter Eisentraut <peter@eisentraut.org>, exclusion@gmail.com,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18970: Atempt to alter type of table column used in row type
 with check leads to assertion failure
In-reply-to: 
 <CACJufxEZ2DzyAKNBLqW+-2BqFAbT5Wnxkgkw6ogcbBBsndkD=Q@mail.gmail.com>
References: <18970-a7d1cfe1f8d5d8d9@postgresql.org>
 <CACJufxGYBfGbOj3x0C0U-FsMR4K-gDEyxE6hjBJ870FnkDAO4w@mail.gmail.com>
 <2482012.1751216513@sss.pgh.pa.us> <2537374.1751218360@sss.pgh.pa.us>
 <CACJufxGKJtGNRRSXfwMW9SqVOPEMdP17BJ7DsBf=tNsv9pWU9g@mail.gmail.com>
 <2682232.1751250210@sss.pgh.pa.us>
 <CACJufxH2NDhi4nTx_csqBao9i4bfxCyCXi7B8G=_XnWu8ru2TQ@mail.gmail.com>
 <2828229.1751296483@sss.pgh.pa.us>
 <CACJufxEZ2DzyAKNBLqW+-2BqFAbT5Wnxkgkw6ogcbBBsndkD=Q@mail.gmail.com>
Comments: In-reply-to jian he <jian.universality@gmail.com>
	message dated "Tue, 01 Jul 2025 11:25:20 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <3266568.1751399577.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 01 Jul 2025 15:52:57 -0400
Message-ID: <3266569.1751399577@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3266569.1751399577%40sss.pgh.pa.us>
Precedence: bulk

jian he <jian.universality@gmail.com> writes:
> On Mon, Jun 30, 2025 at 11:14=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> wr=
ote:
>> ... I think that is in fact wrong.  The implementation restriction
>> we have is that we lack code to run around and physically change
>> the stored values of columns that are not top-level table columns.
>> However, in the case of a virtual column we don't need to change
>> anything about the storage: once we've fixed the catalog metadata,
>> we're done.  So I'm not seeing the need to add all this stuff
>> other than the additional locking calls.

> CREATE TABLE t1(a int generated always as ('1') stored);
> CREATE TABLE t2(b t1 CHECK ((b).a IS NOT NULL));
> INSERT INTO t2 default values returning *;
> ERROR:  new row for relation "t2" violates check constraint "t2_b_check"
> DETAIL:  Failing row contains (null).
> INSERT INTO t2 values '(1)' returning *;

> ALTER TABLE t1 ALTER COLUMN a SET EXPRESSION AS (NULL);
> Currently we disallow this "SET EXPRESSION AS".
> I am wondering why we disallow it?

That is an interesting question.  AFAICT we treat the GENERATED
clause as being like a constraint.  We don't apply constraints
of a table to uses of its composite type, so in the above example
t2's b.a does not pay any attention to the GENERATED clause; it
just acts as an ordinary "int" column.  So it's not very clear
why we should forbid ALTER COLUMN a SET EXPRESSION because of t2.

If the idea was to leave the door open for a future change of
that behavior (that is, to start propagating the GENERATED clause to
uses of the composite type), I think that ship has already sailed.

So, rather than add a new restriction for VIRTUAL generated
columns, I'd be inclined to remove the one for STORED columns.
At least if the argument is to make them act the same.  If we're
okay with them working differently then we don't have to do
anything.

> at that time, I was mainly initrucugged by this comment about
> composite type default
> in ATRewriteTables:
>          * If we change column data types, the operation has to be propa=
gated
>          * to tables that use this table's rowtype as a column type.
>          * tab->newvals will also be non-NULL in the case where we're ad=
ding a
>          * column with a default.  We choose to forbid that case as well=
,
>          * since composite types might eventually support defaults.

That (propagating defaults to composite types) again strikes me as a
ship that sailed long ago.  Quite aside from the implementation issues
that would arise, we'd be changing longstanding behavior.

Peter, what's your opinion?

			regards, tom lane