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

On Thu, Nov 16, 2023 at 7:05 PM Amul Sul <sulamul@gmail.com> wrote:

> On Thu, Nov 16, 2023 at 2:50 AM Peter Eisentraut <peter@eisentraut.org>
> wrote:
>
>> On 15.11.23 13:26, Amul Sul wrote:
>> >     Question: Why are you using AT_PASS_ADD_OTHERCONSTR?  I don't know
>> if
>> >     it's right or wrong, but if you have a specific reason, it would be
>> >     good
>> >     to know.
>> >
>> > I referred to ALTER COLUMN DEFAULT and used that.
>>
>> Hmm, I'm not sure if that is a good comparison.  For ALTER TABLE, SET
>> DEFAULT is just a catalog manipulation, it doesn't change any data, so
>> it's pretty easy.  SET EXPRESSION changes data, which other phases might
>> want to inspect?  For example, if you do SET EXPRESSION and add a
>> constraint in the same ALTER TABLE statement, do those run in the
>> correct order?
>>
>
> I think, you are correct, but currently AT_PASS_ADD_OTHERCONSTR is for
> AT_CookedColumnDefault, AT_ColumnDefault, and AT_AddIdentity.
> AT_CookedColumnDefault is only supported for CREATE TABLE.
> AT_ColumnDefault
> and AT_AddIdentity will be having errors while operating on the generated
> column. So
> that anomaly does not exist, but could be in future addition. I think it
> is better to
> use AT_PASS_MISC to keep this operation at last.
>
> While testing this, I found a serious problem with the patch that CHECK and
> FOREIGN KEY constraint check does not happens at rewrite, see this:
>
> create table a (y int primary key);
> insert into a values(1),(2);
> create table b (x int, y int generated always as(x) stored, foreign
> key(y) references a(y));
> insert into b values(1),(2);
> insert into b values(3);        <------ an error, expected one
>
> alter table b alter column y set expression as (x*100);  <------ no
> error, NOT expected
>
> select * from b;
>  x |  y
> ---+-----
>  1 | 100
>  2 | 200
> (2 rows)
>
> Also,
>
> delete from a;               <------ no error, NOT expected.
> select * from a;
>  y
> ---
> (0 rows)
>
> Shouldn't that have been handled by the ATRewriteTables() facility
> implicitly
> like NOT NULL constraints?  Or should we prepare a list of CHECK and FK
> constraints and pass it through tab->constraints?
>

To fix this we should be doing something like ALTER COLUMN TYPE and the pass
should be AT_PASS_ALTER_TYPE (rename it or invent a new one near to that) so
that in ATRewriteCatalogs(), we would execute ATPostAlterTypeCleanup().

I simply tried that by doing blind copy of code from
ATExecAlterColumnType() in
0002 patch.  We don't really need to do all the stuff such as re-adding
indexes, constraints etc, but I am out of time for today to figure out the
optimum code and I will be away from work in the first half of the coming
week and the week after that. Therefore, I thought of sharing an approach to
get comments/thoughts on the direction, thanks.

Regards,
Amul