Horton

Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression

Peter Eisentraut <peter@eisentraut.org>

View thread
From: Peter Eisentraut <peter@eisentraut.org>
To: Amul Sul <sulamul@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.com>, pgsql-hackers@postgresql.org
Date: 2024-01-04T18:58:27Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. ALTER TABLE command to change generation expression

  2. Refactor: separate function to find all objects depending on a column

  3. Turn AT_PASS_* macros into an enum

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.