public inbox for [email protected]  
help / color / mirror / Atom feed
From: Noah Misch <[email protected]>
To: Nitin Motiani <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected]
Cc: Tom Lane <[email protected]>
Cc: Smolkin Grigory <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Subject: Re: race condition in pg_class
Date: Thu, 19 Sep 2024 14:33:46 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAH5HC94xjj7JvRXD4LHVMgv=qVxuMm0On+2OvrGqeON4U-FTuQ@mail.gmail.com>
References: <[email protected]>
	<CAH5HC96yPfrLVQ-srZ65R73t_dBLNM4mCR-RGcT5zNa04DHSxA@mail.gmail.com>
	<[email protected]>
	<CAH5HC96ASNkXyYVEG7pO98qr1WARGTohxGjVpBsu4c3hkM_OcA@mail.gmail.com>
	<[email protected]>
	<CAH5HC96ZwrjKF-RBijntw9uX2UzUtEKC3A7cxB_Xh8arVuKhKw@mail.gmail.com>
	<[email protected]>
	<CAH5HC974+yieU=uTWPtpY1+irUZAY+5GiGKiXjfKhLzo+jsE+g@mail.gmail.com>
	<[email protected]>
	<CAH5HC94xjj7JvRXD4LHVMgv=qVxuMm0On+2OvrGqeON4U-FTuQ@mail.gmail.com>

On Mon, Sep 09, 2024 at 10:55:32AM +0530, Nitin Motiani wrote:
> On Sat, Sep 7, 2024 at 12:25 AM Noah Misch <[email protected]> wrote:
> > https://commitfest.postgresql.org/49/5090/ remains in status="Needs review".
> > When someone moves it to status="Ready for Committer", I will commit
> > inplace090, inplace110, and inplace120 patches.  If one of you is comfortable
> > with that, please modify the status.
> 
> Done.

FYI, here are the branch-specific patches.  I plan to push these after the v17
release freeze lifts next week.  Notes from the back-patch:

1. In v13 and v12, "UPDATE pg_class" or "UPDATE pg_database" can still lose a
concurrent inplace update.  The v14+ fix relied on commit 86dc900 "Rework
planning and execution of UPDATE and DELETE", which moved the last fetch of
the pre-update tuple into nodeModifyTable.c.  Fixing that was always optional.
I prefer leaving it unfixed in those two branches, as opposed to writing a fix
specific to those branches.  Here's what I put in v13 and v12:

 		/*
+		 * We lack the infrastructure to follow rules in README.tuplock
+		 * section "Locking to write inplace-updated tables".  Specifically,
+		 * we lack infrastructure to lock tupleid before this file's
+		 * ExecProcNode() call fetches the tuple's old columns.  Just take a
+		 * lock that silences check_lock_if_inplace_updateable_rel().  This
+		 * doesn't actually protect inplace updates like those rules intend,
+		 * so we may lose an inplace update that overlaps a superuser running
+		 * "UPDATE pg_class" or "UPDATE pg_database".
+		 */
+#ifdef USE_ASSERT_CHECKING
+		if (IsInplaceUpdateRelation(resultRelationDesc))
+		{
+			lockedtid = *tupleid;
+			LockTuple(resultRelationDesc, &lockedtid, InplaceUpdateTupleLock);
+		}
+		else
+			ItemPointerSetInvalid(&lockedtid);
+#endif


2. The other area of tricky conflicts was the back-patch in
ExecMergeMatched(), from v17 to v16.

3. I've added inplace088-SetRelationTableSpace, a back-patch of refactoring
commits 4c9c359 and 2484329 to v13 and v12.  Before those commits, we held the
modifiable copy of the relation's pg_class row throughout a
table_relation_copy_data().  Back-patching it avoids a needless long-duration
LOCKTAG_TUPLE, and it's better than implementing a novel way to avoid that.


Attachments:

  [application/x-tar-gz] inplace-thru-120.tar.gz (147.5K, ../[email protected]/2-inplace-thru-120.tar.gz)
  download

view thread (4+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: race condition in pg_class
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox