Message-ID: <296b2c56-c688-41a0-a752-b242268691fd@wi3ck.info>
Date: Tue, 12 Nov 2024 10:12:40 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Commit Timestamp and LSN Inversion issue
To: Andres Freund <andres@anarazel.de>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Tomas Vondra <tomas@vondra.me>,
 "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 Aleksander Alekseev <aleksander@timescale.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 shveta malik <shveta.malik@gmail.com>, Jan Wieck <jan@wi3ck.info>
References: <CAA4eK1KhAxe6=VTxa6xEbc22_VVBLsVuvRH8Wxws8i50gH=eqw@mail.gmail.com>
 <9f7f4def-7ea8-4ad3-83e7-a8cc9d18c58a@wi3ck.info>
 <gkbrjxwyjbqckjbrkep3uy2mvrcq2mrfzprfmegbeiqxidtmbu@3cgs5iocjykk>
 <OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <xhcapr7lur35itqrbk4qdqn7rzfaqr3g6dvyxzfx6jeyknhvbv@h3wkgixuuuon>
 <CAA4eK1+ZdL=sC4sdUOQz+X9=7Z8Uxhkd0VqwcB9rgzXjvu=6yA@mail.gmail.com>
 <11ee3489-40ba-45d6-ba6a-7c3def725e84@vondra.me>
 <CAA4eK1KQ6H4u5=_5Xq44HhX3RFXW4HW-z0F4T4J1nf2xVCo04w@mail.gmail.com>
 <CAA4eK1L6Vpf9EmL2bpfD3kdvM6_2XQqwJ9GY6uY-hgcot76cqA@mail.gmail.com>
 <00f7cfb2-e3a6-4bd7-b19c-fd75d3838049@wi3ck.info>
 <lyydpy6xly7gefcswkimur733wcbke4bdjwjedvluyo2ebfdr7@xz5k4mbjgivm>
Content-Language: en-US
From: Jan Wieck <jan@wi3ck.info>
In-Reply-To: <lyydpy6xly7gefcswkimur733wcbke4bdjwjedvluyo2ebfdr7@xz5k4mbjgivm>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/296b2c56-c688-41a0-a752-b242268691fd%40wi3ck.info>
Precedence: bulk

Hello,

On 11/12/24 08:55, Andres Freund wrote:
> Hi,
> 
> On 2024-11-12 08:51:49 -0500, Jan Wieck wrote:
>> On 11/11/24 23:21, Amit Kapila wrote:
>> > As the inversion issue can mainly hamper logical replication-based
>> > solutions we can do any of this additional work under spinlock only
>> > when the current record is a commit record (which the currently
>> > proposed patch is already doing) and "wal_level = logical" and also
>> > can have another option at the subscription level to enable this new
>> > code path. I am not sure what is best but just sharing the ideas here.
>>
>> It can indeed be reduced to one extra *unlikely* if test only for commit
>> records and only when WAL level is "logical". Without those two being true
>> there would be zero impact on ReserveXLogInsertLocation().
> 
> No, the impact would be far larger, because it would make it impractical to
> remove this bottleneck by using atomic instructions in
> ReserveXLogInsertLocation(). It doesn't make sense to make it harder to
> resolve one of the core central bottlenecks in postgres for a niche feature
> like this. Sorry, but this is just not a viable path.


The current section of code covered by the spinlock performs the 
following operations:

     - read CurrBytePos (shmem)
     - read PrevBytePos (shmem)
     - store CurrBytePos in PrevBytePos
     - increment CurrBytePos by size

In addition there is ReserveXLogSwitch() that is far more complex and is 
guarded by the same spinlock because both functions manipulate the same 
shared memory variables. With that in mind, how viable is the proposal 
to replace these code sections in both functions with atomic operations?


Best Regards, Jan