Horton

Thread

Commits

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

  2. Unlogged sequences

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

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-05T15:51:05Z

    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)

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-02-08T06:04:47Z

    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

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

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-09T07:19:55Z

    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.