Date: Sat, 23 Mar 2024 09:04:45 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	Nathan Bossart <nathandbossart@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Introduce XID age and inactive timeout based replication slot
 invalidation
Message-ID: <Zf6bLeSngnn+AzXM@ip-10-97-1-34.eu-west-3.compute.internal>
References: <CALj2ACWk5a-LfBPziAjjFA--syoUSD_4jY6dZyhU8g8XuD-i7g@mail.gmail.com>
 <CAA4eK1+46kJF3Z2Tv=xdkyy0nuurrS7RfzqvqY8bq1MF-JLM=A@mail.gmail.com>
 <CALj2ACVKncd+JZR8BeXSgPO4akqic9kO99Z6pCfeXQKwmye2Og@mail.gmail.com>
 <CAA4eK1JwJdsKMxczGO9tYgEzxp_L2=j0QTeDDk2Tx4-ipGSppw@mail.gmail.com>
 <Zf0ump7z3XUSXJBw@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACWuNHJ3qBMmmy6i1YA9V8JDRSaciL2bBM+72bRcqiBE2g@mail.gmail.com>
 <Zf1TOYzC9fniRft1@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACVUCo48FXWAcxC9LEV6P=xgd-8O=e9aPmTRUZvvH_L7Zw@mail.gmail.com>
 <CAA4eK1LjNB8jBgqLHtf+FKnd=SfMz3TXXs871p7KjG4VHnxafg@mail.gmail.com>
 <CALj2ACXQmGx-L073=8CUSOk3hhe=HL_e6dveYeR9WJcEh+7jvg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CALj2ACXQmGx-L073=8CUSOk3hhe=HL_e6dveYeR9WJcEh+7jvg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zf6bLeSngnn%2BAzXM%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Sat, Mar 23, 2024 at 01:11:50PM +0530, Bharath Rupireddy wrote:
> On Sat, Mar 23, 2024 at 11:27 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > How about adding the test in 019_replslot_limit? It is not a direct
> > fit but I feel later we can even add 'invalid_timeout' related tests
> > in this file which will use last_inactive_time feature.
> 
> I'm thinking the other way. Now, the new TAP file 043_replslot_misc.pl
> can have last_inactive_time tests, and later invalid_timeout ones too.
> This way 019_replslot_limit.pl is not cluttered.

I share the same opinion as Amit: I think 019_replslot_limit would be a better
place, because I see the timeout as another kind of limit.

> 
> > It is also
> > possible that some of the tests added by the 'invalid_timeout' feature
> > will obviate the need for some of these tests.
> 
> Might be. But, I prefer to keep both these tests separate but in the
> same file 043_replslot_misc.pl. Because we cover some corner cases the
> last_inactive_time is set upon loading the slot from disk.

Right but I think that this test does not necessary have to be in the same .pl
as the one testing the timeout. Could be added in one of the existing .pl like
001_stream_rep.pl for example.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com