Date: Sun, 6 Apr 2025 11:00:54 -0700
From: Noah Misch <noah@leadboat.com>
To: pgsql-hackers@postgresql.org
Subject: Re: datfrozenxid > relfrozenxid w/ crash before XLOG_HEAP_INPLACE
Message-ID: <20250406180054.26.nmisch@google.com>
References: <20240620012908.92.nmisch@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240620012908.92.nmisch@google.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250406180054.26.nmisch%40google.com>
Precedence: bulk

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)?