public inbox for [email protected]  

help / color / mirror / Atom feed

From: Peter Smith <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Disallow altering invalidated replication slots
Date: Tue, 10 Sep 2024 13:09:45 +1000
Message-ID: <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@mail.gmail.com> (raw)
In-Reply-To: <CALj2ACW4fSOMiKjQ3=2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ@mail.gmail.com>
References: <CALj2ACW4fSOMiKjQ3=2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ@mail.gmail.com>

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, but OTOH I
see the patch message is the same as an existing one. Maybe see what
others think.

======
src/test/recovery/t/035_standby_logical_decoding.pl

3.
There is already a comment about this test:
##################################################
# Recovery conflict: Invalidate conflicting slots, including in-use slots
# Scenario 1: hot_standby_feedback off and vacuum FULL
#
# In passing, ensure that replication slot stats are not removed when the
# active slot is invalidated.
##################################################

IMO we should update that "In passing..." sentence to something like:

In passing, ensure that replication slot stats are not removed when
the active slot is invalidated, and check that an error occurs when
attempting to alter the invalid slot.

======
[1] docs - https://www.postgresql.org/docs/devel/protocol-replication.html

Kind Regards,
Peter Smith.
Fujitsu Austalia

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]
  Subject: Re: Disallow altering invalidated replication slots
  In-Reply-To: <CAHut+PtvnyH2-U4=XmqDOQSkw5arB=A5B9xbYBPvM5uPU2BG_g@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