Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression

On Thu, Nov 16, 2023 at 2:50 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 15.11.23 13:26, Amul Sul wrote:
> > Question: Why are you using AT_PASS_ADD_OTHERCONSTR? I don't know if
> > it's right or wrong, but if you have a specific reason, it would be
> > good
> > to know.
> >
> > I referred to ALTER COLUMN DEFAULT and used that.
>
> Hmm, I'm not sure if that is a good comparison. For ALTER TABLE, SET
> DEFAULT is just a catalog manipulation, it doesn't change any data, so
> it's pretty easy. SET EXPRESSION changes data, which other phases might
> want to inspect? For example, if you do SET EXPRESSION and add a
> constraint in the same ALTER TABLE statement, do those run in the
> correct order?
>

I think, you are correct, but currently AT_PASS_ADD_OTHERCONSTR is for
AT_CookedColumnDefault, AT_ColumnDefault, and AT_AddIdentity.
AT_CookedColumnDefault is only supported for CREATE TABLE. AT_ColumnDefault
and AT_AddIdentity will be having errors while operating on the generated
column. So
that anomaly does not exist, but could be in future addition. I think it is
better to
use AT_PASS_MISC to keep this operation at last.

While testing this, I found a serious problem with the patch that CHECK and
FOREIGN KEY constraint check does not happens at rewrite, see this:

create table a (y int primary key);
insert into a values(1),(2);
create table b (x int, y int generated always as(x) stored, foreign key(y)
references a(y));
insert into b values(1),(2);
insert into b values(3); <------ an error, expected one

alter table b alter column y set expression as (x*100); <------ no error,
NOT expected

select * from b;
x | y
---+-----
1 | 100
2 | 200
(2 rows)

Also,

delete from a; <------ no error, NOT expected.
select * from a;
y
---
(0 rows)

Shouldn't that have been handled by the ATRewriteTables() facility
implicitly
like NOT NULL constraints? Or should we prepare a list of CHECK and FK
constraints and pass it through tab->constraints?

> > Tiny comment: The error message in ATExecSetExpression() does not
> need
> > to mention "stored", since it would be also applicable to virtual
> > generated columns in the future.
> >
> > I had to have the same thought, but later decided when we do that
> > virtual column thing, we could simply change that. I am fine to do that
> > change
> > now as well, let me know your thought.
>
> Not a big deal, but I would change it now.
>
> Another small thing I found: In ATExecColumnDefault(), there is an
> errhint() that suggests DROP EXPRESSION instead of DROP DEFAULT. You
> could now add another hint that suggests SET EXPRESSION instead of SET
> DEFAULT.
>

Ok.

Regards,
Amul Sul