On 25.12.23 13:10, Amul Sul wrote:
> > 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.
>
> Could we have AT_PASS_ADD_COL before AT_PASS_OLD_*? So overall it
> would be
>
> ...
> AT_PASS_ALTER_TYPE,
> AT_PASS_ADD_COL, // moved
> AT_PASS_SET_EXPRESSION, // new
> AT_PASS_OLD_INDEX,
> AT_PASS_OLD_CONSTR,
> AT_PASS_ADD_CONSTR,
> ...
>
> This appears to not break any existing tests.
>
>
> (Sorry, for the delay)
>
> Agree. I did that change in 0001 patch.
I have committed this patch set.
I couple of notes:
You had included the moving of the AT_PASS_ADD_COL enum in the first
patch. This is not a good style. Refactoring patches should not
include semantic changes. I have moved that change the final patch that
introduced the new feature.
I did not commit the 0002 patch that renamed some functions. I think
names like AlterColumn are too general, which makes this renaming
possibly counterproductive. I don't know a better name, maybe
AlterTypeOrSimilar, but that's obviously silly. I think leaving it at
AlterType isn't too bad, since most of the code is indeed for ALTER TYPE
support. We can reconsider this if we have a better idea.
In RememberAllDependentForRebuilding(), I dropped some of the additional
errors that you introduced for the AT_SetExpression cases. These didn't
seem useful. For example, it is not possible for a generated column to
depend on another generated column, so there is no point checking for
it. Also, there were no test cases that covered any of these
situations. If we do want any of these, we should have tests and
documentation for them.
For the tests that examine the EXPLAIN plans, I had to add an ANALYZE
after the SET EXPRESSION. Otherwise, I got unstable test results,
presumably because in some cases the analyze happened in the background.