On Thu, Nov 16, 2023 at 7:05 PM Amul Sul <sula...@gmail.com> wrote:
> 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? > To fix this we should be doing something like ALTER COLUMN TYPE and the pass should be AT_PASS_ALTER_TYPE (rename it or invent a new one near to that) so that in ATRewriteCatalogs(), we would execute ATPostAlterTypeCleanup(). I simply tried that by doing blind copy of code from ATExecAlterColumnType() in 0002 patch. We don't really need to do all the stuff such as re-adding indexes, constraints etc, but I am out of time for today to figure out the optimum code and I will be away from work in the first half of the coming week and the week after that. Therefore, I thought of sharing an approach to get comments/thoughts on the direction, thanks. Regards, Amul
v4-0001-Allow-to-change-generated-column-expression.patch
Description: Binary data
v4-0002-POC-FK-and-CHECK-constraint-check.patch
Description: Binary data
