Subject: Re: Adding a stored generated column without long-lived locks

From: "Alberto Piai" <[email-masked]>
Date: Fri, 10 Jul 2026 02:10:57 +0200

Hi Laurenz,

thanks a lot for another thoughtful review, I appreciate it a lot. Let
me address the most important point first.

On Tue Jul 7, 2026 at 8:16 PM CEST, Laurenz Albe wrote:
> I don't usually mention that, but since you are a new contributor and explicitly
> asked several people for a review (which is fine!): it is expected that you also
> review other's patches in the commitfest.

You are right to bring that up, it is clearly spelled out in the
developer docs on the wiki and I agree that it's the best (the only?)
way to keep a project going with this volume of contributions.

Until now, I have dedicated all my bandwidth to learning as much as I
could to get this patch to a presentable state.  From now on, I'll do my
best to get involved with patch reviews!

>> In the attached v5 patch I've implemented this design, and went one step
>> further (let me know what you think). While discussing this with my
>> colleagues at work, the question came up (thanks, Philip!): now that we
>> mention the constraint explicitly, what's the point of repeating the
>> expression too? The constraint already defines an equality to an
>> expression. I think this is a very good point, and it removes one
>> further way in which the operation could fail,  so I went ahead and
>> changed the command to not mention the expression. It takes the
>> expression defined in the constraint and uses _that_ as the generator
>> expression of the column.
>
> I agree with that idea, it shortend the syntax and leaves less room for
> mistakes.
>
> The syntax you ended up with (ADD GENERATED ALWAYS STORED USING CONSTRAINT)
> is ugly as hell.  I see your point in having ALWAYS and STORED, but perhaps
> ADD GENERATED ALWAYS USING CONSTRAINT ... STORED would be better, as it is
> syntactically more like GENERATED ALWAYS AS (...) STORED, which would make
> it easier to remember.
>
> Or perhaps ADD GENERATED USING CONSTRAINT would be enough, since ALWAYS and
> STORED are the only possible choice anyway.  I am a bit uncertain on what
> is best here.

Your point about moving STORED at the end is good, I feel bad about
dropping it completely though, because in the ADD COLUMN subcommand the
default is VIRTUAL when omitted. Maybe we could drop the ALWAYS, since
that is only relevant when talking about identity columns. That would
result in:

... ADD GENERATED USING CONSTRAINT constr_name STORED

Another option would be to be consistent with SET|DROP EXPRESSION:

... ADD EXPRESSION USING CONSTRAINT constr_name STORED

I would not omit STORED here either, because SET|DROP EXPRESSION act on
what's already a generated column (so they don't need to specify the
type at all), but this one does need to know.

The ALTER TABLE page of the manual already groups these 3 together:

    ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
    SET GENERATED { ALWAYS | BY DEFAULT }
    DROP IDENTITY [ IF EXISTS ]

      These forms change whether a column is an identity column...

Using EXPRESSION for this new command would result in a similar group:

    ADD EXPRESSION USING CONSTRAINT constr_name STORED
    SET EXPRESSION AS
    DROP EXPRESSION [ IF EXISTS ]

the three forms change whether a column is a generated column or change
the generator expression. The first and the third only work with stored
generated columns.

But at this point we are just debating whether to say GENERATED or
EXPRESSION. Personally I find both OK with a slight preference for the
latter.

>> > I feel that automatically dropping the constraint is a bit too much
>> > black magic, but it is more a feeling than a conviction.
>>
>> I don't have a strong opinion on whether to cleanup or not, I'll gladly
>> take your input. This version of the patch does not drop the constraint
>> anymore.
>
> I like it that way, because ALTER TABLE ATTACH PARTITION and ALTER TABLE
> ALTER COLUMN SET NOT NULL don't drop the constraint they use either.
> But the case is not exactly the same, so I won't insist.

It's a valid point, I would leave it like this if there are no further
objections.

All your feedback about docs, error messages and comment is valid too, I
will gladly adapt in the next iteration.

