Feedback-ID: id4a34324:Fastmail
Date: Wed, 11 Jan 2023 08:52:15 -0800
From: Andres Freund <andres@anarazel.de>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	Thomas Munro <thomas.munro@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Ibrar Ahmed <ibrar.ahmad@gmail.com>,
	Amit Khandekar <amitdkhan.pg@gmail.com>, fabriziomello@gmail.com,
	tushar <tushar.ahuja@enterprisedb.com>,
	Rahila Syed <rahila.syed@2ndquadrant.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Minimal logical decoding on standbys
Message-ID: <20230111165215.44nfwrcy2jhse3cj@awork3.anarazel.de>
References: <CA+TgmobmTvEUDVOBzARa9eD9XLqsvDW+v29dV+WvDKZPJAwNTQ@mail.gmail.com>
 <CA+TgmoaOsoQuXEo6gMq0GP8SyvCaS=ptYqdYc0uzDMoeShf_nw@mail.gmail.com>
 <CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
 <1f2ad9fd-53ff-74cc-a602-72d45423bcab@gmail.com>
 <a78de011-81cf-729c-e110-e6d636fdca4d@gmail.com>
 <05c0c4f3-82eb-bc6d-c876-5a265c05927b@gmail.com>
 <CA+TgmoY8LAaTwAhS+YNQNrM+3K8eqxGvPT3gbJaHER86XShvFw@mail.gmail.com>
 <20230106034036.2m4qnn7ep7b5ipet@awork3.anarazel.de>
 <6f8503b7-93d6-e78d-1d61-09cfec4d59c9@gmail.com>
 <CALj2ACXFkMOOD2PwMVLw4vb2cZHn-dd60ooiZay2jTdA-XfoYg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALj2ACXFkMOOD2PwMVLw4vb2cZHn-dd60ooiZay2jTdA-XfoYg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230111165215.44nfwrcy2jhse3cj%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-01-11 13:02:13 +0530, Bharath Rupireddy wrote:
> 3. Is this feature still a 'minimal logical decoding on standby'?
> Firstly, why is it 'minimal'?

It's minimal in comparison to other proposals at the time that did explicit /
active coordination between primary and standby to allow logical decoding.


> 0002:
> 1.
> -    if (InvalidateObsoleteReplicationSlots(_logSegNo))
> +    InvalidateObsoleteOrConflictingLogicalReplicationSlots(_logSegNo,
> &invalidated, InvalidOid, NULL);
> 
> Isn't the function name too long and verbose?

+1


> How about just InvalidateLogicalReplicationSlots() let the function comment
> talk about what sorts of replication slots it invalides?

I'd just leave the name unmodified at InvalidateObsoleteReplicationSlots().


> 2.
> +                                errdetail("Logical decoding on
> standby requires wal_level to be at least logical on master"));
> + *     master wal_level is set back to replica, so existing logical
> slots need to
> invalidate such slots. Also do the same thing if wal_level on master
> 
> Can we use 'primary server' instead of 'master' like elsewhere? This
> comment also applies for other patches too, if any.

+1


> 3. Can we show a new status in pg_get_replication_slots's wal_status
> for invalidated due to the conflict so that the user can monitor for
> the new status and take necessary actions?

Invalidated slots are not a new concept introduced in this patchset, so I'd
say we can introduce such a field separately.

Greetings,

Andres Freund