On Mon, Feb 19, 2024 at 09:49:24AM +0000, Bertrand Drouvot wrote:
> On Mon, Feb 19, 2024 at 01:45:16PM +0530, Bharath Rupireddy wrote:
>> 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.
Prefixing these with "initial_" is fine, IMO. That shows the
intention that these come from the slot's data before doing the
termination. So I'm OK with what's been proposed in v3.
>> 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.
Documenting that the risk exists rather than all the possible reasons
why this could happen is surely more maintainable. In short, I'm OK
with what the patch does, just telling that it is possible.
>> + 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.
Both are the same to me, so I have no extra opinion to offer. ;)
--
Michael