On Tue, Nov 28, 2023 at 5:40 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
> On 23.11.23 15:13, Amul Sul wrote: > > The exact sequencing of this seems to be tricky. It's clear that we > > need to do it earlier than at the end. I also think it should be > > strictly after AT_PASS_ALTER_TYPE so that the new expression can > refer > > to the new type of a column. It should also be after > AT_PASS_ADD_COL, > > so that the new expression can refer to any newly added column. But > > then it's after AT_PASS_OLD_INDEX and AT_PASS_OLD_CONSTR, is that a > > problem? > > > > AT_PASS_ALTER_TYPE and AT_PASS_ADD_COL cannot be together, the ALTER TYPE > > cannot see that column, I think we can adopt the samebehaviour. > > With your v5 patch, I see the following behavior: > > create table t1 (a int, b int generated always as (a + 1) stored); > alter table t1 add column c int, alter column b set expression as (a + c); > ERROR: 42703: column "c" does not exist > > I think intuitively, this ought to work. Maybe just moving the new pass > after AT_PASS_ADD_COL would do it. > > I think we can't support that (like alter type) since we need to place this new pass before AT_PASS_OLD_INDEX & AT_PASS_OLD_CONSTR to re-add indexes and constraints for the validation. While looking at this, I figured that converting the AT_PASS_* macros to > an enum would make this code simpler and easier to follow. For patches > like yours you wouldn't have to renumber the whole list. We could put > this patch before yours if people agree with it. > Ok, 0001 patch does that. > > > I tried to reuse the code by borrowing code from ALTER TYPE, see if that > > looks good to you. > > > > But I have concerns, with that code reuse where we drop and re-add the > > indexes > > and constraints which seems unnecessary for SET EXPRESSION where column > > attributes will stay the same. I don't know why ATLER TYPE does that for > > index > > since finish_heap_swap() anyway does reindexing. We could skip re-adding > > index for SET EXPRESSION which would be fine but we could not skip the > > re-addition of constraints, since rebuilding constraints for checking > might > > need a good amount of code copy especially for foreign key constraints. > > > > Please have a look at the attached version, 0001 patch does the code > > refactoring, and 0002 is the implementation, using the newly refactored > > code to > > re-add indexes and constraints for the validation. Added tests for the > same. > > This looks reasonable after a first reading. (I have found that using > git diff --patience makes the 0001 patch look more readable. Maybe > that's helpful.) Yeah, the attached version is generated with a better git-diff algorithm for the readability. Regards, Amul
v6-0002-Code-refactor-separate-function-to-find-all-depen.patch
Description: Binary data
v6-0001-Code-refactor-convert-macro-listing-to-enum.patch
Description: Binary data
v6-0003-Allow-to-change-generated-column-expression.patch
Description: Binary data
