Fix propagation of persistence to sequences in ALTER TABLE / ADD COLUMN

Started by Peter Eisentrautalmost 2 years ago3 messages

peter@eisentraut.org

almost 2 years ago

1 attachment(s)

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)

Attachments:

0001-Fix-propagation-of-persistence-to-sequences-in-ALTER.patchtext/plain; charset=UTF-8; name=0001-Fix-propagation-of-persistence-to-sequences-in-ALTER.patchDownload

From a3cb389a3aaaea8df9371ef389d9d61ecede3713 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 5 Feb 2024 11:22:01 +0100
Subject: [PATCH] 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 56ac4f516ea..c7efd8d8cee 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 08f95674ca8..84b59dca13f 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 9f35214751c..7596f0a36f8 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -214,6 +214,12 @@ CREATE TABLE itest5 (a serial, b text);
 
 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.43.0

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

almost 2 years ago

In reply to: Peter Eisentraut (#1)

2 attachment(s)

Re: Fix propagation of persistence to sequences in ALTER TABLE / ADD COLUMN

On Mon, Feb 5, 2024 at 9:21 PM Peter Eisentraut <peter@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

Attachments:

0001-Fix-propagation-of-persistence-to-sequences-20240208.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-propagation-of-persistence-to-sequences-20240208.patchDownload

From c142bfc5ec8b3c9c5a01f48693118d132145b45b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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

0002-Additional-tests-20240208.patchtext/x-patch; charset=US-ASCII; name=0002-Additional-tests-20240208.patchDownload

From a29aaa16cda9ee70829012a676a14c556cf236c5 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@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

Peter Eisentraut

peter@eisentraut.org

almost 2 years ago

In reply to: Ashutosh Bapat (#2)

Re: Fix propagation of persistence to sequences in ALTER TABLE / ADD COLUMN

On 08.02.24 07:04, Ashutosh Bapat wrote:

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.

Ah good, that pattern already existed.

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.

Yes, this additional coverage seems good. Committed with your additions.