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

On Mon, Nov 13, 2023 at 1:40 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 09.11.23 13:00, Amul Sul wrote:
> > On Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <peter(at)eisentraut(dot)org
> > <mailto:peter(at)eisentraut(dot)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.
>

Understood. Will do the same.

> 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.
>

I am not sure if that is sufficient; we need to get rid of the dependencies
of
existing expressions on other columns and/or objects that need to be
removed.
The drop and re-insert does that easily.

> 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.)
>

I have omitted "AS" intentionally, to keep syntax similar to our existing
ALTER COLUMN ... SET DEFAULT <a_expr>. Let me know if you want
me to add that.

The STORED suggested by Vik[1]. I think we could skip that if there is no
need
to differentiate between stored and virtual columns at ALTER.

Regards,
Amul

1] postgr.es/m/d15cf691-55d0-e405-44ec-6448986c3276@postgresfriends.org