| From: | Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | Baji Shaik <baji(dot)pgdev(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations |
| Date: | 2026-05-28 14:54:07 |
| Message-ID: | ahhVcOk20rDXD1gl@alvherre.pgsql |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On 2026-May-27, Chao Li wrote:
> > On May 27, 2026, at 11:06, Baji Shaik <baji(dot)pgdev(at)gmail(dot)com> wrote:
> >
> > 0001 -- When wal_level < replica, REPACK (CONCURRENTLY) currently
> > surfaces generic "replication slots ... wal_level" error
> > from CheckSlotRequirements(), with a CONTEXT line referring
> > to an internal worker. Add an upfront check that reports a
> > REPACK-specific error.
>
> LGTM
Pushed this one earlier. I changed the errcode though, because in my
mind "object" is a database object, and the server configuration is not
an object. So I used INVALID_PARAMETER_VALUE instead. I also don't
think it makes sense to say "cannot repack table X", so the user leaves
thinking they could repack table Y instead. The whole point being that
you cannot vacuum _any_ tables. So I made the errmsg() say that.
> When I was working on 832e220d99a, I actually considered for more
> detailed error messages, but I ended up giving up. I think we should
> be careful about adding more branches here unless the existing message
> is causing significant confusion in practice.
>
> So, I personally don’t like 0002.
I'll give this a look after some icecream.
> > 0003 -- Four ereport(ERROR) calls in the REPACK CONCURRENTLY code
> > path lack errcode() and default to ERRCODE_INTERNAL_ERROR.
> > Add appropriate errcodes; in particular, the
> > apply_concurrent_update/delete failures map cleanly to
> > ERRCODE_T_R_SERIALIZATION_FAILURE.
Also pushed, with additional editorialization. I have recollections of
out message policy saying something about "could not do X" instead of
"failed to do X", so I changed it that way. (But I couldn't find that
in the style guide.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andres Freund | 2026-05-28 15:07:22 | Re: Heads Up: cirrus-ci is shutting down June 1st |
| Previous Message | vignesh C | 2026-05-28 14:49:14 | Re: Proposal: Conflict log history table for Logical Replication |