Message-ID: <b33ffba7-0253-4f11-9fd5-cfff7832c3ac@intel.com>
Date: Mon, 6 Jan 2025 14:46:03 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC] Lock-free XLog Reservation from WAL
To: wenhui qiu <qiuwenhuifx@gmail.com>, Yura Sokolov
	<y.sokolov@postgrespro.ru>
CC: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <PH7PR11MB5796659F654F9BE983F3AD97EF142@PH7PR11MB5796.namprd11.prod.outlook.com>
 <421eede5-0ecf-421f-9b56-f05d4f51e099@postgrespro.ru>
 <CAGjGUAJbWgv5BB8-zm+6McUw7OqJ3paDv5CqeppTgXZk6OHF-Q@mail.gmail.com>
Content-Language: en-US
From: "Zhou, Zhiguo" <zhiguo.zhou@intel.com>
In-Reply-To: <CAGjGUAJbWgv5BB8-zm+6McUw7OqJ3paDv5CqeppTgXZk6OHF-Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N2tKd0tCVXZWYU1nTHRtMGljM25pclNrTlNKV1VjZUdpNEoybVNaRStRV2ty?=
 =?utf-8?B?Q2JDNmdJVml6VHJEcEVhRHpTWkdlWkN2c0VwZzNEQWkvRFdZN01HaXRyNXc3?=
 =?utf-8?B?ZzMxQ1k0ZDVJQmJXWWtXdnZNT0R5cWpFaGZPV1pKRURPcVJIYVZzQlhsWFVQ?=
 =?utf-8?B?b1E4MUJNR0JRTlAyM1RlYmJ2Z1ZQajhNUisvMmZTSVVaaVBCQ3lxZ0dsK3A3?=
 =?utf-8?B?a3FzUDVFQkxNTTNSSmMrQ1RBOVRsemZOUFpMbDdSWG1BcDRlWGhXNnBXcnlH?=
 =?utf-8?B?YmdNd2NubkcrLzE1Um0zUXIzZmxvbi9QN2p6VTVGZmtNaUcwbjBNSHZORjJH?=
 =?utf-8?B?MGcwTXJDRnQ2N1hyRHJCMXBGSjBUeTVYZWZTa1U3bWhXZjkvV01sWHd6R1JE?=
 =?utf-8?B?QUovTm4yNGdwRE8wK0FGNmpQZElEbURjcWc1ZEo0TnFFVWpRVGJHRHVyZ1Fl?=
 =?utf-8?B?TkZKSHFKOE5UN3NCTnBUSGRKYmhvbTdJYllPTkVNODRNUFRaLzVzUVJJYkdk?=
 =?utf-8?B?bFVqT0JzeWNudSthek9QeGRoKzJXbUE1MHZHdng4dGdIckN3eGN4eVJlVU9L?=
 =?utf-8?B?NlFmVldtcElOdENEVGFHZG1aUVhCcHdyYkVhNkJ0VGVZL05GVjhHOGRmL3BZ?=
 =?utf-8?B?V2xZRkg0T1g1M0dzTTYvN0wrLzlMWHVHdnhiVzRCWkhkSFB3SVl2OWtxa1FJ?=
 =?utf-8?B?VlBFZVF5KzFWUFJuZlkrV09Vc0pvUUdiUGhnV2kxbTF3UFBrbHFiYUFYTHlI?=
 =?utf-8?B?TTREelZ2VitRZkc5TFVpb0pYYnlDNy8wUlBxQkFmaS9UOGhkWE9UMEFRcE85?=
 =?utf-8?B?Q0ViK2FnTnVkeUl3SjByV0ExYVArWWw3ZW9nSTB0UGxZakhtSWZndlBtdEhQ?=
 =?utf-8?B?NktvcGZsTlVZV25uUzYwdUVzL3lzbENHam9RWVZNbGZoZzFyaGxybUJ0TitO?=
 =?utf-8?B?aUpLL1o2RXh3S2thQlltbkJIQ2wwV2JqS2JhM2JSR09yOTBud1lzUnBHVHJv?=
 =?utf-8?B?SGsxYzg1ZUhFa3lGWWdTdUZ0YVk1dGtLdXlpRy9yelNxNFRuSVdubGcvU25s?=
 =?utf-8?B?T01IYldYVWZSS0p0aGUyakNJZFNSUkJkWmVTZE4ydEhzM01td25jalBGUE54?=
 =?utf-8?B?NGtVUXlxeE95K056aXNxeGpNejJiSGJxQTh5dC9LcTBGMXoxN1NMQmg4d0N6?=
 =?utf-8?B?cDRBY2NobTV3UlpTZGt0THA2QjBRc2s1SEh0Ukd2YVpncUdJM1M0RlJKMFh1?=
 =?utf-8?B?NEtldHpzQzNSR1lPOEZkNCtwRXQ5YVJwZlZ4STZTOVc1dUpzdnVjL2RucTQz?=
 =?utf-8?B?SnA2SFdvSkxWbmJtMzlRMlNxc1hIYmxQcTFhd0dVQTg5ZUl4cXBKalZFaGNK?=
 =?utf-8?B?Y0xRWWZuNmVsYnloaXk0OXREQzVxK1JRZHlqV0plUkRZTTNGRmJoS2lvdVJO?=
 =?utf-8?B?YVJyaW9uSDFkTUFieURiWDIxOEZKdStzZDBHSlVGWFR2WHJzZmZJN0JxMnlq?=
 =?utf-8?B?cnUvSmUyM3hyUWNjQVlwZnpnRll3bjdscy85L2hZNHVOcFYrQXo4ZUNPQkZB?=
 =?utf-8?B?bjREMDJtZXhydE52SjNOZ3VIYjhhOW52em9BY08yNHVnaGRsT0lLbmRQaFIv?=
 =?utf-8?B?Nzgwcko5WlJJMDRDOGV2SnFiaEZOQ3Zwbk4vQVMydUlkaG1yT2MrY3JmQlNZ?=
 =?utf-8?B?WEp0WDBDNk40eTJZdGI0WTJvYm5wdkhGKzFtTmpSN09EQVpmWnljYm0vTVJn?=
 =?utf-8?B?Z1c1QlUwL1dPZWgwY1dGOWU4TXI2dEFrNzhKS2dKVVJzaVk3bnFPSUtxcFpN?=
 =?utf-8?B?TEZFZnR4a0NRK2NiSm5naEIxcS90ZXkyR1ZTbi9oNzczVUk0aVpIdmx3RVJk?=
 =?utf-8?B?cS9pRHliS0VPdVlIQS9kZTl4S2F3SjNmUGdsb2UrTk5VS2ZiQXdaMjFXUG9O?=
 =?utf-8?B?VXEwTjNlbXZFTk42KzJYWkJPckRrSlFaUnY0RUJESnRiZHB5YWd4VVlmN2RW?=
 =?utf-8?B?ZUxPbGhjS2pNMDBOWEUyRzAwY2Z4K1N2OVVtU1h5QUhNQjZWd08rVmxSeENR?=
 =?utf-8?B?K2VET3dZaUNGRU9SYk5IQ1FzMkw3N0lRdU1tcy9ZK2dLMjVJWnFjaW5uSmJR?=
 =?utf-8?Q?afHEs8HqHls+WxrL7gic1nb88?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a79bda9-2257-4b45-5419-08dd2e1dcd5e
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5796.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jan 2025 06:46:09.3933
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: HBpLo0b4+y1vky6Z0sXbXQu+rzGbk7jDkLFzqzMYuK57mIy51cBni0Nwz9HdO8D1DZdFtwONoPtYjzIYkEr9eg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8668
X-OriginatorOrg: intel.com
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/b33ffba7-0253-4f11-9fd5-cfff7832c3ac%40intel.com>
Precedence: bulk

