On Mon, Nov 13, 2023 at 1:40 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
> On 09.11.23 13:00, Amul Sul wrote: > > On Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <pe...@eisentraut.org > > <mailto:pe...@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 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-6448986c3...@postgresfriends.org
