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