| From: | "Alberto Piai" <alberto(dot)piai(at)gmail(dot)com> |
|---|---|
| To: | "Laurenz Albe" <laurenz(dot)albe(at)cybertec(dot)at>, "Alberto Piai" <alberto(dot)piai(at)gmail(dot)com>, <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Adding a stored generated column without long-lived locks |
| Date: | 2026-05-27 17:44:23 |
| Message-ID: | DITN9EVYH0RK.N3T595ULOV4T@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue May 26, 2026 at 5:23 PM CEST, Laurenz Albe wrote:
>
> 1) The SQL standard knows ALTER TABLE ... ADD ... GENERATED ALWAYS AS (...)
> and ALTER TABLE ... ALTER ... DROP EXPRESSION, but there is no provision
> for ALTER TABLE ... ADD GENERATED ALWAYS AS (...).
> So this patch adds non-standard syntax that may one day conflict with
> a new version of the standard. I think we can still do it, and the
> proposed syntax looks right, but I thought I should mention it.
Thank you for bringing this up. I don't have access to the standard, but
the chance of a possible conflict with future editions was at the back
of my mind. I don't see a way to exclude it completely. In a sibling
mail in this thread (you should be in CC), I have made a new iteration
on this proposal, which also tries to make the command more specific to
avoid future conflicts.
> 2) We currently have ALTER TABLE ... ALTER ... SET EXPRESSION AS (...) to
> change the generation expression of a column. This command always
> rewrites the table, according to the documentation.
> I think that if the present patch adds support to skip rewriting the table
> when a generation expression is added and the expression matches a check
> constraint, changing the generation expression should also be possible
> without a rewrite. If not, I would consider that a violation of the
> principle of least astonishment.
> Would it be difficult to extend the patch to support that?
Yes, I don't see a way to make that work. Since we're talking only about
stored values, a rewrite will always be necessary. However, using this
new command, a user could add a column with the new expression, then
atomically drop the old one and rename. All without holding onto an
AccessExclusiveLock for a long time :)
> 3) We already have a couple of tricks to avoid blocking for a long time:
>
> - ALTER TABLE ... ALTER ... SET NOT NULL can skip the table scan if there
> is a check constraint that makes sure that the column is NOT NULL
>
> - ALTER TABLE ... ATTACH PARTITION can skip the scan of the new partition
> if there is a check constraint matching the partition constraint
>
> It would be great to document these little tricks in the documentation,
> probably on the ALTER TABLE page. This is not necessarily the job of
> this patch, but it would also not be off-topic for the patch.
The SET NOT NULL one and the ATTACH PARTITION one are documented in the
section specific to the command. However
or, if an equivalent index already exists, it will be attached to the
target table's index, as if ALTER INDEX ATTACH PARTITION had been
executed
is not very explicit about the advantages this has for online
migrations.
In the NOTES section of the ALTER TABLE page, there is a paragraph about
NOT VALID / VALIDATE, which is another operation in the same spirit as
this.
Maybe we could group them all in a new section dedicated to online
schema migrations?
(However, even if it's definitely on-topic with this patch, I would work
on this in a separate patch / email thread.)
> Missing parts:
>
> - There is no documentation. At least ALTER TABLE needs a description of the
> new syntax, and would ideally mention the trick with the check constraint.
Yes, I will work on this next. I also believe it's a great way to show a
feature, even early during development. I just wanted to avoid doing it
_too_ early, before having had any feedback about the idea.
> - There should be support for command line completion for the new syntax.
Great idea, I'll add this too.
> Bugs:
>
> - The patch doesn't test if the column is an identity column:
>
> CREATE TABLE tab (id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY);
> INSERT INTO tab VALUES (DEFAULT);
> ALTER TABLE tab ALTER id ADD GENERATED ALWAYS AS (1) STORED;
>
> The ALTER TABLE should fail, but doesn't.
>
> - Strange behavior with sequences owned by the column:
>
> CREATE TABLE tab (id bigserial);
> INSERT INTO tab VALUES (DEFAULT);
> ALTER TABLE tab ALTER id ADD GENERATED ALWAYS AS (1) STORED;
> \ds tab_id_seq
> List of sequences
> Schema | Name | Type | Owner
> --------+------------+----------+----------
> public | tab_id_seq | sequence | postgres
> (1 row)
>
> I think that any sequence owned by the column should be dropped.
> Alternatively, you could throw an error.
Thanks for testing this!
I have reused RememberAllDependentForRebuilding() which does some
validation, but was originally meant for ALTER COLUMN TYPE. I will add
checks and tests for these cases, but to be consistent with how the
other dependencies are handled, I think it's better to throw an error
here (this is what happens for example if trying to ALTER TYPE of a
column used by a function).
>
> - Incorrect handling of NULL values:
See sibling mail, in the next iteration the constraint will have to use
IS NOT DISTINCT FROM. I think that should cover all cases.
>
> Comments on the code:
>
>> --- a/src/backend/commands/tablecmds.c
>> +++ b/src/backend/commands/tablecmds.c
>> @@ -5093,6 +5102,13 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
>> ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
>> pass = AT_PASS_SET_EXPRESSION;
>> break;
>> + case AT_AddGeneratedAsExprStored:
>
> You should add a comment, same as for the other branches.
>
>> @@ -6695,6 +6717,8 @@ alter_table_type_to_string(AlterTableType cmdtype)
>> return "ALTER COLUMN ... SET NOT NULL";
>> case AT_SetExpression:
>> return "ALTER COLUMN ... SET EXPRESSION";
>> + case AT_AddGeneratedAsExprStored:
>> + return "ALTER COLUMN ... ADD GENERATED ALWAYS AS (...) STORED";
>
> Keep it short, like "ALTER COLUMN ... ADD GENERATED".
>
>> --- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
>> +++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
>> @@ -129,6 +129,9 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
>> case AT_SetNotNull:
>> strtype = "SET NOT NULL";
>> break;
>> + case AT_AddGeneratedAsExprStored:
>> + strtype = "ADD GENERATED ALWAYS AS (...) STORED";
>> + break;
>
> I suggest "ALTER COLUMN ADD GENERATED ALWAYS AS", but I won't insist.
Agreed, will fix all these in the next version of the patch.
Thank you again for the review!
Best regards,
Alberto
--
Alberto Piai
Sensational AG
Zürich, Switzerland
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Alexandra Wang | 2026-05-27 17:49:44 | Re: Is there value in having optimizer stats for joins/foreignkeys? |
| Previous Message | Nathan Bossart | 2026-05-27 17:43:58 | Re: remove pg_spin_delay() from atomics code |