Re: [RFC] Lock-free XLog Reservation from WAL

Good day, Yura!

On 1/10/2025 8:42 PM, Yura Sokolov wrote:
> If you consider hash-table fillrate, than 256 is quite enough for 128
> concurrent inserters.

The profile of your patch didn't show significant hotspots in the hash
table functions, so I believe the 256 entries should be enough.

>>
>> I will soon try to evaluate the performance impact of your patch on my
>> device with the TPCC benchmark and also profile it to see if there are
>> any changes that could be made to further improve it.
>
> It would be great. On my notebook (Mac Air M1) I don't see any benefits
> neither from mine, nor from yours patch ))
> My colleague will also test it on 20 core virtual machine (but
> backported to v15).
>

I've tested the performance impact of our patches on an Intel Sapphire
Rapids device with 480 vCPUs using a HammerDB TPC-C workload (256 VUs).
The results show a 72.3% improvement (average of 3 rounds, RSD: 1.5%)
with your patch and a 76.0% boost (average of 3 rounds, RSD: 2.95%) with
mine, applied to the latest codebase. This optimization is most
effective on systems with over 64 cores, as our core-scaling experiments
suggest minimal impact on lower-core setups like your notebook or a
20-core VM.

>
>> BTW, do you have a plan to merge this patch to the master branch? Thanks!
>
> I'm not committer )) We are both will struggle to make something
> committed for many months ;-)
>
> BTW, your version could make alike trick for guaranteed atomicity:
> - change XLogRecord's `XLogRecPtr xl_prev` to `uint32 xl_prev_offset`
> and store offset to prev record's start.
>
> Since there are two limits:
>
>     #define XLogRecordMaxSize    (1020 * 1024 * 1024)
>     #define WalSegMaxSize 1024 * 1024 * 1024
>
> offset to previous record could not be larger than 2GB.
>
> Yes, it is format change, that some backup utilities will have to adopt.
> But it saves 4 bytes in XLogRecord (that could be spent to store
> FullTransactionId instead of TransactionId) and it is better compressible.
>
> And your version than will not need the case when this value is split
> among two buffers (since MAXALIGN is not less than 4), and PostgreSQL
> already relies on 4 byte read/write atomicity (in some places even
> without use of pg_atomic_uint32).
>
> ----
>
> regards
> Sokolov Yura aka funny-falcon

Thanks for the great suggestion!

I think we've arrived at a critical juncture where we need to decide
which patch to move forward with for our optimization efforts. I've
evaluated the pros and cons of my implementation:

Pros:
-Achieves an additional 4% performance improvement.

Cons:
-Breaks the current convention of XLog insertions.
-TAP tests are not fully passed, requiring time to resolve.
-May necessitate changes to the format and backup tools, potentially
leading to backward compatibility issues.

Given these considerations, I believe your implementation is superior to
mine. I'd greatly appreciate it if you could share your insights on this
matter.

Regards,
Zhiguo