MIME-Version: 1.0
References: <CAA4eK1KZyjdn7oXS3d7qJU5ag9ss4xSk0Jkjsh5NOrRdkPkV1g@mail.gmail.com>
 <ZfqRbKci5EVYHNla@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACUvK7ShPyirAURWf62=qOQqu=NwnyL5CMfVjM5Ody7Oxw@mail.gmail.com>
 <ZfvdWH9+gNiIB7EI@ip-10-97-1-34.eu-west-3.compute.internal>
 <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>
In-Reply-To: <Zf1TOYzC9fniRft1@ip-10-97-1-34.eu-west-3.compute.internal>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Fri, 22 Mar 2024 15:56:23 +0530
Message-ID: <CAA4eK1+2mJgi1q0Chfh4NbQ+M6W4OXWGbYHwM4C2SpfGp5aUfw@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	Nathan Bossart <nathandbossart@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/CAA4eK1%2B2mJgi1q0Chfh4NbQ%2BM6W4OXWGbYHwM4C2SpfGp5aUfw%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 22, 2024 at 3:15=E2=80=AFPM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Fri, Mar 22, 2024 at 01:45:01PM +0530, Bharath Rupireddy wrote:
>
> 1 =3D=3D=3D
>
> @@ -691,6 +699,13 @@ ReplicationSlotRelease(void)
>                 ConditionVariableBroadcast(&slot->active_cv);
>         }
>
> +       if (slot->data.persistency =3D=3D RS_PERSISTENT)
> +       {
> +               SpinLockAcquire(&slot->mutex);
> +               slot->last_inactive_at =3D GetCurrentTimestamp();
> +               SpinLockRelease(&slot->mutex);
> +       }
>
> I'm not sure we should do system calls while we're holding a spinlock.
> Assign a variable before?
>
> 2 =3D=3D=3D
>
> Also, what about moving this here?
>
> "
>     if (slot->data.persistency =3D=3D RS_PERSISTENT)
>     {
>         /*
>          * Mark persistent slot inactive.  We're not freeing it, just
>          * disconnecting, but wake up others that may be waiting for it.
>          */
>         SpinLockAcquire(&slot->mutex);
>         slot->active_pid =3D 0;
>         SpinLockRelease(&slot->mutex);
>         ConditionVariableBroadcast(&slot->active_cv);
>     }
> "
>
> That would avoid testing twice "slot->data.persistency =3D=3D RS_PERSISTE=
NT".
>

That sounds like a good idea. Also, don't we need to consider physical
slots where we don't reserve WAL during slot creation? I don't think
there is a need to set inactive_at for such slots. If we agree,
probably checking restart_lsn should suffice the need to know whether
the WAL is reserved or not.

>
> 5 =3D=3D=3D
>
> Most of the fields that reflect a time (not duration) in the system views=
 are
> xxxx_time, so I'm wondering if instead of "last_inactive_at" we should us=
e
> something like "last_inactive_time"?
>

How about naming it as last_active_time? This will indicate the time
at which the slot was last active.

--=20
With Regards,
Amit Kapila.