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

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 will
> + then be performed without rewriting the table.

The "generation expression" suddenly surfaces towards the end of the paragraph
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 = 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.

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 DISTINCT 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 columns.
> + */
> + if (!recursing)
> + {
> [...]
> + if (attTup->attinhcount > 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("cannot change inherited column to be a stored generated column")));
> + }

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

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

> +/*
> + * Detect dependencies which should stop us from turning a regular column
> + * into a stored generated column.
> + */
> +static void
> +checkDependenciesForAddExprStored(Relation rel,
> + AttrNumber attnum,
> + const char *colName)
> [...]
> + switch (foundObject.classId)
> + {
> + case RelationRelationId:
> + {
> + char relKind = get_rel_relkind(foundObject.objectId);
> +
> + if (relKind == RELKIND_SEQUENCE)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot convert a serial column to a stored generated column"),
> + errdetail("\"%s\" of relation \"%s\" depends on sequence %s",
> + colName, RelationGetRelationName(rel),
> + getObjectDescription(&foundObject, false))));

There is an extra space in the error message.

> + break;
> + }
> + 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.

> + }
> + else
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot convert a column referenced in a default expression to a stored generated column"),
> + errdetail("Column \"%s\" is referenced by 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 in a
DEFAULT expression. But you can never use other columns in a default expression
anyway, right?
The detail message says something different, namely that the column is referenced
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 will
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 constraint
> + * 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 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.

> +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 generated column"),
> + errdetail("column \"%s\" of relation \"%s\" is an identity column",
> + colName, RelationGetRelationName(rel))));

Message style: the main error message should start with a lower case character,
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 == 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\" CHECK (\"%s\" = expr) or CHECK (\"%s\" IS NOT DISTINCT FROM (expr))",
> + def->conname,
> + colName,
> + colName) :
> + errdetail("could not find a valid constraint \"%s\" CHECK (\"%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 are correct
HINT: The constraint must be CHECK ("%s" = 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 errors
instead, then they could be more specific and pertinent.

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.

Yours,
Laurenz Albe