The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested
Hi, Andrey! The patch applies correctly to d4c0f91f (master) Here is some minor review comments. *** It's worth adding a comment explaining why we don't use the lock swap protocol for MultiXactOffsetCtl in RecordNewMultiXact, unlike for MultiXactMemberCtl (where we check whether rotation is required before swapping the lock). That the lock will be the same at wraparound only (when MultiXactOffsetCtl->nbanks = 1). Since this is a rare case, it's not worth handling in the code, but it should be documented with a comment. It seems even more confusing if we inspect GetMultiXactIdMembers where MultiXactOffsetCtl checks wraparound case before rotate lock (rotation only required at wraparound). So it would be better to simplify MultiXactOffsetCtl lock swapping at GetMultiXactIdMembers in the same manner as it is done at RecordNewMultiXact (exclude extra checks because it returns that swap required almost every time). *** I believe we should use int64 instead of int for next_pageno in RecordNewMultiXact(). There's a specific reason for using int64 - see below: 4ed8f09 Index SLRUs by 64-bit integers rather than by 32-bit integers https://postgr.es/m/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com https://postgr.es/m/CAJ7c6TPDOYBYrnCAeyndkBktO0WG2xSdYduTF0nxq%2BvfkmTF5Q%40mail.gmail.com *** We should delete MULTIXACT_CREATION wait event type from src/backend/utils/activity/wait_event_names.txt because we delete corresponding conditional variable. *** I also noticed some minor typos; here is the corrected version. ---- /* Also we record next offset here */ Kill9 works unpredictably on Windows / exta 'a' was in unpredictably # Verify that recorded multi is readable, this call must not hang. recorded multi is readable # Test mxid wraparound The new status of this patch is: Waiting on Author
