Re: race condition in pg_class

On Mon, Sep 09, 2024 at 10:55:32AM +0530, Nitin Motiani wrote:
> On Sat, Sep 7, 2024 at 12:25 AM Noah Misch <noah(at)leadboat(dot)com> 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.