public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Kapila <[email protected]>
To: Peter Smith <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Disallow altering invalidated replication slots
Date: Tue, 10 Sep 2024 10:53:07 +0530
Message-ID: <CAA4eK1LHpX4JUm1UGq6ixJRSdXpLNXfsxPcVrk=35ZHAvwj0cg@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
References: <CALj2ACW4fSOMiKjQ3=2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ@mail.gmail.com>
<CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <[email protected]> wrote:
>
> Hi, here are some review comments for patch v1.
>
> ======
> Commit message
>
> 1.
> ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
> as there is no way...
>
> suggestion:
> ALTER_REPLICATION_SLOT for invalid replication slots should not be
> allowed because there is no way...
>
> ======
> 2. Missing docs update
>
> Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
> not allowed for invalid slots?
>
> ======
> src/backend/replication/slot.c
>
> 3.
> + if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot alter replication slot \"%s\"", name),
> + errdetail("This replication slot was invalidated due to \"%s\".",
> + SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
> +
>
> I thought including the reason "invalid" (e.g. "cannot alter invalid
> replication slot \"%s\"") in the message might be better,
>
Agreed, I could see a similar case with a message ("cannot alter
invalid database \"%s\"") in the code. Additionally, we should also
include Shveta's suggestion to change the detailed message to other
similar messages in logical.c
--
With Regards,
Amit Kapila.
view thread (4+ messages) latest in thread
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Disallow altering invalidated replication slots
In-Reply-To: <CAA4eK1LHpX4JUm1UGq6ixJRSdXpLNXfsxPcVrk=35ZHAvwj0cg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox