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