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.
