Content-Type: text/plain; charset=UTF-8
Date: Fri, 10 Jul 2026 02:10:57 +0200
Message-Id: <DJUGET7QLNRK.7G2H4M2PPJM9@gmail.com>
From: "Alberto Piai" <alberto.piai@gmail.com>
To: "Laurenz Albe" <laurenz.albe@cybertec.at>, "Alberto Piai"
 <alberto.piai@gmail.com>, <pgsql-hackers@postgresql.org>
Subject: Re: Adding a stored generated column without long-lived locks
Cc: =?utf-8?q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
References: <abkrpUwlGngF4e-d@phidippus.sen.work>
 <DHMSK551GIM8.1B1CN2JN8BK50@gmail.com>
 <DI19FP3X4VI0.21FO6V1HGJK5P@gmail.com>
 <DIIRFCWJIA80.2Q5DYIX6C7KZ5@gmail.com>
 <DITN90YX3328.2A0I3LCV164BA@gmail.com>
 <3af338791e8ab27c4a7efbc4217c97162de76bc8.camel@cybertec.at>
 <DJMFASI8OYBS.2JBMQ7KZ2XOTO@gmail.com>
 <DJOPT622HYJK.3JRH1VQCV5CDL@gmail.com>
 <2eb68e74ea95fc2f691de7d8cd28448d00d1d216.camel@cybertec.at>
In-Reply-To: <2eb68e74ea95fc2f691de7d8cd28448d00d1d216.camel@cybertec.at>
Archived-At: 
 <https://www.postgresql.org/message-id/DJUGET7QLNRK.7G2H4M2PPJM9%40gmail.com>
Precedence: bulk

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 exp=
licitly
> asked several people for a review (which is fine!): it is expected that y=
ou 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 CONSTRAIN=
T)
> is ugly as hell.  I see your point in having ALWAYS and STORED, but perha=
ps
> ADD GENERATED ALWAYS USING CONSTRAINT ... STORED would be better, as it i=
s
> syntactically more like GENERATED ALWAYS AS (...) STORED, which would mak=
e
> it easier to remember.
>
> Or perhaps ADD GENERATED USING CONSTRAINT would be enough, since ALWAYS a=
nd
> 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 =3D 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" =3D 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 =3D 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 =3D GetAttrDefaultColumnAddress(fo=
undObject.objectId);
>> +
>> +                   if (col.objectId =3D=3D RelationGetRelid(rel) &&
>> +                       col.objectSubId =3D=3D attnum)
>> +                   {
>> +                       /*
>> +                        * Ignore the column's own default expression. W=
e
>> +                        * handle sequences above, and for a column whic=
h is
>> +                        * already a generated column we should never ge=
t
>> +                        * here.
>> +                        */
>
> It's not strictly required, but it would be great if you could run pginde=
nt
> 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 con=
straint
>> + * 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


--=20
Alberto Piai
Sensational AG
Z=C3=BCrich, Switzerland