MIME-Version: 1.0
References: <CALj2ACW4fSOMiKjQ3=2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ@mail.gmail.com>
 <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
In-Reply-To: <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 10 Sep 2024 10:53:07 +0530
Message-ID: <CAA4eK1LHpX4JUm1UGq6ixJRSdXpLNXfsxPcVrk=35ZHAvwj0cg@mail.gmail.com>
Subject: Re: Disallow altering invalidated replication slots
To: Peter Smith <smithpb2250@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1LHpX4JUm1UGq6ixJRSdXpLNXfsxPcVrk%3D35ZHAvwj0cg%40mail.gmail.com>
Precedence: bulk

On Tue, Sep 10, 2024 at 8:40=E2=80=AFAM Peter Smith <smithpb2250@gmail.com>=
 wrote:
>
> Hi, here are some review comments for patch v1.
>
> =3D=3D=3D=3D=3D=3D
> 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...
>
> =3D=3D=3D=3D=3D=3D
> 2. Missing docs update
>
> Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
> not allowed for invalid slots?
>
> =3D=3D=3D=3D=3D=3D
> src/backend/replication/slot.c
>
> 3.
> + if (MyReplicationSlot->data.invalidated !=3D 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

--=20
With Regards,
Amit Kapila.