Message-ID: <2eb68e74ea95fc2f691de7d8cd28448d00d1d216.camel@cybertec.at>
Subject: Re: Adding a stored generated column without long-lived locks
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Alberto Piai <alberto.piai@gmail.com>, pgsql-hackers@postgresql.org
Cc: =?ISO-8859-1?Q?=C1lvaro?= Herrera <alvherre@kurilemu.de>
Date: Tue, 07 Jul 2026 20:16:36 +0200
In-Reply-To: <DJOPT622HYJK.3JRH1VQCV5CDL@gmail.com>
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.58.3 (3.58.3-1.fc43) 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/2eb68e74ea95fc2f691de7d8cd28448d00d1d216.camel%40cybertec.at>
Precedence: bulk

On Fri, 2026-07-03 at 08:42 +0200, Alberto Piai wrote:
> 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.

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

> This version addresses your inputs from the last review:
>
> - I added documentation for the new alter table form to alter_table.sgml
> - Tab completion for psql is there now
> - The missing error conditions in case of an identity column or
>   sequences are now handled, more about this in the next section.

Thanks.

I think you didn't adapt the documentation sufficiently after you dropped
the generation expression from the syntax:

> +      This form changes a regular column into a stored generated column,=
 using
> +      the expression from the given constraint. The constraint must be a
> +      <literal>CHECK</literal> constraint proving that the values of the
> +      column already satisfy the generation expression. The operation wi=
ll
> +      then be performed without rewriting the table.

The "generation expression" suddenly surfaces towards the end of the paragr=
aph
and makes the reader wonder where it comes from.

Perhaps:

  This form changes a regular column into a stored generated column, using
  the expression from the given check constraint as generation expression.
  The operation will be performed without rewriting the table, which avoids
  holding an <literal>ACCESS EXCLUSIVE</literal> lock for a longer time.

That would render the immediately following paragraph unnecessary.


The patch applies, builds and passes the regression tests.


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 CH=
ECK ("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.


The following error message is not very helpful:

  CREATE TABLE tab (
    a integer DEFAULT 2,
    b integer
      CONSTRAINT con CHECK (b IS NOT DISTINCT FROM 2 + random())
  );

  ALTER TABLE tab ALTER b ADD GENERATED ALWAYS STORED USING CONSTRAINT con;
  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 "con" CHECK ("b" IS NOT DISTIN=
CT FROM (expr))

Perhaps it would be better to proceed in three steps:

- find a check constraint with the given name
- verify that the check constraint has the correct shape
- verify that the expression is immutable

Each step could have a different, helpful error message.


I looked at the code too, and I could only spot smaller problems:

> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> [...]
> +ATPrepAddExpressionStored(Relation rel,
> +                         AlterTableCmd *cmd,
> +                         bool recurse, bool recursing,
> +                         LOCKMODE lockmode)
> [...]
> +   /*
> +    * Cannot change only inherited columns to be stored generated column=
s.
> +    */
> +   if (!recursing)
> +   {
> [...]
> +       if (attTup->attinhcount > 0)
> +           ereport(ERROR,
> +                   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> +                    errmsg("cannot change inherited column to be a store=
d generated column")));
> +   }

The comment is cryptic, and I had to read the code to understand what you m=
ean.
Perhaps:

/* convert inherited columns only if the entire hierarchy is changed */

> +/*
> + * Detect dependencies which should stop us from turning a regular colum=
n
> + * into a stored generated column.
> + */
> +static void
> +checkDependenciesForAddExprStored(Relation rel,
> +                                 AttrNumber attnum,
> +                                 const char *colName)
> [...]
> +       switch (foundObject.classId)
> +       {
> +           case RelationRelationId:
> +               {
> +                   char        relKind =3D get_rel_relkind(foundObject.o=
bjectId);
> +
> +                   if (relKind =3D=3D RELKIND_SEQUENCE)
> +                       ereport(ERROR,
> +                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISI=
TE_STATE),
> +                                errmsg("cannot convert a serial column t=
o a stored generated column"),
> +                                errdetail("\"%s\" of relation \"%s\"  de=
pends on sequence %s",
> +                                          colName, RelationGetRelationNa=
me(rel),
> +                                          getObjectDescription(&foundObj=
ect, false))));

There is an extra space in the error message.

> +                   break;
> +               }
> +           case AttrDefaultRelationId:
> +               {
> +                   ObjectAddress col =3D GetAttrDefaultColumnAddress(fou=
ndObject.objectId);
> +
> +                   if (col.objectId =3D=3D RelationGetRelid(rel) &&
> +                       col.objectSubId =3D=3D 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.

> +                   }
> +                   else
> +                   {
> +                       ereport(ERROR,
> +                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISI=
TE_STATE),
> +                                errmsg("cannot convert a column referenc=
ed in a default expression to a stored generated column"),
> +                                errdetail("Column \"%s\" is referenced b=
y generated column \"%s\".",
> +                                          colName,
> +                                          get_attname(col.objectId, col.=
objectSubId, false))));

This is confusing me.
The error message seems to suggest that you cannot use a generated column i=
n a
DEFAULT expression. But you can never use other columns in a default expres=
sion
anyway, right?
The detail message says something different, namely that the column is refe=
renced
my a generated column (do you mean it is used in the generation expression)=
?

I believe that if I get confused by the error message, the average user wil=
l
also get confused.

> +                   }
> +                   break;
> +               }
> +           default:
> +               /* We're not interested in the row */
> +               break;
> +       }

Perhaps a better comment would be

/* other dependencies, e.g. by views, are no problem */

> +/*
> + * Subroutine for ATExecAddExpressionStored, used to find a CHECK constr=
aint
> + * to prove that the column values statisfy what will be the generator
> + * expression.
> + *
> [...]
> + *
> + * If a valid constraint is found, this returns both the Oid of the cons=
traint
> + * 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.

> +static ObjectAddress
> +ATExecAddExpressionStored(AlteredTableInfo *tab,
> +                         Relation rel,
> +                         const char *colName,
> +                         Constraint *def)
> +{
> [...]
> +   if (attTup->attidentity)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                errmsg("Cannot convert an identity column to a stored ge=
nerated column"),
> +                errdetail("column \"%s\" of relation \"%s\" is an identi=
ty column",
> +                          colName, RelationGetRelationName(rel))));

Message style: the main error message should start with a lower case charac=
ter,
and the detail message should be a whole sentence (initial capitalization, =
period).
Error messages should try not to exceed 80 characters (no problem with that=
 here),

> +   if (has_partition_attrs(rel, colRefs, &is_expr))
> +       ereport(ERROR,
> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                errmsg("cannot convert a column into a stored generated =
column if it's referenced by a partition key"),
> +                errdetail("column \"%s\" is part of the partition key of=
 relation \"%s\"",
> +                          colName, RelationGetRelationName(rel))));

Same as above, plus the error message is too long.
Perhaps:

  ERROR:  cannot convert a column used in a partitioning key to a generated=
 column

I don't think we need to say "stored" everywhere.

> +   if (foundConstraintExpr =3D=3D NULL)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                errmsg("cannot convert a column into a stored generated =
column without a constraint to prove that the values are consistent"),
> +                attTup->attnotnull ?
> +                errdetail("could not find a valid constraint \"%s\" CHEC=
K (\"%s\" =3D expr) or CHECK (\"%s\" IS NOT DISTINCT FROM (expr))",
> +                          def->conname,
> +                          colName,
> +                          colName) :
> +                errdetail("could not find a valid constraint \"%s\" CHEC=
K (\"%s\" IS NOT DISTINCT FROM (expr))",
> +                          def->conname,
> +                          colName)));

Again, the message must be shorter, and perhaps a hint would be better than=
 a detail:

  ERROR:  cannot find a check constraint to prove that the column values ar=
e correct
  HINT:  The constraint must be CHECK ("%s" =3D expr) or CHECK ("%s" IS NOT=
 DISTINCT FROM expr).

I talked about this error message in the beginning.  It is thrown whenever
findUsableConstraintForAddExprStored() returns nothing, which is a bit too =
unspecific.

Perhaps you could have findUsableConstraintForAddExprStored() throw the err=
ors
instead, then they could be more specific and pertinent.


I don't usually mention that, but since you are a new contributor and expli=
citly
asked several people for a review (which is fine!): it is expected that you=
 also
review other's patches in the commitfest.

Yours,
Laurenz Albe