On Mon, Jan 22, 2024 at 09:07:45AM +0000, Bertrand Drouvot wrote:
> On Mon, Jan 22, 2024 at 03:54:44PM +0900, Michael Paquier wrote:
>> Also, wouldn't it be better to document in the test why
>> txid_current_snapshot() is chosen? Contrary to txid_current(), it
>> does not generate a Transaction/COMMIT to make the test more
>> predictible, something you have mentioned upthread, and implied in the
>> test.
>
> Good point, added more comments in v8 (but not mentioning txid_current() as
> after giving more thought about it then I think it was not the right approach in
> any case).
Fine by me.
>> - INSERT INTO flush_wal DEFAULT VALUES; -- see create table flush_wal
>>
>> This removes two INSERTs on flush_wal and refactors the code to do the
>> INSERT in wait_until_vacuum_can_remove(), using a SQL comment to
>> document a reference about the reason why an INSERT is used. Couldn't
>> you just use a normal comment here?
>
> Agree, done in v8.
I've rewritten some of that, and applied the patch down to v16. Let's
see how this stabilizes things, but that's likely going to take some
time as it depends on skink's mood.
>> I need to review what you have more thoroughly, but is it OK to assume
>> that both of you are happy with the latest version of the patch in
>> terms of stability gained? That's still not the full picture, still a
>> good step towards that.
>
> Yeah, I can clearly see how this patch helps from a theoritical perspective but
> rely on Alexander's testing to see how it actually stabilizes the test.
Anyway, that's not the end of it. What should we do for snapshot
snapshot records coming from the bgwriter? The slower the system, the
higher the odds of hitting a conflict with such records, even if the
horizon check should help.
--
Michael