public inbox for [email protected]help / color / mirror / Atom feed
Re: Unexpected results from CALL and AUTOCOMMIT=off 8+ messages / 5 participants [nested] [flat]
* Re: Unexpected results from CALL and AUTOCOMMIT=off @ 2024-06-03 18:15 Victor Yegorov <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Victor Yegorov @ 2024-06-03 18:15 UTC (permalink / raw) To: Pierre Forstmann <[email protected]>; +Cc: pgsql-general <[email protected]> пн, 3 июн. 2024 г. в 20:40, Pierre Forstmann <[email protected]>: > You declared function f_get_x as stable which means: > > … > > If you remove stable from function declaration, it works as expected: > Well, I checked https://www.postgresql.org/docs/current/xfunc-volatility.html There's a paragraph describing why STABLE (and IMMUTABLE) use different snapshots: > For functions written in SQL or in any of the standard procedural languages, there is a second important property determined by the volatility category, namely the visibility of any data changes that have been made by the SQL command that is calling the function. A > VOLATILE function will see such changes, a STABLE or IMMUTABLE function will not. This behavior is implemented using the snapshotting behavior of MVCC (see Chapter 13): STABLE and IMMUTABLE functions use a snapshot established as of the start of the > calling query, whereas VOLATILE functions obtain a fresh snapshot at the start of each query they execute. But later, docs state, that > Because of this snapshotting behavior, a function containing only SELECT commands can safely be marked STABLE, even if it selects from tables that might be undergoing modifications by concurrent queries. PostgreSQL will execute all commands of a STABLE function using the snapshot established for the calling query, and so it will see a fixed view of the database throughout that query. And therefore I assume STABLE should work in this case. Well, it seems not to. I assume there's smth to do with implicit BEGIN issued in non-AUTOCOMMIT mode and non-atomic DO block behaviour. -- Victor Yegorov ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Unexpected results from CALL and AUTOCOMMIT=off @ 2024-06-03 19:28 Tom Lane <[email protected]> parent: Victor Yegorov <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Tom Lane @ 2024-06-03 19:28 UTC (permalink / raw) To: Victor Yegorov <[email protected]>; +Cc: Pierre Forstmann <[email protected]>; pgsql-general <[email protected]> Victor Yegorov <[email protected]> writes: > пн, 3 июн. 2024 г. в 20:40, Pierre Forstmann <[email protected]>: >> If you remove stable from function declaration, it works as expected: > ... therefore I assume STABLE should work in this case. Well, it seems not > to. I agree that this looks like a bug, since your example shows that the same function works as-expected in an ordinary expression but not in a CALL. The dependency on AUTOCOMMIT (that is, being within an outer transaction block) seems even odder. I've not dug into it yet, but I suppose we're passing the wrong snapshot to the CALL arguments. A volatile function wouldn't use that snapshot, explaining Pierre's result. regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Unexpected results from CALL and AUTOCOMMIT=off @ 2024-06-04 01:32 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Tom Lane @ 2024-06-04 01:32 UTC (permalink / raw) To: [email protected]; +Cc: Peter Eisentraut <[email protected]>; Victor Yegorov <[email protected]>; Pierre Forstmann <[email protected]> [ redirecting to pgsql-hackers ] I wrote: > I agree that this looks like a bug, since your example shows that the > same function works as-expected in an ordinary expression but not in > a CALL. The dependency on AUTOCOMMIT (that is, being within an outer > transaction block) seems even odder. I've not dug into it yet, but > I suppose we're passing the wrong snapshot to the CALL arguments. I poked into this and found that the source of the problem is that plpgsql's exec_stmt_call passes allow_nonatomic = true even when it's running in an atomic context. So we can fix it with basically a one-line change: - options.allow_nonatomic = true; + options.allow_nonatomic = !estate->atomic; I'm worried about whether external callers might've made a comparable mistake, but I think all we can do is document it a little better. AFAICS there isn't any good way for spi.c to realize that this mistake has been made, else we could have it patch up the mistake centrally. I've not attempted to make those doc updates in the attached draft patch though, nor have I added a test case yet. Before realizing that this was the issue, I spent a fair amount of time on the idea that _SPI_execute_plan() was doing things wrong, and that led me to notice that its comment about having four modes of snapshot operation has been falsified in multiple ways. So this draft does include fixes for that comment. Thoughts? regards, tom lane Attachments: [text/x-diff] fix-call-in-atomic-context-draft.patch (2.3K, ../../[email protected]/2-fix-call-in-atomic-context-draft.patch) download | inline diff: diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index a97a7e3bd4..c93b6c192f 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -2425,12 +2425,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, * snapshot != InvalidSnapshot, read_only = false: use the given snapshot, * modified by advancing its command ID before each querytree. * - * snapshot == InvalidSnapshot, read_only = true: use the entry-time - * ActiveSnapshot, if any (if there isn't one, we run with no snapshot). + * snapshot == InvalidSnapshot, read_only = true: do nothing for queries + * that require no snapshot. For those that do, ensure that a Portal + * snapshot exists; then use that, or use the entry-time ActiveSnapshot if + * that exists and is different. * - * snapshot == InvalidSnapshot, read_only = false: take a full new - * snapshot for each user command, and advance its command ID before each - * querytree within the command. + * snapshot == InvalidSnapshot, read_only = false: do nothing for queries + * that require no snapshot. For those that do, ensure that a Portal + * snapshot exists; then, in atomic execution (!allow_nonatomic) take a + * full new snapshot for each user command, and advance its command ID + * before each querytree within the command. In allow_nonatomic mode we + * just use the Portal snapshot unmodified. * * In the first two cases, we can just push the snap onto the stack once * for the whole plan list. diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 6947575b94..54ede9d85e 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -2218,12 +2218,12 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt) * for the plan. This avoids refcount leakage complaints if the called * procedure ends the current transaction. * - * Also, tell SPI to allow non-atomic execution. + * Also, tell SPI to allow non-atomic execution if appropriate. */ memset(&options, 0, sizeof(options)); options.params = paramLI; options.read_only = estate->readonly_func; - options.allow_nonatomic = true; + options.allow_nonatomic = !estate->atomic; options.owner = estate->procedure_resowner; rc = SPI_execute_plan_extended(expr->plan, &options); ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Unexpected results from CALL and AUTOCOMMIT=off @ 2024-06-04 18:28 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Tom Lane @ 2024-06-04 18:28 UTC (permalink / raw) To: [email protected]; +Cc: Peter Eisentraut <[email protected]>; Victor Yegorov <[email protected]>; Pierre Forstmann <[email protected]> I wrote: > I poked into this and found that the source of the problem is that > plpgsql's exec_stmt_call passes allow_nonatomic = true even when > it's running in an atomic context. So we can fix it with basically > a one-line change: > - options.allow_nonatomic = true; > + options.allow_nonatomic = !estate->atomic; > I'm worried about whether external callers might've made a comparable > mistake, but I think all we can do is document it a little better. > AFAICS there isn't any good way for spi.c to realize that this mistake > has been made, else we could have it patch up the mistake centrally. Actually, after poking around some more I found that there *is* a way to deal with this within spi.c: we can make _SPI_execute_plan ignore options->allow_nonatomic unless the SPI_OPT_NONATOMIC flag was given when connecting. I like this better than my first solution because (a) it seems to make the allow_nonatomic flag behave in a more intuitive way; (b) spi.c gates some other behaviors on SPI_OPT_NONATOMIC, so that gating this one too seems more consistent, and (c) this way, we fix not only plpgsql but anything that has copied its coding pattern. Hence, new patch attached, now with docs and tests. Barring objections I'll push this one. regards, tom lane Attachments: [text/x-diff] fix-call-in-atomic-context-v1.patch (7.2K, ../../[email protected]/2-fix-call-in-atomic-context-v1.patch) download | inline diff: diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index bb3778688b..7d154914b9 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -752,7 +752,9 @@ int SPI_execute_extended(const char *<parameter>command</parameter>, <listitem> <para> <literal>true</literal> allows non-atomic execution of CALL and DO - statements + statements (but this field is ignored unless + the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed + to <function>SPI_connect_ext</function>) </para> </listitem> </varlistentry> @@ -1893,7 +1895,9 @@ int SPI_execute_plan_extended(SPIPlanPtr <parameter>plan</parameter>, <listitem> <para> <literal>true</literal> allows non-atomic execution of CALL and DO - statements + statements (but this field is ignored unless + the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed + to <function>SPI_connect_ext</function>) </para> </listitem> </varlistentry> diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index a97a7e3bd4..e516c0a67c 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -2399,6 +2399,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, uint64 my_processed = 0; SPITupleTable *my_tuptable = NULL; int res = 0; + bool allow_nonatomic; bool pushed_active_snap = false; ResourceOwner plan_owner = options->owner; SPICallbackArg spicallbackarg; @@ -2406,6 +2407,12 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, CachedPlan *cplan = NULL; ListCell *lc1; + /* + * We allow nonatomic behavior only if options->allow_nonatomic is set + * *and* the SPI_OPT_NONATOMIC flag was given when connecting. + */ + allow_nonatomic = options->allow_nonatomic && !_SPI_current->atomic; + /* * Setup error traceback support for ereport() */ @@ -2425,12 +2432,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, * snapshot != InvalidSnapshot, read_only = false: use the given snapshot, * modified by advancing its command ID before each querytree. * - * snapshot == InvalidSnapshot, read_only = true: use the entry-time - * ActiveSnapshot, if any (if there isn't one, we run with no snapshot). + * snapshot == InvalidSnapshot, read_only = true: do nothing for queries + * that require no snapshot. For those that do, ensure that a Portal + * snapshot exists; then use that, or use the entry-time ActiveSnapshot if + * that exists and is different. * - * snapshot == InvalidSnapshot, read_only = false: take a full new - * snapshot for each user command, and advance its command ID before each - * querytree within the command. + * snapshot == InvalidSnapshot, read_only = false: do nothing for queries + * that require no snapshot. For those that do, ensure that a Portal + * snapshot exists; then, in atomic execution (!allow_nonatomic) take a + * full new snapshot for each user command, and advance its command ID + * before each querytree within the command. In allow_nonatomic mode we + * just use the Portal snapshot unmodified. * * In the first two cases, we can just push the snap onto the stack once * for the whole plan list. @@ -2440,6 +2452,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, */ if (snapshot != InvalidSnapshot) { + /* this intentionally tests the options field not the derived value */ Assert(!options->allow_nonatomic); if (options->read_only) { @@ -2585,7 +2598,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, * Skip it when doing non-atomic execution, though (we rely * entirely on the Portal snapshot in that case). */ - if (!options->read_only && !options->allow_nonatomic) + if (!options->read_only && !allow_nonatomic) { if (pushed_active_snap) PopActiveSnapshot(); @@ -2685,14 +2698,13 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, QueryCompletion qc; /* - * If the SPI context is atomic, or we were not told to allow - * nonatomic operations, tell ProcessUtility this is an atomic - * execution context. + * If we're not allowing nonatomic operations, tell + * ProcessUtility this is an atomic execution context. */ - if (_SPI_current->atomic || !options->allow_nonatomic) - context = PROCESS_UTILITY_QUERY; - else + if (allow_nonatomic) context = PROCESS_UTILITY_QUERY_NONATOMIC; + else + context = PROCESS_UTILITY_QUERY; InitializeQueryCompletion(&qc); ProcessUtility(stmt, diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out index 17235fca91..0a63b1d44e 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_call.out +++ b/src/pl/plpgsql/src/expected/plpgsql_call.out @@ -564,3 +564,53 @@ NOTICE: inner_p(44) (1 row) +-- Check that stable functions in CALL see the correct snapshot +CREATE TABLE t_test (x int); +INSERT INTO t_test VALUES (0); +CREATE FUNCTION f_get_x () RETURNS int +AS $$ +DECLARE l_result int; +BEGIN + SELECT x INTO l_result FROM t_test; + RETURN l_result; +END +$$ LANGUAGE plpgsql STABLE; +CREATE PROCEDURE f_print_x (x int) +AS $$ +BEGIN + RAISE NOTICE 'f_print_x(%)', x; +END +$$ LANGUAGE plpgsql; +-- test in non-atomic context +DO $$ +BEGIN + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + ROLLBACK; +END +$$; +NOTICE: f_get_x(1) +NOTICE: f_print_x(1) +NOTICE: f_get_x(2) +NOTICE: f_print_x(2) +-- test in atomic context +BEGIN; +DO $$ +BEGIN + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); +END +$$; +NOTICE: f_get_x(1) +NOTICE: f_print_x(1) +NOTICE: f_get_x(2) +NOTICE: f_print_x(2) +ROLLBACK; diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql index 869d021a07..4cbda0382e 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_call.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql @@ -522,3 +522,53 @@ CREATE FUNCTION f(int) RETURNS int AS $$ SELECT $1 + 2 $$ LANGUAGE sql; CALL outer_p(42); SELECT outer_f(42); + +-- Check that stable functions in CALL see the correct snapshot + +CREATE TABLE t_test (x int); +INSERT INTO t_test VALUES (0); + +CREATE FUNCTION f_get_x () RETURNS int +AS $$ +DECLARE l_result int; +BEGIN + SELECT x INTO l_result FROM t_test; + RETURN l_result; +END +$$ LANGUAGE plpgsql STABLE; + +CREATE PROCEDURE f_print_x (x int) +AS $$ +BEGIN + RAISE NOTICE 'f_print_x(%)', x; +END +$$ LANGUAGE plpgsql; + +-- test in non-atomic context +DO $$ +BEGIN + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + ROLLBACK; +END +$$; + +-- test in atomic context +BEGIN; + +DO $$ +BEGIN + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); +END +$$; + +ROLLBACK; ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Unexpected results from CALL and AUTOCOMMIT=off @ 2024-06-04 20:13 Nathan Bossart <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Nathan Bossart @ 2024-06-04 20:13 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]>; Victor Yegorov <[email protected]>; Pierre Forstmann <[email protected]> On Tue, Jun 04, 2024 at 02:28:43PM -0400, Tom Lane wrote: > Actually, after poking around some more I found that there *is* a way > to deal with this within spi.c: we can make _SPI_execute_plan ignore > options->allow_nonatomic unless the SPI_OPT_NONATOMIC flag was given > when connecting. > > I like this better than my first solution because (a) it seems to > make the allow_nonatomic flag behave in a more intuitive way; > (b) spi.c gates some other behaviors on SPI_OPT_NONATOMIC, so that > gating this one too seems more consistent, and (c) this way, we fix > not only plpgsql but anything that has copied its coding pattern. +1 > Hence, new patch attached, now with docs and tests. Barring > objections I'll push this one. Should we expand the documentation for SPI_connect_ext() to note that SPI_execute_extended()/SPI_execute_plan_extended() depend on the flag? -- nathan ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Unexpected results from CALL and AUTOCOMMIT=off @ 2024-06-04 20:31 Tom Lane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Tom Lane @ 2024-06-04 20:31 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]>; Victor Yegorov <[email protected]>; Pierre Forstmann <[email protected]> Nathan Bossart <[email protected]> writes: > On Tue, Jun 04, 2024 at 02:28:43PM -0400, Tom Lane wrote: >> Hence, new patch attached, now with docs and tests. Barring >> objections I'll push this one. > Should we expand the documentation for SPI_connect_ext() to note that > SPI_execute_extended()/SPI_execute_plan_extended() depend on the flag? Perhaps. They already did, in that the atomic flag was taken into account while deciding how to handle a nested CALL; basically what this fix does is to make sure that the snapshot handling is done the same way. I think that what I added to the docs is probably sufficient, but I'll yield to majority opinion if people think not. regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Adding a stored generated column without long-lived locks @ 2026-07-10 00:10 Alberto Piai <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Alberto Piai @ 2026-07-10 00:10 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; Alberto Piai <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]> 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Adding a stored generated column without long-lived locks @ 2026-07-10 06:51 Laurenz Albe <[email protected]> parent: Alberto Piai <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Laurenz Albe @ 2026-07-10 06:51 UTC (permalink / raw) To: Alberto Piai <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]> On Fri, 2026-07-10 at 02:10 +0200, Alberto Piai wrote: > > 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 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. > > > > > 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. 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. > > > + /* > > > + * 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. 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-07-10 06:51 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-06-03 18:15 Re: Unexpected results from CALL and AUTOCOMMIT=off Victor Yegorov <[email protected]> 2024-06-03 19:28 ` Tom Lane <[email protected]> 2024-06-04 01:32 ` Tom Lane <[email protected]> 2024-06-04 18:28 ` Tom Lane <[email protected]> 2024-06-04 20:13 ` Nathan Bossart <[email protected]> 2024-06-04 20:31 ` Tom Lane <[email protected]> 2026-07-10 00:10 Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-07-10 06:51 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox