Fix propagation of persistence to sequences in ALTER TABLE / ADD COLUMN
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+33-2
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+33-2
0002-Additional-tests-20240208.patchtext/x-patch; charset=US-ASCII; name=0002-Additional-tests-20240208.patchDownload+67-1
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.