From: Noah Misch <noah@leadboat.com>
To: pgsql-hackers@postgresql.org
Subject: Re: datfrozenxid > relfrozenxid w/ crash before XLOG_HEAP_INPLACE
Date: Sun, 6 Apr 2025 11:00:54 -0700
Message-ID: <20250406180054.26.nmisch@google.com> (raw)
In-Reply-To: <20240620012908.92.nmisch@google.com>
References: <20240620012908.92.nmisch@google.com>
On Wed, Jun 19, 2024 at 06:29:08PM -0700, Noah Misch wrote:
> https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote:
> > Separable, nontrivial things not fixed in the attached patch stack:
>
> > - Trouble is possible, I bet, if the system crashes between the inplace-update
> > memcpy() and XLogInsert(). See the new XXX comment below the memcpy().
>
> That comment:
>
> /*----------
> * XXX A crash here can allow datfrozenxid() to get ahead of relfrozenxid:
> *
> * ["D" is a VACUUM (ONLY_DATABASE_STATS)]
> * ["R" is a VACUUM tbl]
> * D: vac_update_datfrozenid() -> systable_beginscan(pg_class)
> * D: systable_getnext() returns pg_class tuple of tbl
> * R: memcpy() into pg_class tuple of tbl
> * D: raise pg_database.datfrozenxid, XLogInsert(), finish
> * [crash]
> * [recovery restores datfrozenxid w/o relfrozenxid]
> */
>
> > Might solve this by inplace update setting DELAY_CHKPT, writing WAL, and
> > finally issuing memcpy() into the buffer.
>
> That fix worked.
I pushed that as commit 8e7e672 (2024-10-25). I now think DELAY_CHKPT_START
is superfluous here, per this proc.h comment:
* (In the
* extremely common case where the data being modified is in shared buffers
* and we acquire an exclusive content lock on the relevant buffers before
* writing WAL, this mechanism is not needed, because phase 2 will block
* until we release the content lock and then flush the modified data to
* disk.)
heap_inplace_update_and_unlock() meets those conditions. Its closest
precedent is XLogSaveBufferForHint(), which does need DELAY_CHKPT_START due to
having only BUFFER_LOCK_SHARED. heap_inplace_update_and_unlock() has
BUFFER_LOCK_EXCLUSIVE, so it doesn't need DELAY_CHKPT_START.
I'm considering two options:
1. Stop using DELAY_CHKPT_START in heap_inplace_update_and_unlock().
2. Add a comment. BUFFER_LOCK_EXCLUSIVE entitles
heap_inplace_update_and_unlock() to omit DELAY_CHKPT_START. Instead, the
function elects to keep itself aligned with XLogSaveBufferForHint(), to
make analysis simpler.
I'm leaning toward (2), since inplace update isn't frequent enough that it
needs to pursue small optimizations. Would anyone strongly prefer (1)?
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com
Subject: Re: datfrozenxid > relfrozenxid w/ crash before XLOG_HEAP_INPLACE
In-Reply-To: <20250406180054.26.nmisch@google.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox