pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Álvaro Herrera <alvherre@kurilemu.de>
To: Chao Li <li.evan.chao@gmail.com>
Cc: Baji Shaik <baji.pgdev@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
Date: Thu, 28 May 2026 16:54:07 +0200
Message-ID: <ahhVcOk20rDXD1gl@alvherre.pgsql> (raw)
In-Reply-To: <172EB2C2-DE11-4E5B-B115-38A7AD3B6A3D@gmail.com>

On 2026-May-27, Chao Li wrote:

> > On May 27, 2026, at 11:06, Baji Shaik <baji.pgdev@gmail.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/





view thread (9+ messages)  latest in thread

Message-ID: <ahhVcOk20rDXD1gl@alvherre.pgsql>
Permalink:  ../ahhVcOk20rDXD1gl@alvherre.pgsql/
Also on:    postgresql.org/message-id/ahhVcOk20rDXD1gl@alvherre.pgsql

 · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: alvherre@kurilemu.de, li.evan.chao@gmail.com, baji.pgdev@gmail.com, pgsql-hackers@lists.postgresql.org
  Subject: Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
  In-Reply-To: <ahhVcOk20rDXD1gl@alvherre.pgsql>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox