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

On 09.11.23 13:00, Amul Sul wrote:
> On Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <peter@eisentraut.org 
> <mailto:peter@eisentraut.org>> wrote:
> 
>     On 25.10.23 08:12, Amul Sul wrote:
>      > Here is the rebase version for the latest master head(673a17e3120).
>      >
>      > I haven't done any other changes related to the ON UPDATE trigger
>     since that
>      > seems non-trivial; need a bit of work to add trigger support in
>      > ATRewriteTable().
>      > Also, I am not sure yet, if we were doing these changes, and the
>     correct
>      > direction
>      > for that.
> 
>     I did some detailed testing on Db2, Oracle, and MariaDB (the three
>     existing implementations of this feature that I'm tracking), and
>     none of
>     them fire any row or statement triggers when the respective
>     statement to
>     alter the generation expression is run.  So I'm withdrawing my comment
>     that this should fire triggers.  (I suppose event triggers are
>     available
>     if anyone really needs the functionality.)
> 
> 
> Thank you for the confirmation.
> 
> Here is the updated version patch. Did minor changes to documents and tests.

I don't like the renaming in the 0001 patch.  I think it would be better 
to keep the two subcommands (DROP and SET) separate.  There is some 
overlap now, but for example I'm thinking about virtual generated 
columns, then there will be even more conditionals in there.  Let's keep 
it separate for clarity.

Also, it seems to me that the SET EXPRESSION variant should just do an 
update of the catalog table instead of a drop and re-insert.

The documentation needs some improvements:

+    ALTER [ COLUMN ] <replaceable 
class="parameter">column_name</replaceable> SET EXPRESSION <replaceable 
class="parameter">expression</replaceable> STORED

If we're going to follow the Db2 syntax, there should be an "AS" after 
EXPRESSION.  And the implemented syntax requires parentheses, so they 
should appear in the documentation.

Also, the keyword STORED shouldn't be there.  (The same command should 
be applicable to virtual generated columns in the future.)

There should be separate <varlistentry>s for SET and DROP in 
alter_table.sgml.

The functionality looks ok otherwise.