Date: Mon, 31 Aug 2026 20:41:33 +0200
From: Alberto Piai <alberto.piai@gmail.com>
To: Laurenz Albe <laurenz.albe@cybertec.at>,
	Alberto Piai <alberto.piai@gmail.com>, pgsql-hackers@postgresql.org
Cc: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
Subject: Re: Adding a stored generated column without long-lived locks
Message-ID: <DL3CJBUV86UG.3IZJ0KV63YKZ2@gmail.com>
References: <DKHUPZ08JV6G.2938OV580K72C@gmail.com>
 <6f5ea02f6e5205a96a9b3979190a4d7cb3c99414.camel@cybertec.at>
 <DKXFEG915LK8.12HECZQW2OWQE@gmail.com>
 <86a9336dc25d9a085916ad247a5791dc53d2ab13.camel@cybertec.at>
 <DKY5OJ1NSSGO.3U2FKRKCHM533@gmail.com>
 <57bd5c0e2b532c07a1e6cd867520e899b1007622.camel@cybertec.at>
 <DL0C8TG99HCO.1XVPKG2IM0C1S@gmail.com>
 <31fec16020bdf25d4a62cbbb4b8ea00af007e0d9.camel@cybertec.at>
 <DL0PFQ4CM3KO.38HQUI04TKPLT@gmail.com>
 <82554bb40f6822bebbda7fcbc5ec1dc6a1823c0d.camel@cybertec.at>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="lthiybfwoezahbdk"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <82554bb40f6822bebbda7fcbc5ec1dc6a1823c0d.camel@cybertec.at>
Archived-At: 
 <https://www.postgresql.org/message-id/DL3CJBUV86UG.3IZJ0KV63YKZ2%40gmail.com>
Precedence: bulk


--lthiybfwoezahbdk
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hi Laurenz,

On Sat Aug 29, 2026 at 8:14 AM CEST, Laurenz Albe wrote:
> On Fri, 2026-08-28 at 18:07 +0200, Alberto Piai wrote:
>> Before this gets eventually picked up by a committer: I am having second
>> thoughts about my choice to allow = in addition to IS NOT DISTINCT FROM.
>
> I think I see what you mean: this command exists exclusively so that users
> can add a generated column to a bigger table without downtime.  So they
> will create the constraint specifically for this purpose, and it wouldn't
> be a loss of functionality to force them to use IS NOT DISTINCT FROM.
> Removing support for a constraint with = would simplify the code and the
> documentation.
>
> I won't object to that, but I like the patch as it is now.
> I can imagine a case where somebody uses a regular column with a check
> constraint and at some later point decides to turn the column into a
> generated column.  That user might be annoyed if they had to create a
> second check constraint, since there already is a perfectly good one.

I liked the = form too (but: there is a "but" coming later :)), I always
looked at this from the perspective of a user who is intentionally
running through these steps to perform a very specific migration.
Assuming they don't make mistakes (or the expression is not nullable, as
in the simple case of "b = a + 1" where the referenced a is NOT NULL), =
works fine.

The problem is that it's relatively easy to make a mistake (or a
malicious user could take advantage of it) whenever the expression is
nullable. In that case, a row could be added to the table that still
satisfies the constraint (since CHECK constraints are satisfied when the
expression evaluates to NULL), the alter table would happily run
through, and the db would be left in an inconsistent state.

In the case above of "b = a + 1", if a is nullable, b is NOT NULL and a
row (a, b) with values (null, 42) is inserted, rewriting operations like
update ... set a = a, or pg_dump/pg_restore would fail.

(This can easily be tested manually. I tried to trigger a run on the
GitHub CI to show this, but the pg_upgrade test suite with
PG_TEST_EXTRA=regress_dump_restore doesn't run there.)

I also considered trying to prove non-nullability of the expression
(there is for example expr_is_nonnullable() in clauses.c which does
this), but the attempt failed on the realization that this would really
require full static analysis of arbitrary functions in the expression.
(The source column being NOT NULL is not sufficient, as a function used
in the expression could still decide to return NULL in arbitrary cases.)

So I think this is unfortunately a no-go, even if the form with = looks
a lot more user-friendly.

I have attached v10, which:

- removes support for the = form of the constraints, adapting tests,
  documentation and error messages

- removes an unnecessary ifdef I had around the injection points
  definitions

- improves the detail message in the case where the constraint is found,
  but its shape doesn't match the expected form:

 alter table tgen.t1 add constraint chk_gen check (b = a * 2);
 alter table tgen.t1 alter column b add generated using constraint chk_gen stored;
 ERROR:  cannot convert column "b" to generated
-DETAIL:  Could not find a valid constraint "chk_gen" CHECK ("b" IS NOT DISTINCT FROM expr).
+DETAIL:  The constraint "chk_gen" is not of the form CHECK ("b" IS NOT DISTINCT FROM expr).


> I'd say that if you remove support for =, you might as well also remove
> support for check constraints in the shape "(expression IS NOT DISTINCT
> FROM column)".

I left this in, as the extra code complexity is really tiny, so I
thought it doesn't hurt much to have it there.


I wish I'd have caught this sooner (especially after having been warned
about this very problem in the first version of the patch) as it would
have saved you some review effort. At least it's fixed before the next
reviews :)


Best regards,

Alberto


-- 
Alberto Piai
Sensational AG
Zürich, Switzerland


--lthiybfwoezahbdk
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment;
	filename="v10-0001-Support-changing-a-column-into-a-stored-generate.patch"