Re: Fix race condition in InvalidatePossiblyObsoleteSlot()

Hi,

On Mon, Feb 19, 2024 at 01:45:16PM +0530, Bharath Rupireddy wrote:
> On Mon, Feb 19, 2024 at 11:44 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > > Yeah, comments added in v3.
> >
> > The contents look rather OK, I may do some word-smithing for both.
>
> Here are some comments on v3:

Thanks for looing at it!

> 1.
> + XLogRecPtr initial_effective_xmin = InvalidXLogRecPtr;
> + XLogRecPtr initial_catalog_effective_xmin = InvalidXLogRecPtr;
> + XLogRecPtr initial_restart_lsn = InvalidXLogRecPtr;
>
> Prefix 'initial_' makes the variable names a bit longer, I think we
> can just use effective_xmin, catalog_effective_xmin and restart_lsn,
> the code updating then only when if (!terminated) tells one that they
> aren't updated every time.

I'm not sure about that. I prefer to see meaningfull names instead of having
to read the code where they are used.

> 2.
> + /*
> + * We'll release the slot's mutex soon, so it's possible that
> + * those values change since the process holding the slot has been
> + * terminated (if any), so record them here to ensure we would
> + * report the slot as obsolete correctly.
> + */
>
> This needs a bit more info as to why and how effective_xmin,
> catalog_effective_xmin and restart_lsn can move ahead after signaling
> a backend and before the signalled backend reports back.

I'm not sure of the added value of such extra details in this comment and if
the comment would be easy to maintain. I've the feeling that knowing it's possible
is enough here. Happy to hear what others think about it too.

> 3.
> + /*
> + * Assert that the conflict cause recorded previously before we
> + * terminate the process did not change now for any reason.
> + */
> + Assert(!(conflict_prev != RS_INVAL_NONE && terminated &&
> + conflict_prev != conflict));
>
> It took a while for me to understand the above condition, can we
> simplify it like below using De Morgan's laws for better readability?
>
> Assert((conflict_prev == RS_INVAL_NONE) || !terminated ||
> (conflict_prev == conflict));

I don't have a strong opinon on this, looks like a matter of taste.

Regards,

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