> There is a weird asymmetry in that the order in which you write the check
> constraint matters:
>
>   CREATE TABLE tab (a integer PRIMARY KEY, b integer NOT NULL);
>   INSERT INTO tab VALUES (1, 2);
>
>   -- this fails
>   ALTER TABLE tab ADD CONSTRAINT c CHECK (2 * a = b);
>   ALTER TABLE tab ALTER b ADD GENERATED ALWAYS STORED USING CONSTRAINT c;
>   ERROR:  cannot convert a column into a stored generated column without a constraint to prove that the values are consistent
>   DETAIL:  could not find a valid constraint "c" CHECK ("b" = expr) or CHECK ("b" IS NOT DISTINCT FROM (expr))
>
>   -- but this works
>   ALTER TABLE tab DROP CONSTRAINT c;
>   ALTER TABLE tab ADD CONSTRAINT c CHECK (b = 2 * a);
>   ALTER TABLE tab ALTER b ADD GENERATED ALWAYS STORED USING CONSTRAINT c;
>
> I think that both variants should be accepted, but I am not certain.

My original approach here was to be as literal as possible with the
shape of the constraint required to perform this operation. It is after
all an extremely ad-hoc constraint, added just for the purpose of doing
this online migration. But just checking both directions for the
equalities seems simple enough, I'll try to do this in the next version
of the patch.

>> +               }
>> +           case AttrDefaultRelationId:
>> +               {
>> +                   ObjectAddress col = GetAttrDefaultColumnAddress(foundObject.objectId);
>> +
>> +                   if (col.objectId == RelationGetRelid(rel) &&
>> +                       col.objectSubId == attnum)
>> +                   {
>> +                       /*
>> +                        * Ignore the column's own default expression. We
>> +                        * handle sequences above, and for a column which is
>> +                        * already a generated column we should never get
>> +                        * here.
>> +                        */
>
> It's not strictly required, but it would be great if you could run pgindent
> to get comments and other parts of the code formatted properly.

This is strange, I did run pgindent. Which part seems formatted
incorrectly to you? I'll try to see if we're hitting some edge case that
makes pgindent skip the code.

>> + * If a valid constraint is found, this returns both the Oid of the constraint
>> + * and the unpacked expression.
>> + */
>> +static Node *
>> +findUsableConstraintForAddExprStored(Relation rel, AttrNumber attnum,
>> +                                    bool attisnotnull,
>> +                                    const char *conname)
>
> I see that it returns a Node, not an Oid and an expression.
> If the Oid of the constraint is actually returned somewhere inside the
> "Node", the comment should be more specific about it.

Oops, this is a leftover from when I still returned the Oid of the
constraint to be able to drop it later. Will fix.

Thanks again for your feedback!

Regards,

Alberto

-- 
Alberto Piai
Sensational AG
Zürich, Switzerland

---

From: Laurenz Albe <[email-masked]>
Date: Fri, 10 Jul 2026 08:51:58 +0200

> EXPRESSION. Personally I find both OK with a slight preference for the

I take your point about keeping the STORED.  If VIRTUAL is the default
when creating a generated column, then keeping the explicit STORED makes
sense.

About GENERATED vs. EXPRESSION: my feelings go the other way.  If I read

  ALTER TABLE tab ALTER col ADD EXPRESSION ...

then it is not clear to me *what* expression is added.  After all, there
are not only generation expressions, but also column default expressions,
and I think it is good to make clear that this is about generated columns.

I think I understand your preference: after all, we are not ADDing a
GENERATED column, but only an EXPRESSION that turns a regular column
into a generated column.  Perhaps I can convince you by showing the
similar

  ALTER TABLE tab ALTER col ADD GENERATED ALWAYS AS IDENTITY;

Here, too, we are not adding a new generated column, only turning a
regular column into a generated one.  I think it is good to stay as
close to that already established syntax as possible.

> equalities seems simple enough, I'll try to do this in the next version

Right; I thought that it was no big deal.  And this would be the only case
in the PostgreSQL SQL dialect that I know where the order of the operands
for an equality condition matters.

> makes pgindent skip the code.

I had not tested it, but the lines seemed to be shorter than the 80 columns
to which pgindent formats comments.
If you already ran pgindent, please ignore my comment.

Yours,
Laurenz Albe