On Mon, Feb 5, 2024 at 9:21 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
>
> Commit 344d62fb9a9 (2022) introduced unlogged sequences and made it so
> that identity/serial sequences automatically get the persistence level
> of their owning table. But this works only for CREATE TABLE and not for
> ALTER TABLE / ADD COLUMN. This patch fixes that. (should be
> backpatched to 15, 16)
The patch looks ok.
+ seqstmt->sequence->relpersistence = cxt->rel ?
cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
+
This condition looks consistent with the other places in the code
around line 435, 498. But I was worried that cxt->rel may not get
latest relpersistence if the ALTER TABLE changes persistence as well.
Added a test (0002) which shows that ctx->rel has up-to-date
relpersistence. Also added a few other tests. Feel free to
include/reject them while committing.
0001 - same as your patch
0002 - additional tests
--
Best Wishes,
Ashutosh Bapat
From c142bfc5ec8b3c9c5a01f48693118d132145b45b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 5 Feb 2024 11:22:01 +0100
Subject: [PATCH 1/2] Fix propagation of persistence to sequences in ALTER
TABLE / ADD COLUMN
Fix for 344d62fb9a9: That commit introduced unlogged sequences and
made it so that identity/serial sequences automatically get the
persistence level of their owning table. But this works only for
CREATE TABLE and not for ALTER TABLE / ADD COLUMN. This is fixed
here.
TODO: backpatch to 15, 16
---
src/backend/parser/parse_utilcmd.c | 11 ++++++++++-
src/test/regress/expected/identity.out | 17 +++++++++++++++++
src/test/regress/sql/identity.sql | 6 ++++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 56ac4f516e..c7efd8d8ce 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -459,7 +459,16 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
seqstmt = makeNode(CreateSeqStmt);
seqstmt->for_identity = for_identity;
seqstmt->sequence = makeRangeVar(snamespace, sname, -1);
- seqstmt->sequence->relpersistence = cxt->relation->relpersistence;
+
+ /*
+ * Copy the persistence of the table. For CREATE TABLE, we get the
+ * persistence from cxt->relation, which comes from the CreateStmt in
+ * progress. For ALTER TABLE, the parser won't set
+ * cxt->relation->relpersistence, but we have cxt->rel as the existing
+ * table, so we copy the persistence from there.
+ */
+ seqstmt->sequence->relpersistence = cxt->rel ? cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
+
seqstmt->options = seqoptions;
/*
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 08f95674ca..84b59dca13 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -365,6 +365,23 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl
ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error
ERROR: identity column type must be smallint, integer, or bigint
+-- check that unlogged propagates to sequence
+CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
+ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+\d itest17
+ Unlogged table "public.itest17"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+------------------------------
+ a | integer | | not null | generated always as identity
+ b | text | | |
+
+\d itest17_a_seq
+ Unlogged sequence "public.itest17_a_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest17.a
+
-- kinda silly to change property in the same command, but it should work
ALTER TABLE itest3
ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY,
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 9f35214751..7596f0a36f 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -214,6 +214,12 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl
ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error
+-- check that unlogged propagates to sequence
+CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
+ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+\d itest17
+\d itest17_a_seq
+
-- kinda silly to change property in the same command, but it should work
ALTER TABLE itest3
ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY,
--
2.25.1
From a29aaa16cda9ee70829012a676a14c556cf236c5 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Thu, 8 Feb 2024 11:26:00 +0530
Subject: [PATCH 2/2] Additional tests.
---
src/test/regress/expected/identity.out | 55 ++++++++++++++++++++++++++
src/test/regress/sql/identity.sql | 12 ++++++
2 files changed, 67 insertions(+)
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 84b59dca13..4c45837f66 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -368,12 +368,16 @@ ERROR: identity column type must be smallint, integer, or bigint
-- check that unlogged propagates to sequence
CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+ALTER TABLE itest17 ADD COLUMN c int GENERATED ALWAYS AS IDENTITY;
+CREATE TABLE itest18 (a int NOT NULL, b text);
+ALTER TABLE itest18 SET UNLOGGED, ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
\d itest17
Unlogged table "public.itest17"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+------------------------------
a | integer | | not null | generated always as identity
b | text | | |
+ c | integer | | not null | generated always as identity
\d itest17_a_seq
Unlogged sequence "public.itest17_a_seq"
@@ -382,6 +386,57 @@ ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
integer | 1 | 1 | 2147483647 | 1 | no | 1
Sequence for identity column: public.itest17.a
+\d itest17_c_seq
+ Unlogged sequence "public.itest17_c_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest17.c
+
+\d itest18
+ Unlogged table "public.itest18"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+------------------------------
+ a | integer | | not null | generated always as identity
+ b | text | | |
+
+\d itest18_a_seq
+ Unlogged sequence "public.itest18_a_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest18.a
+
+ALTER TABLE itest18 SET LOGGED;
+\d itest18
+ Table "public.itest18"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+------------------------------
+ a | integer | | not null | generated always as identity
+ b | text | | |
+
+\d itest18_a_seq
+ Sequence "public.itest18_a_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest18.a
+
+ALTER TABLE itest18 SET UNLOGGED;
+\d itest18
+ Unlogged table "public.itest18"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+------------------------------
+ a | integer | | not null | generated always as identity
+ b | text | | |
+
+\d itest18_a_seq
+ Unlogged sequence "public.itest18_a_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest18.a
+
-- kinda silly to change property in the same command, but it should work
ALTER TABLE itest3
ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY,
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 7596f0a36f..8c76fa1ceb 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -217,8 +217,20 @@ ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error
-- check that unlogged propagates to sequence
CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+ALTER TABLE itest17 ADD COLUMN c int GENERATED ALWAYS AS IDENTITY;
+CREATE TABLE itest18 (a int NOT NULL, b text);
+ALTER TABLE itest18 SET UNLOGGED, ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
\d itest17
\d itest17_a_seq
+\d itest17_c_seq
+\d itest18
+\d itest18_a_seq
+ALTER TABLE itest18 SET LOGGED;
+\d itest18
+\d itest18_a_seq
+ALTER TABLE itest18 SET UNLOGGED;
+\d itest18
+\d itest18_a_seq
-- kinda silly to change property in the same command, but it should work
ALTER TABLE itest3
--
2.25.1
