public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v45 2/3] Filling gaps in jsonb 9+ messages / 3 participants [nested] [flat]
* [PATCH v45 2/3] Filling gaps in jsonb @ 2020-12-31 14:19 Dmitrii Dolgov <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Dmitrii Dolgov @ 2020-12-31 14:19 UTC (permalink / raw) Introduces two new modes for jsonb assignment: * Appending array elements on the specified position, gaps filled with nulls (similar to JavaScript behavior). This mode also instructs to create the whole path in a jsonb object if some part of the path (more than just the last element) is not present. * Assigning keeps array positions consistent by prevent prepending of elements. Originally proposed by Nikita Glukhov based on polymorphic subscripting patch, but transformed into an independent change. --- doc/src/sgml/json.sgml | 33 ++++ src/backend/utils/adt/jsonfuncs.c | 226 ++++++++++++++++++++++++++-- src/test/regress/expected/jsonb.out | 135 +++++++++++++++++ src/test/regress/sql/jsonb.sql | 81 ++++++++++ 4 files changed, 460 insertions(+), 15 deletions(-) diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml index 100d1a60f4..9af015d222 100644 --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -645,6 +645,39 @@ UPDATE table_name SET jsonb_field['a'] = '1'; -- If jsonb_field here is NULL, the result is [1] UPDATE table_name SET jsonb_field[0] = '1'; +</programlisting> + + Jsonb assignment via subscripting handles few edge cases differently + from <literal>jsonb_set</literal>. When assigning to the jsonb array + to the specified index, but there are no other elements present, the + result will be a jsonb array with the ewn value by specified index and + <type>null</type> elements from the first index to the specified index. + +<programlisting> +-- If jsonb_field is [], the result is [null, null, 2] +UPDATE table_name SET jsonb_field[2] = '2'; +</programlisting> + + When assigning to the jsonb array to the specified index, but position + of the last element in the array is less than the specified index, the + result will be a jsonb array with the new value by specified index and + <type>null</type> elements from the last index to the specified index. + +<programlisting> +-- If jsonb_field is [0], the result is [0, null, 2] +UPDATE table_name SET jsonb_field[2] = '2'; +</programlisting> + + When assigning using the path which is not present in the source jsonb, + the result will be a jsonb with the specified path created and the new + value at the end of the path. + +<programlisting> +-- If jsonb_field is {}, the result is {'a': [{'b': 1}]} +UPDATE table_name SET jsonb_field['a'][0]['b'] = '1'; + +-- If jsonb_field is [], the result is [{'a': 1}] +UPDATE table_name SET jsonb_field[0]['a'] = '1'; </programlisting> </para> diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 5a0ba6b220..f14f6c3191 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -44,6 +44,8 @@ #define JB_PATH_INSERT_AFTER 0x0010 #define JB_PATH_CREATE_OR_INSERT \ (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER | JB_PATH_CREATE) +#define JB_PATH_FILL_GAPS 0x0020 +#define JB_PATH_CONSISTENT_POSITION 0x0040 /* state for json_object_keys */ typedef struct OkeysState @@ -1634,14 +1636,116 @@ jsonb_set_element(Jsonb* jb, Datum *path, int path_len, it = JsonbIteratorInit(&jb->root); - res = setPath(&it, path, path_nulls, path_len, &state, 0, - newval, JB_PATH_CREATE); + res = setPath(&it, path, path_nulls, path_len, &state, 0, newval, + JB_PATH_CREATE | JB_PATH_FILL_GAPS | + JB_PATH_CONSISTENT_POSITION); pfree(path_nulls); PG_RETURN_JSONB_P(JsonbValueToJsonb(res)); } +static void +push_null_elements(JsonbParseState **ps, int num) +{ + JsonbValue null; + + null.type = jbvNull; + + while (num-- > 0) + pushJsonbValue(ps, WJB_ELEM, &null); +} + +/* + * Prepare a new structure containing nested empty objects and arrays + * corresponding to the specified path, and assign a new value at the end of + * this path. E.g. the path [a][0][b] with the new value 1 will produce the + * structure {a: [{b: 1}]}. + * + * Called is responsible to make sure such path does not exist yet. + */ +static void +push_path(JsonbParseState **st, int level, Datum *path_elems, + bool *path_nulls, int path_len, JsonbValue *newval) +{ + /* + * tpath contains expected type of an empty jsonb created at each level + * higher or equal than the current one, either jbvObject or jbvArray. + * Since it contains only information about path slice from level to the + * end, the access index must be normalized by level. + */ + enum jbvType *tpath = palloc0((path_len - level) * sizeof(enum jbvType)); + long lindex; + JsonbValue newkey; + + /* + * Create first part of the chain with beginning tokens. For the current + * level WJB_BEGIN_OBJECT/WJB_BEGIN_ARRAY was already created, so start + * with the next one. + */ + for(int i = level + 1; i < path_len; i++) + { + char *c, *badp; + + if (path_nulls[i]) + break; + + /* + * Try to convert to an integer to find out the expected type, + * object or array. + */ + c = TextDatumGetCString(path_elems[i]); + errno = 0; + lindex = strtol(c, &badp, 10); + if (errno != 0 || badp == c || *badp != '\0' || lindex > INT_MAX || + lindex < INT_MIN) + { + /* text, an object is expected */ + newkey.type = jbvString; + newkey.val.string.len = VARSIZE_ANY_EXHDR(path_elems[i]); + newkey.val.string.val = VARDATA_ANY(path_elems[i]); + + (void) pushJsonbValue(st, WJB_BEGIN_OBJECT, NULL); + (void) pushJsonbValue(st, WJB_KEY, &newkey); + + tpath[i - level] = jbvObject; + } + else + { + /* integer, an array is expected */ + (void) pushJsonbValue(st, WJB_BEGIN_ARRAY, NULL); + + push_null_elements(st, lindex); + + tpath[i - level] = jbvArray; + } + + } + + /* Insert an actual value for either an object or array */ + if (tpath[(path_len - level) - 1] == jbvArray) + { + (void) pushJsonbValue(st, WJB_ELEM, newval); + } + else + (void) pushJsonbValue(st, WJB_VALUE, newval); + + /* + * Close everything up to the last but one level. The last one will be + * closed outside of this function. + */ + for(int i = path_len - 1; i > level; i--) + { + if (path_nulls[i]) + break; + + if (tpath[i - level] == jbvObject) + (void) pushJsonbValue(st, WJB_END_OBJECT, NULL); + else + (void) pushJsonbValue(st, WJB_END_ARRAY, NULL); + } +} + /* * Return the text representation of the given JsonbValue. */ @@ -4782,6 +4886,21 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, * Bits JB_PATH_INSERT_BEFORE and JB_PATH_INSERT_AFTER in op_type * behave as JB_PATH_CREATE if new value is inserted in JsonbObject. * + * If JB_PATH_FILL_GAPS bit is set, this will change an assignment logic in + * case if target is an array. The assignment index will not be restricted by + * number of elements in the array, and if there are any empty slots between + * last element of the array and a new one they will be filled with nulls. If + * the index is negative, it still will be considered an an index from the end + * of the array. Of a part of the path is not present and this part is more + * than just one last element, this flag will instruct to create the whole + * chain of corresponding objects and insert the value. + * + * JB_PATH_CONSISTENT_POSITION for an array indicates that the called wants to + * keep values with fixed indices. Indices for existing elements could be + * changed (shifted forward) in case if the array is prepended with a new value + * and a negative index out of the range, so this behavior will be prevented + * and return an error. + * * All path elements before the last must already exist * whatever bits in op_type are set, or nothing is done. */ @@ -4876,6 +4995,8 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, memcmp(k.val.string.val, VARDATA_ANY(path_elems[level]), k.val.string.len) == 0) { + done = true; + if (level == path_len - 1) { /* @@ -4895,7 +5016,6 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, (void) pushJsonbValue(st, WJB_KEY, &k); (void) pushJsonbValue(st, WJB_VALUE, newval); } - done = true; } else { @@ -4940,6 +5060,31 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, } } } + + /* + * If we got here there are only few possibilities: + * - no target path was found, and an open object with some keys/values was + * pushed into the state + * - an object is empty, only WJB_BEGIN_OBJECT is pushed + * + * In both cases if instructed to create the path when not present, + * generate the whole chain of empty objects and insert the new value + * there. + */ + if (!done && (op_type & JB_PATH_FILL_GAPS) && (level < path_len - 1)) + { + JsonbValue newkey; + + newkey.type = jbvString; + newkey.val.string.len = VARSIZE_ANY_EXHDR(path_elems[level]); + newkey.val.string.val = VARDATA_ANY(path_elems[level]); + + (void) pushJsonbValue(st, WJB_KEY, &newkey); + (void) push_path(st, level, path_elems, path_nulls, + path_len, newval); + + /* Result is closed with WJB_END_OBJECT outside of this function */ + } } /* @@ -4978,25 +5123,48 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, if (idx < 0) { if (-idx > nelems) - idx = INT_MIN; + { + /* + * If asked to keep elements position consistent, it's not allowed + * to prepend the array. + */ + if (op_type & JB_PATH_CONSISTENT_POSITION) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("path element at position %d is out of range: %d", + level + 1, idx))); + else + idx = INT_MIN; + } else idx = nelems + idx; } - if (idx > 0 && idx > nelems) - idx = nelems; + /* + * Filling the gaps means there are no limits on the positive index are + * imposed, we can set any element. Otherwise limit the index by nelems. + */ + if (!(op_type & JB_PATH_FILL_GAPS)) + { + if (idx > 0 && idx > nelems) + idx = nelems; + } /* * if we're creating, and idx == INT_MIN, we prepend the new value to the * array also if the array is empty - in which case we don't really care * what the idx value is */ - if ((idx == INT_MIN || nelems == 0) && (level == path_len - 1) && (op_type & JB_PATH_CREATE_OR_INSERT)) { Assert(newval != NULL); + + if (op_type & JB_PATH_FILL_GAPS && nelems == 0 && idx > 0) + push_null_elements(st, idx); + (void) pushJsonbValue(st, WJB_ELEM, newval); + done = true; } @@ -5007,6 +5175,8 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, if (i == idx && level < path_len) { + done = true; + if (level == path_len - 1) { r = JsonbIteratorNext(it, &v, true); /* skip */ @@ -5024,8 +5194,6 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, if (op_type & (JB_PATH_INSERT_AFTER | JB_PATH_REPLACE)) (void) pushJsonbValue(st, WJB_ELEM, newval); - - done = true; } else (void) setPath(it, path_elems, path_nulls, path_len, @@ -5053,14 +5221,42 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, (void) pushJsonbValue(st, r, r < WJB_BEGIN_ARRAY ? &v : NULL); } } - - if ((op_type & JB_PATH_CREATE_OR_INSERT) && !done && - level == path_len - 1 && i == nelems - 1) - { - (void) pushJsonbValue(st, WJB_ELEM, newval); - } } } + + if ((op_type & JB_PATH_CREATE_OR_INSERT) && !done && level == path_len - 1) + { + /* + * If asked to fill the gaps, idx could be bigger than nelems, + * so prepend the new element with nulls if that's the case. + */ + if (op_type & JB_PATH_FILL_GAPS && idx > nelems) + push_null_elements(st, idx - nelems); + + (void) pushJsonbValue(st, WJB_ELEM, newval); + done = true; + } + + /* + * If we got here there are only few possibilities: + * - no target path was found, and an open array with some keys/values was + * pushed into the state + * - an array is empty, only WJB_BEGIN_ARRAY is pushed + * + * In both cases if instructed to create the path when not present, + * generate the whole chain of empty objects and insert the new value + * there. + */ + if (!done && (op_type & JB_PATH_FILL_GAPS) && (level < path_len - 1)) + { + if (idx > 0) + push_null_elements(st, idx - nelems); + + (void) push_path(st, level, path_elems, path_nulls, + path_len, newval); + + /* Result is closed with WJB_END_OBJECT outside of this function */ + } } /* diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out index bb3f25ec3f..b7c268b53f 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -4999,6 +4999,141 @@ select * from test_jsonb_subscript; 3 | [1] (3 rows) +-- Fill the gaps logic +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[0]'); +update test_jsonb_subscript set test_json[5] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+-------------------------------- + 1 | [0, null, null, null, null, 1] +(1 row) + +update test_jsonb_subscript set test_json[-4] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+----------------------------- + 1 | [0, null, 1, null, null, 1] +(1 row) + +update test_jsonb_subscript set test_json[-8] = '1'; +ERROR: path element at position 1 is out of range: -8 +select * from test_jsonb_subscript; + id | test_json +----+----------------------------- + 1 | [0, null, 1, null, null, 1] +(1 row) + +-- keep consistent values position +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[]'); +update test_jsonb_subscript set test_json[5] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+----------------------------------- + 1 | [null, null, null, null, null, 1] +(1 row) + +-- create the whole path +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a'][0]['b'][0]['c'] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+---------------------------- + 1 | {"a": [{"b": [{"c": 1}]}]} +(1 row) + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a'][2]['b'][2]['c'][2] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+------------------------------------------------------------------ + 1 | {"a": [null, null, {"b": [null, null, {"c": [null, null, 1]}]}]} +(1 row) + +-- create the whole path with already existing keys +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"b": 1}'); +update test_jsonb_subscript set test_json['a'][0] = '2'; +select * from test_jsonb_subscript; + id | test_json +----+-------------------- + 1 | {"a": [2], "b": 1} +(1 row) + +-- the start jsonb is an object, first subscript is treated as a key +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json[0]['a'] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+----------------- + 1 | {"0": {"a": 1}} +(1 row) + +-- the start jsonb is an array +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[]'); +update test_jsonb_subscript set test_json[0]['a'] = '1'; +update test_jsonb_subscript set test_json[2]['b'] = '2'; +select * from test_jsonb_subscript; + id | test_json +----+---------------------------- + 1 | [{"a": 1}, null, {"b": 2}] +(1 row) + +-- overwriting an existing path +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a']['b'][1] = '1'; +update test_jsonb_subscript set test_json['a']['b'][10] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+---------------------------------------------------------------------------- + 1 | {"a": {"b": [null, 1, null, null, null, null, null, null, null, null, 1]}} +(1 row) + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[]'); +update test_jsonb_subscript set test_json[0][0][0] = '1'; +update test_jsonb_subscript set test_json[0][0][1] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+------------ + 1 | [[[1, 1]]] +(1 row) + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a']['b'][10] = '1'; +update test_jsonb_subscript set test_json['a'][10][10] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+------------------------------------------------------------------------------------------------------------------------------------------------------ + 1 | {"a": {"b": [null, null, null, null, null, null, null, null, null, null, 1], "10": [null, null, null, null, null, null, null, null, null, null, 1]}} +(1 row) + +-- an empty sub element +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": {}}'); +update test_jsonb_subscript set test_json['a']['b']['c'][2] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+-------------------------------------- + 1 | {"a": {"b": {"c": [null, null, 1]}}} +(1 row) + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": []}'); +update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1'; +select * from test_jsonb_subscript; + id | test_json +----+--------------------------------------- + 1 | {"a": [null, {"c": [null, null, 1]}]} +(1 row) + -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); to_tsvector diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql index 20aa8fe0e2..0320db0ea4 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1290,6 +1290,87 @@ update test_jsonb_subscript set test_json = NULL where id = 3; update test_jsonb_subscript set test_json[0] = '1'; select * from test_jsonb_subscript; +-- Fill the gaps logic +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[0]'); + +update test_jsonb_subscript set test_json[5] = '1'; +select * from test_jsonb_subscript; + +update test_jsonb_subscript set test_json[-4] = '1'; +select * from test_jsonb_subscript; + +update test_jsonb_subscript set test_json[-8] = '1'; +select * from test_jsonb_subscript; + +-- keep consistent values position +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[]'); + +update test_jsonb_subscript set test_json[5] = '1'; +select * from test_jsonb_subscript; + +-- create the whole path +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a'][0]['b'][0]['c'] = '1'; +select * from test_jsonb_subscript; + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a'][2]['b'][2]['c'][2] = '1'; +select * from test_jsonb_subscript; + +-- create the whole path with already existing keys +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"b": 1}'); +update test_jsonb_subscript set test_json['a'][0] = '2'; +select * from test_jsonb_subscript; + +-- the start jsonb is an object, first subscript is treated as a key +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json[0]['a'] = '1'; +select * from test_jsonb_subscript; + +-- the start jsonb is an array +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[]'); +update test_jsonb_subscript set test_json[0]['a'] = '1'; +update test_jsonb_subscript set test_json[2]['b'] = '2'; +select * from test_jsonb_subscript; + +-- overwriting an existing path +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a']['b'][1] = '1'; +update test_jsonb_subscript set test_json['a']['b'][10] = '1'; +select * from test_jsonb_subscript; + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '[]'); +update test_jsonb_subscript set test_json[0][0][0] = '1'; +update test_jsonb_subscript set test_json[0][0][1] = '1'; +select * from test_jsonb_subscript; + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{}'); +update test_jsonb_subscript set test_json['a']['b'][10] = '1'; +update test_jsonb_subscript set test_json['a'][10][10] = '1'; +select * from test_jsonb_subscript; + +-- an empty sub element + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": {}}'); +update test_jsonb_subscript set test_json['a']['b']['c'][2] = '1'; +select * from test_jsonb_subscript; + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": []}'); +update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1'; +select * from test_jsonb_subscript; + -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); -- 2.21.0 --2v7pubbjx3p2om6u Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v45-0003-Replace-assuming-a-composite-object-on-a-scalar.patch" ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks @ 2026-05-26 15:23 ` Laurenz Albe <[email protected]> 2026-05-27 17:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: Laurenz Albe @ 2026-05-26 15:23 UTC (permalink / raw) To: Alberto Piai <[email protected]>; pgsql-hackers On Thu, 2026-05-14 at 15:46 -0700, Alberto Piai wrote: > > > On Tue Mar 17, 2026 at 5:31 PM +07, Alberto Piai wrote: > > > > > > > I recently needed to add a stored generated column to a table of > > > > nontrivial size, and realized that currently there is no way to do > > > > that without rewriting the table under an AccessExclusiveLock. > > The attached v4 is a rebase against current master I understand the need that the patch fulfills, and I agree that it would be a nice feature. I have a few thoughts about this that don't concern the implementation: 1) The SQL standard knows ALTER TABLE ... ADD ... GENERATED ALWAYS AS (...) and ALTER TABLE ... ALTER ... DROP EXPRESSION, but there is no provision for ALTER TABLE ... ADD GENERATED ALWAYS AS (...). So this patch adds non-standard syntax that may one day conflict with a new version of the standard. I think we can still do it, and the proposed syntax looks right, but I thought I should mention it. 2) We currently have ALTER TABLE ... ALTER ... SET EXPRESSION AS (...) to change the generation expression of a column. This command always rewrites the table, according to the documentation. I think that if the present patch adds support to skip rewriting the table when a generation expression is added and the expression matches a check constraint, changing the generation expression should also be possible without a rewrite. If not, I would consider that a violation of the principle of least astonishment. Would it be difficult to extend the patch to support that? 3) We already have a couple of tricks to avoid blocking for a long time: - ALTER TABLE ... ALTER ... SET NOT NULL can skip the table scan if there is a check constraint that makes sure that the column is NOT NULL - ALTER TABLE ... ATTACH PARTITION can skip the scan of the new partition if there is a check constraint matching the partition constraint It would be great to document these little tricks in the documentation, probably on the ALTER TABLE page. This is not necessarily the job of this patch, but it would also not be off-topic for the patch. Comments on the patch: ---------------------- The patch applies and builds cleanly and passes the regression tests. Missing parts: - There is no documentation. At least ALTER TABLE needs a description of the new syntax, and would ideally mention the trick with the check constraint. - There should be support for command line completion for the new syntax. Bugs: - The patch doesn't test if the column is an identity column: CREATE TABLE tab (id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY); INSERT INTO tab VALUES (DEFAULT); ALTER TABLE tab ALTER id ADD GENERATED ALWAYS AS (1) STORED; The ALTER TABLE should fail, but doesn't. - Strange behavior with sequences owned by the column: CREATE TABLE tab (id bigserial); INSERT INTO tab VALUES (DEFAULT); ALTER TABLE tab ALTER id ADD GENERATED ALWAYS AS (1) STORED; \ds tab_id_seq List of sequences Schema | Name | Type | Owner --------+------------+----------+---------- public | tab_id_seq | sequence | postgres (1 row) I think that any sequence owned by the column should be dropped. Alternatively, you could throw an error. - Incorrect handling of NULL values: CREATE TABLE tab (col1 integer, col2 integer); INSERT INTO tab VALUES (2, NULL); -- works, because NULL results from the check are accepted ALTER TABLE tab ADD CHECK (col2 = col1); SELECT pg_relation_filenode('tab'); pg_relation_filenode ---------------------- 19920 (1 row) ALTER TABLE tab ALTER col2 ADD GENERATED ALWAYS AS (col1) STORED; SELECT pg_relation_filenode('tab'); pg_relation_filenode ---------------------- 19920 (1 row) TABLE tab; col1 | col2 ------+------ 2 | ∅ (1 row) I am not sure what the correct approach would be. The simple approach would be to only skip the rewrite if the column has a NOT NULL constraint or an equivalent check constraint, but perhaps you can think of a way to do better. Comments on the code: > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -5093,6 +5102,13 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, > ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context); > pass = AT_PASS_SET_EXPRESSION; > break; > + case AT_AddGeneratedAsExprStored: You should add a comment, same as for the other branches. > @@ -6695,6 +6717,8 @@ alter_table_type_to_string(AlterTableType cmdtype) > return "ALTER COLUMN ... SET NOT NULL"; > case AT_SetExpression: > return "ALTER COLUMN ... SET EXPRESSION"; > + case AT_AddGeneratedAsExprStored: > + return "ALTER COLUMN ... ADD GENERATED ALWAYS AS (...) STORED"; Keep it short, like "ALTER COLUMN ... ADD GENERATED". > --- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c > +++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c > @@ -129,6 +129,9 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS) > case AT_SetNotNull: > strtype = "SET NOT NULL"; > break; > + case AT_AddGeneratedAsExprStored: > + strtype = "ADD GENERATED ALWAYS AS (...) STORED"; > + break; I suggest "ALTER COLUMN ADD GENERATED ALWAYS AS", but I won't insist. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks 2026-05-26 15:23 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> @ 2026-05-27 17:44 ` Alberto Piai <[email protected]> 2026-06-15 20:41 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Alberto Piai @ 2026-05-27 17:44 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; Alberto Piai <[email protected]>; pgsql-hackers On Tue May 26, 2026 at 5:23 PM CEST, Laurenz Albe wrote: > > 1) The SQL standard knows ALTER TABLE ... ADD ... GENERATED ALWAYS AS (...) > and ALTER TABLE ... ALTER ... DROP EXPRESSION, but there is no provision > for ALTER TABLE ... ADD GENERATED ALWAYS AS (...). > So this patch adds non-standard syntax that may one day conflict with > a new version of the standard. I think we can still do it, and the > proposed syntax looks right, but I thought I should mention it. Thank you for bringing this up. I don't have access to the standard, but the chance of a possible conflict with future editions was at the back of my mind. I don't see a way to exclude it completely. In a sibling mail in this thread (you should be in CC), I have made a new iteration on this proposal, which also tries to make the command more specific to avoid future conflicts. > 2) We currently have ALTER TABLE ... ALTER ... SET EXPRESSION AS (...) to > change the generation expression of a column. This command always > rewrites the table, according to the documentation. > I think that if the present patch adds support to skip rewriting the table > when a generation expression is added and the expression matches a check > constraint, changing the generation expression should also be possible > without a rewrite. If not, I would consider that a violation of the > principle of least astonishment. > Would it be difficult to extend the patch to support that? Yes, I don't see a way to make that work. Since we're talking only about stored values, a rewrite will always be necessary. However, using this new command, a user could add a column with the new expression, then atomically drop the old one and rename. All without holding onto an AccessExclusiveLock for a long time :) > 3) We already have a couple of tricks to avoid blocking for a long time: > > - ALTER TABLE ... ALTER ... SET NOT NULL can skip the table scan if there > is a check constraint that makes sure that the column is NOT NULL > > - ALTER TABLE ... ATTACH PARTITION can skip the scan of the new partition > if there is a check constraint matching the partition constraint > > It would be great to document these little tricks in the documentation, > probably on the ALTER TABLE page. This is not necessarily the job of > this patch, but it would also not be off-topic for the patch. The SET NOT NULL one and the ATTACH PARTITION one are documented in the section specific to the command. However or, if an equivalent index already exists, it will be attached to the target table's index, as if ALTER INDEX ATTACH PARTITION had been executed is not very explicit about the advantages this has for online migrations. In the NOTES section of the ALTER TABLE page, there is a paragraph about NOT VALID / VALIDATE, which is another operation in the same spirit as this. Maybe we could group them all in a new section dedicated to online schema migrations? (However, even if it's definitely on-topic with this patch, I would work on this in a separate patch / email thread.) > Missing parts: > > - There is no documentation. At least ALTER TABLE needs a description of the > new syntax, and would ideally mention the trick with the check constraint. Yes, I will work on this next. I also believe it's a great way to show a feature, even early during development. I just wanted to avoid doing it _too_ early, before having had any feedback about the idea. > - There should be support for command line completion for the new syntax. Great idea, I'll add this too. > Bugs: > > - The patch doesn't test if the column is an identity column: > > CREATE TABLE tab (id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY); > INSERT INTO tab VALUES (DEFAULT); > ALTER TABLE tab ALTER id ADD GENERATED ALWAYS AS (1) STORED; > > The ALTER TABLE should fail, but doesn't. > > - Strange behavior with sequences owned by the column: > > CREATE TABLE tab (id bigserial); > INSERT INTO tab VALUES (DEFAULT); > ALTER TABLE tab ALTER id ADD GENERATED ALWAYS AS (1) STORED; > \ds tab_id_seq > List of sequences > Schema | Name | Type | Owner > --------+------------+----------+---------- > public | tab_id_seq | sequence | postgres > (1 row) > > I think that any sequence owned by the column should be dropped. > Alternatively, you could throw an error. Thanks for testing this! I have reused RememberAllDependentForRebuilding() which does some validation, but was originally meant for ALTER COLUMN TYPE. I will add checks and tests for these cases, but to be consistent with how the other dependencies are handled, I think it's better to throw an error here (this is what happens for example if trying to ALTER TYPE of a column used by a function). > > - Incorrect handling of NULL values: See sibling mail, in the next iteration the constraint will have to use IS NOT DISTINCT FROM. I think that should cover all cases. > > Comments on the code: > >> --- a/src/backend/commands/tablecmds.c >> +++ b/src/backend/commands/tablecmds.c >> @@ -5093,6 +5102,13 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, >> ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context); >> pass = AT_PASS_SET_EXPRESSION; >> break; >> + case AT_AddGeneratedAsExprStored: > > You should add a comment, same as for the other branches. > >> @@ -6695,6 +6717,8 @@ alter_table_type_to_string(AlterTableType cmdtype) >> return "ALTER COLUMN ... SET NOT NULL"; >> case AT_SetExpression: >> return "ALTER COLUMN ... SET EXPRESSION"; >> + case AT_AddGeneratedAsExprStored: >> + return "ALTER COLUMN ... ADD GENERATED ALWAYS AS (...) STORED"; > > Keep it short, like "ALTER COLUMN ... ADD GENERATED". > >> --- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c >> +++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c >> @@ -129,6 +129,9 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS) >> case AT_SetNotNull: >> strtype = "SET NOT NULL"; >> break; >> + case AT_AddGeneratedAsExprStored: >> + strtype = "ADD GENERATED ALWAYS AS (...) STORED"; >> + break; > > I suggest "ALTER COLUMN ADD GENERATED ALWAYS AS", but I won't insist. Agreed, will fix all these in the next version of the patch. Thank you again for the review! Best regards, Alberto -- Alberto Piai Sensational AG Zürich, Switzerland ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks 2026-05-26 15:23 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 2026-05-27 17:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> @ 2026-06-15 20:41 ` Laurenz Albe <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Laurenz Albe @ 2026-06-15 20:41 UTC (permalink / raw) To: Alberto Piai <[email protected]>; pgsql-hackers On Wed, 2026-05-27 at 19:44 +0200, Alberto Piai wrote: > On Tue May 26, 2026 at 5:23 PM CEST, Laurenz Albe wrote: > > > > 2) We currently have ALTER TABLE ... ALTER ... SET EXPRESSION AS (...) to > > change the generation expression of a column. This command always > > rewrites the table, according to the documentation. > > I think that if the present patch adds support to skip rewriting the table > > when a generation expression is added and the expression matches a check > > constraint, changing the generation expression should also be possible > > without a rewrite. If not, I would consider that a violation of the > > principle of least astonishment. > > Would it be difficult to extend the patch to support that? > > Yes, I don't see a way to make that work. Since we're talking only about > stored values, a rewrite will always be necessary. However, using this > new command, a user could add a column with the new expression, then > atomically drop the old one and rename. All without holding onto an > AccessExclusiveLock for a long time :) With your new proposal to never rewrite the table, but fail instead if there is no constraint, my objection loses its point, so I withdraw it. > > 3) We already have a couple of tricks to avoid blocking for a long time: > > > > - ALTER TABLE ... ALTER ... SET NOT NULL can skip the table scan if there > > is a check constraint that makes sure that the column is NOT NULL > > > > - ALTER TABLE ... ATTACH PARTITION can skip the scan of the new partition > > if there is a check constraint matching the partition constraint > > > > It would be great to document these little tricks in the documentation, > > probably on the ALTER TABLE page. This is not necessarily the job of > > this patch, but it would also not be off-topic for the patch. > > The SET NOT NULL one and the ATTACH PARTITION one are documented in the > section specific to the command. However > > or, if an equivalent index already exists, it will be attached to the > target table's index, as if ALTER INDEX ATTACH PARTITION had been > executed > > is not very explicit about the advantages this has for online > migrations. > > In the NOTES section of the ALTER TABLE page, there is a paragraph about > NOT VALID / VALIDATE, which is another operation in the same spirit as > this. > > Maybe we could group them all in a new section dedicated to online > schema migrations? You are right, the existing shortcuts are documented. Your new proposal makes the proposed feature different from these existing cases, so I don't think lumping them together is a good idea now. > Agreed, will fix all these in the next version of the patch. Great; I'm looking forward to it. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks @ 2026-05-27 17:43 ` Alberto Piai <[email protected]> 2026-06-15 18:38 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: Alberto Piai @ 2026-05-27 17:43 UTC (permalink / raw) To: Alberto Piai <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]>; Laurenz Albe <[email protected]> Hi, On Fri May 15, 2026 at 12:46 AM CEST, Alberto Piai wrote: > On Fri Apr 24, 2026 at 2:10 AM PDT, Alberto Piai wrote: >> On Tue Apr 7, 2026 at 5:02 PM +08, Alberto Piai wrote: >>> On Tue Mar 17, 2026 at 5:31 PM +07, Alberto Piai wrote: >>> >>>> I recently needed to add a stored generated column to a table of >>>> nontrivial size, and realized that currently there is no way to do >>>> that without rewriting the table under an AccessExclusiveLock. here's a not-so-brief summary of the conversations around this topic at pgconf.dev, and a new proposal at the end. I had the chance to bring this up with other attendees, and many recognized the use case as a useful one, addressing a real operational issue. In particular, I had great feedback from Staš Kotarac Guček, who pointed out a major flaw in my current proposal: a constraint of the form CHECK (c = expr) would not work correctly when expr evaluates to null for some rows. Thank you Staš, in the next iteration I will change the constraint to use IS NOT DISTINCT FROM, instead. I briefly mentioned this topic to Tom Lane, who quickly replied with the question: should this not fail when it can't use the constraint, instead of overwriting the contents of the column? Thanks Tom, I will get to this later in this mail. I had registered this patch for the in-person commitfest at pgconf.dev, and Álvaro Herrera picked it up for review. Thank you Álvaro, and thank you Peter for organizing the event. We managed to find some time on the very last day of the conference, and went through the current design and code. The open items (which I will address in the next iteration of this patch) are: * missing user documentation I will work on this next. I think it's a good way to explain the feature even early during development. I just didn't want to do it _too_ early, without having had any feedback. * try to minimize command counter increments There might be one call to CommandCounterIncrement() which is not necessary, I'll try remove it. * comment on why it is necessary to clear missing values when rewriting the table ATExecAlterColumnType() and ATExecSetExpression() both do this explicitly when requesting a table rewrite. I'll extend the comment, and also look into whether this is something that should be done any time a table rewrite happens. In that case, it might be worth moving this into the rewriting code rather than having each caller do it. * interactions with other subcommands in the same alter table statement My reasoning regarding this was: if I do this in AT_PASS_SET_EXPRESSION, it should be safe. I will invest some more time into this and add tests, too. We also looked at the overall design of the new command, and we agreed that it is a fitting addition to our current SET EXPRESSION and DROP EXPRESSION. Regarding the question of whether it should be SET or ADD, we agreed that ADD (i.e. the current proposal) is clearer, especially for its similarity to ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY. Regarding the question of "should this fail or rewrite the table when a usable constraint isn't found": Álvaro's suggestion here was to use a more ad-hoc command, meant more specifically for this use case of converting into a stored generated column without rewriting it. If the command would be dedicated specifically to this, it would make sense to have it fail when a usable constraint isn't found. Last but not least, I also discussed this with Laurenz Albe, and he wrote a very useful review in this thread. I will address that separately and reply directly to that mail, but one point I can already merge in this discussion is about the syntax of the command: > 1) The SQL standard knows ALTER TABLE ... ADD ... GENERATED ALWAYS AS (...) > and ALTER TABLE ... ALTER ... DROP EXPRESSION, but there is no provision > for ALTER TABLE ... ADD GENERATED ALWAYS AS (...). > So this patch adds non-standard syntax that may one day conflict with > a new version of the standard. I think we can still do it, and the > proposed syntax looks right, but I thought I should mention it. I'd like to take his point, together with the question from Tom and the suggestion by Álvaro, and make a new proposal for the design of this command. Design iteration 2 ------------------ Syntax: ALTER TABLE t ALTER COLUMN c ADD GENERATED ALWAYS AS (expr) STORED USING CONSTRAINT check_name check_name must be a valid constraint of the form CHECK (c IS NOT DISTINCT FROM (expr)) This fails if: - a check constraint named check_name is not found for table c - the constraint is not valid - the constraint does not match exactly the expr the user intends to use as a stored default expression On success, the table c is now a stored generated column with the given default expression, and the check_name constraint has been removed. This addresses Tom's remark, we can now fail instead of just rewriting the column. It improves slightly upon the issue of a potential conflict with a future edition of the SQL standard, by being more specific. I don't see a way to be completely sure we won't have conflicts. We could improve more by making the syntax more "alien" and very unlinkely to be picked up by the standard, but at a usability cost for Postgres. I'm open to suggestions. It improves upon another question raised by Álvaro: does the user have to clean up the constraint? In v1 I felt it was better to have the user remove it after the migration. Since here it's explicitly mentioned as the constraint to use to migrate the column, I think it's OK to remove it. We are conceptually moving it from being a constraint to being the new default expression. The implementation should also be simpler, since there will never be a table rewrite. Any thoughts about this? Best regards, Alberto -- Alberto Piai Sensational AG Zürich, Switzerland ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks 2026-05-27 17:43 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> @ 2026-06-15 18:38 ` Laurenz Albe <[email protected]> 2026-06-30 13:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Laurenz Albe @ 2026-06-15 18:38 UTC (permalink / raw) To: Alberto Piai <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]> On Wed, 2026-05-27 at 19:43 +0200, Alberto Piai wrote: > I'd like to take his point, together with the question from Tom and the > suggestion by Álvaro, and make a new proposal for the design of this > command. > > Design iteration 2 > ------------------ > > Syntax: > > ALTER TABLE t ALTER COLUMN c > ADD GENERATED ALWAYS AS (expr) STORED USING CONSTRAINT check_name > > check_name must be a valid constraint of the form > > CHECK (c IS NOT DISTINCT FROM (expr)) > > This fails if: > > - a check constraint named check_name is not found for table c > - the constraint is not valid > - the constraint does not match exactly the expr the user intends to use > as a stored default expression > > On success, the table c is now a stored generated column with the given > default expression, and the check_name constraint has been removed. > > This addresses Tom's remark, we can now fail instead of just rewriting > the column. I like this proposal. It avoids the question "to rewrite or not to rewrite" by just outright failing if there is no suitable constraint. The idea to avoid the problem with NULL by forcing IS NOT DISTINCT FROM in the constraint is a good solution. Perhaps you could also allow the equality operator if the column in question is defined NOT NULL. > It improves slightly upon the issue of a potential conflict with a > future edition of the SQL standard, by being more specific. I don't see > a way to be completely sure we won't have conflicts. We could improve > more by making the syntax more "alien" and very unlinkely to be picked > up by the standard, but at a usability cost for Postgres. I'm open to > suggestions. I don't think that the new proposal makes it less likely to get in conflict with later additions to the standard. But I don't think that inventing unlikely syntax to avoid such conflicts is a good idea. First, the standard committee itself seems (or seemed) to have a strong predilection for alien, verbose syntax. Second, if we end up with weird, unwieldy syntax, that would be bad. No, the syntax you are proposing sounds reasonable to me. > It improves upon another question raised by Álvaro: does the user have > to clean up the constraint? In v1 I felt it was better to have the user > remove it after the migration. Since here it's explicitly mentioned as > the constraint to use to migrate the column, I think it's OK to remove > it. We are conceptually moving it from being a constraint to being the > new default expression. > > The implementation should also be simpler, since there will never be a > table rewrite. > > Any thoughts about this? Yes. I think that you should not drop the constraint. That's what I'd expect, similar to how we don't drop the check constraint that allows to skip the table scan in ALTER TABLE ... ALTER COLUMN ... SET NOT NULL or ALTER TABLE ... ATTACK PARTITION. I feel that automatically dropping the constraint is a bit too much black magic, but it is more a feeling than a conviction. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks 2026-05-27 17:43 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-06-15 18:38 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> @ 2026-06-30 13:44 ` Alberto Piai <[email protected]> 2026-07-03 06:42 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Alberto Piai @ 2026-06-30 13:44 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; Alberto Piai <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]> On Mon Jun 15, 2026 at 8:38 PM CEST, Laurenz Albe wrote: > On Wed, 2026-05-27 at 19:43 +0200, Alberto Piai wrote: >> Design iteration 2 >> ------------------ > > I like this proposal. It avoids the question "to rewrite or not to > rewrite" by just outright failing if there is no suitable constraint. > > The idea to avoid the problem with NULL by forcing IS NOT DISTINCT FROM > in the constraint is a good solution. Perhaps you could also allow > the equality operator if the column in question is defined NOT NULL. 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. Design iteration 3 ------------------ Syntax: ALTER TABLE t ALTER COLUMN c ADD GENERATED ALWAYS STORED USING CONSTRAINT check_name check_name must be a valid constraint of a specific shape. If the column is nullable: CHECK (c IS NOT DISTINCT FROM expr) If the column is NOT NULL, either of the following is acceptable: CHECK (c = expr) CHECK (c IS NOT DISTINCT FROM expr) The column is then changed to be a stored generated column, with the "expr" from the constraint as its generator expression. >> Any thoughts about this? > > Yes. I think that you should not drop the constraint. That's what I'd > expect, similar to how we don't drop the check constraint that allows > to skip the table scan in ALTER TABLE ... ALTER COLUMN ... SET NOT > NULL or ALTER TABLE ... ATTACK PARTITION. > > 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. 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. Failure conditions ------------------ There's quite a few invalid states that cannot be reached via CREATE TABLE and should not be reachable via ALTER TABLE either. The following are detected and fail the operation: - the column is already a generated column - the column is an identity column - the column is referenced by a sequence (it is most likely a serial column) - the column is referenced by another column's default expression - the column references another generated column - the column is referenced in a partition key, either directly or through a whole row expression - the new default expression is not immutable Additionally, we of course bail if the constraint is not found, not valid, not enforced or doesn't match the specific structure we need. Another case I considered is the column being referenced in the body of a pre-parsed function (BEGIN ATOMIC SQL functions). In this case though, it seems to me that we don't need to fail here: we are not altering the type of the column, and when reading a stored generated column there's no expression replacement happening (as it does when reading virtual columns). Partitioning/inheritance is supported only on the whole hierarchy at once (see 8bf6ec3ba3a44448817af47a080587f3b71bee08). Trying to change the column at only one level will fail, as well as any of the subtrees. I also added a test to explictily check that we're not accidentally enqueuing a table scan for verification in phase 3, as avoiding this kind of work is the whole point of the command. Logical replication ------------------- The interaction with logical replication is tricky, since a publication can have the option to publish generated columns or not (which is the default). When not publishing stored generated columns, inserts or updates would be replicated while backfilling, and would then suddenly stop when the column is turned into a stored generated column. One way to avoid this is to set up triggers on the subscriber too, before altering the column on the publisher. This way updates and inserts would not lose the column's value on the subscriber, which can then be migrated by using the new alter table command. When publishing stored generated columns instead, it is not possible to have the same column be stored generated on both the publisher and the subscriber (see Table 29.2 in section 29.6. Generated Column Replication). The only supported configuration has a regular column on the side of the subscriber. (Note that this is not specific to this new command.) This makes this scenario a lot easier: the column is migrated on the publisher only, and the subscriber won't lose any value. To test these two scenarios, I wrote TAP tests for the subscription suite. However, I'm inclined to not add them to the test suite. I have attached them to this email separately. Other changes since v4 ---------------------- I have changed phase 2 to be ran at AT_PASS_ADD_OTHERCONSTR, before it was at AT_PASS_SET_EXPRESSION. The reason to do it there was to reuse the cleanup steps in ATPostAlterTypeCleanup when a table rewrite did happen. But since now never rewrite, this is not necessary anymore. Looking forward to your thoughts on this! Alberto -- Alberto Piai Sensational AG Zürich, Switzerland ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks 2026-05-27 17:43 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-06-15 18:38 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 2026-06-30 13:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> @ 2026-07-03 06:42 ` Alberto Piai <[email protected]> 2026-07-07 18:16 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Alberto Piai @ 2026-07-03 06:42 UTC (permalink / raw) To: Alberto Piai <[email protected]>; Laurenz Albe <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]> v5 had a rather brittle test that was relying on a DEBUG message being emitted when phase 3 verifies or rewrites a table (via client_min_messages). That was failing on the CI because the output depends on any other setting that might affect logging (in this case it was log_statement). The attached v6 replaces it with an injection_point test. If adding new injection points to detect scans/rewrites is considered too much, I can back it out and set/reset log_statement too. But I do prefer the injection_point test. If the new injection point is good, there are also a couple more tests which might be updated to use it. Regards, Alberto -- Alberto Piai Sensational AG Zürich, Switzerland ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Adding a stored generated column without long-lived locks 2026-05-27 17:43 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-06-15 18:38 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 2026-06-30 13:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-07-03 06:42 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> @ 2026-07-07 18:16 ` Laurenz Albe <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Laurenz Albe @ 2026-07-07 18:16 UTC (permalink / raw) To: Alberto Piai <[email protected]>; pgsql-hackers; +Cc: Álvaro Herrera <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-07-07 18:16 UTC | newest] Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-12-31 14:19 [PATCH v45 2/3] Filling gaps in jsonb Dmitrii Dolgov <[email protected]> 2026-05-26 15:23 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 2026-05-27 17:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-06-15 20:41 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 2026-05-27 17:43 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-06-15 18:38 ` Re: Adding a stored generated column without long-lived locks Laurenz Albe <[email protected]> 2026-06-30 13:44 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-07-03 06:42 ` Re: Adding a stored generated column without long-lived locks Alberto Piai <[email protected]> 2026-07-07 18:16 ` 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