On Thu, Nov 16, 2023 at 2:50 AM Peter Eisentraut <pe...@eisentraut.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