Hi Yura and Wenhui,

Thanks for kindly reviewing this work!

On 1/3/2025 9:01 PM, wenhui qiu wrote:
> Hi
>      Thank you for your path，NUM_XLOGINSERT_LOCKS increase to 128，I 
> think it will be challenged，do we make it guc ？
> 

I noticed there have been some discussions (for example, [1] and its 
responses) about making NUM_XLOGINSERT_LOCKS a GUC, which seems to be a 
controversial proposal. Given that, we may first focus on the lock-free 
XLog reservation implementation, and leave the increase of 
NUM_XLOGINSERT_LOCKS for a future patch, where we would provide more 
quantitative evidence for the various implementations. WDYT?


> On Fri, 3 Jan 2025 at 20:36, Yura Sokolov <y.sokolov@postgrespro.ru 
> <mailto:y.sokolov@postgrespro.ru>> wrote:
> 
>     Good day, Zhiguo.
> 
>     Idea looks great.
> 
>     Minor issue:
>     - you didn't remove use of `insertpos_lck` from `ReserveXLogSwitch`.
> 
>     I initially thought it became un-synchronized against
>     `ReserveXLogInsertLocation`, but looking closer I found it is
>     synchronized with `WALInsertLockAcquireExclusive`.
>     Since there are no other `insertpos_lck` usages after your patch, I
>     don't see why it should exists and be used in `ReserveXLogSwitch`.
> 
>     Still I'd prefer to see CAS loop in this place to be consistent with
>     other non-locking access. And it will allow to get rid of
>     `WALInsertLockAcquireExclusive`, (though probably it is not a big
>     issue).
> 

Exactly, it should be safe to remove `insertpos_lck`. And I agree with 
you on getting rid of `WALInsertLockAcquireExclusive` with CAS loop 
which should significantly reduce the synchronization cost here 
especially when we intend to increase NUM_XLOGINSERT_LOCKS. I will try 
it in the next version of patch.


>     Major issue:
>     - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/read with on
>     platforms where MAXALIGN != 8 or without native 64 load/store. Branch
>     with 'memcpy` is rather obvious, but even pointer de-referencing on
>     "lucky case" is not safe either.
> 
>     I have no idea how to fix it at the moment.
> 

Indeed, non-atomic write/read operations can lead to safety issues in 
some situations. My initial thought is to define a bit near the 
prev-link to flag the completion of the update. In this way, we could 
allow non-atomic or even discontinuous write/read operations on the 
prev-link, while simultaneously guaranteeing its atomicity through 
atomic operations (as well as memory barriers) on the flag bit. What do 
you think of this as a viable solution?


>     Readability issue:
>     - It would be good to add `Assert(ptr >= upto)` into `GetXLogBuffer`.
>     I had hard time to recognize `upto` is strictly not in the future.
>     - Certainly, final version have to have fixed and improved comments.
>     Many patch's ideas are strictly non-obvious. I had hard time to
>     recognize patch is not a piece of ... (excuse me for the swear
>     sentence).

Thanks for the suggestion and patience. It's really more readable after 
inserting the assertion, I will fix it and improve other comments in the 
following patches.


>     Indeed, patch is much better than it looks on first sight.
>     I came with alternative idea yesterday, but looking closer to your
>     patch
>     today I see it is superior to mine (if atomic access will be fixed).
> 
>     ----
> 
>     regards,
>     Yura Sokolov aka funny-falcon
> 
> 

Regards,
Zhiguo


[1] https://www.postgresql.org/message-id/2266698.1704854297%40sss.pgh.pa.us