| From: | Ewan Young <kdbase(dot)hack(at)gmail(dot)com> |
|---|---|
| To: | Alvaro Herrera <alvherre(at)kurilemu(dot)de> |
| Cc: | Antonin Houska <ah(at)cybertec(dot)at>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, mihailnikalayeu(at)gmail(dot)com |
| Subject: | Re: REPACK CONCURRENTLY fails on tables with generated columns |
| Date: | 2026-07-03 14:20:42 |
| Message-ID: | CAON2xHMa=+oeNF4UOwrGnT_w7sMuRkg5nmjTKgwSaDz_T1kAjA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks, Álvaro.
On Fri, Jul 3, 2026 at 7:12 PM Alvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> Hello,
>
> On 2026-Jun-22, Ewan Young wrote:
>
> > I applied the patch and ran it through an injection-point reproducer
> > (cassert). Without the fix the bug reproduces (ERROR: no generation
> > expression found for column number 3 ...); with it, REPACK CONCURRENTLY
> > succeeds under a concurrent non-HOT UPDATE for a STORED generated column, an
> > index directly on the generated column, and a VIRTUAL column, with correct
> > values afterwards. Your repack.spec change passes.
> >
> > The approach is right and I've confirmed it fixes the bug, so +1 from me in
> > this direction.
>
> Cool, thanks for reviewing -- I have pushed this fix, with some
> stylistic changes and one bigger change: these catalog rows are only
> needed in concurrent mode, so there was no reason to copy them in the
> other case. So I restricted the copying to that case.
>
> I've been looking at the other proposed change, and I agree with it.
> Here's it, again with some style changes, and only one other proposed
> change: for setting up updatedCols, ignore dropped columns. I don't
> think this should change anything in practice, but it just feels wrong
> to claim that a dropped column is being changed by an update.
I read through the range-table change and it looks correct.
Agreed on skipping dropped columns — a dropped attribute can never be
an index key column, so its presence in updatedCols has no functional effect;
leaving it out is a pure clarity win.
Over-claiming "all columns updated" is the safe direction: it only
affects the btree
indexUnchanged / bottom-up-deletion hint, never correctness. And for the
concurrent-apply path losing that hint should rarely matter, so erring
this way looks right.
+1 to commit.
>
> --
> Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
--
Regards,
Ewan Young
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nazir Bilal Yavuz | 2026-07-03 14:26:30 | ci: namespace ccache by PostgreSQL major version |
| Previous Message | Tom Lane | 2026-07-03 14:15:02 | Re: updates for handling optional argument in system functions |