Subject: Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid.

From: "Anton A. Melnikov" <[email-masked]>
Date: Fri, 14 Jun 2024 10:45:38 +0300

Hello!

The src/backend/access/heap/README.tuplock says about HEAP_XMAX_INVALID bit
that "Any tuple with this bit set does not have a valid value stored in XMAX."

Found that FreezeMultiXactId() tries to process such an invalid multi xmax
and may looks for an update xid in the pg_multixact for it.

Maybe not do this work in FreezeMultiXactId() and exit immediately if the
bit HEAP_XMAX_INVALID was already set?

For instance, like that:

master
@@ -6215,6 +6215,15 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
         /* We should only be called in Multis */
         Assert(t_infomask & HEAP_XMAX_IS_MULTI);
  
+       /* Xmax is already marked as invalid */
+       if (MultiXactIdIsValid(multi) &&
+               (t_infomask & HEAP_XMAX_INVALID))
+       {
+               *flags |= FRM_INVALIDATE_XMAX;
+               pagefrz->freeze_required = true;
+               return InvalidTransactionId;
+       }
+
         if (!MultiXactIdIsValid(multi) ||
                 HEAP_LOCKED_UPGRADED(t_infomask))

With the best regards,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

---

From: "Anton A. Melnikov" <[email-masked]>
Date: Tue, 18 Jun 2024 09:57:11 +0300

> bit HEAP_XMAX_INVALID was already set?

Seems it is important to save the check that multi xmax is not behind relminmxid.
So saved it and correct README.tuplock accordingly.

Would be glad if someone take a look at the patch attached.

With the best regards,

From 7608078bddf35904427cb1c04497ab0c98e9d111 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <[email-masked]>
Date: Fri, 14 Jun 2024 10:42:55 +0300
Subject: [PATCH] Don't process multi xmax in the FreezeMultiXactId() if the
 HEAP_XMAX_INVALID bit was already set.

---
 src/backend/access/heap/README.tuplock | 1 +
 src/backend/access/heap/heapam.c       | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..e198942647 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -124,6 +124,7 @@ The following infomask bits are applicable:
 
 - HEAP_XMAX_INVALID
   Any tuple with this bit set does not have a valid value stored in XMAX.
+  Although if it's a multi xmax it must follow relminmxid.
 
 - HEAP_XMAX_IS_MULTI
   This bit is set if the tuple's Xmax is a MultiXactId (as opposed to a
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 82bb9cb33b..d1cba6970d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6227,6 +6227,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
 				(errcode(ERRCODE_DATA_CORRUPTED),
 				 errmsg_internal("found multixact %u from before relminmxid %u",
 								 multi, cutoffs->relminmxid)));
+	else if (MultiXactIdIsValid(multi) &&
+			(t_infomask & HEAP_XMAX_INVALID))
+	{
+		/* Xmax is already marked as invalid */
+		*flags |= FRM_INVALIDATE_XMAX;
+		pagefrz->freeze_required = true;
+		return InvalidTransactionId;
+	}
 	else if (MultiXactIdPrecedes(multi, cutoffs->OldestMxact))
 	{
 		TransactionId update_xact;
-- 
2.45.2

---

From: Maxim Orlov <[email-masked]>
Date: Tue, 18 Jun 2024 17:29:13 +0300

Hi!

Maybe, I'm too bold, but looks like a kinda bug to me.  At least, I don't
understand why we do not check the HEAP_XMAX_INVALID flag.
My guess is nobody noticed, that MultiXactIdIsValid call does not check the
mentioned flag in the "first" condition, but it's all my speculation.
Does anyone know if there are reasons to deliberately ignore the HEAP_XMAX
INVALID flag? Or this is just an unfortunate oversight.

PFA, my approach on this issue.

-- 
Best regards,
Maxim Orlov.

---

From: Peter Geoghegan <[email-masked]>
Date: Tue, 18 Jun 2024 11:47:58 -0400

On Tue, Jun 18, 2024 at 10:29 AM Maxim Orlov <[email-masked]> wrote:
> Maybe, I'm too bold, but looks like a kinda bug to me.  At least, I don't understand why we do not check the HEAP_XMAX_INVALID flag.
> My guess is nobody noticed, that MultiXactIdIsValid call does not check the mentioned flag in the "first" condition, but it's all my speculation.

A related code path was changed in commit 02d647bbf0. That change made
the similar xmax handling that covers XIDs (not MXIDs) *stop* doing
what you're now proposing to do in the Multi path.

Why do you think this is a bug?

> Does anyone know if there are reasons to deliberately ignore the HEAP_XMAX INVALID flag? Or this is just an unfortunate oversight.

HEAP_XMAX_INVALID is just a hint.

-- 
Peter Geoghegan

---

From: Yura Sokolov <[email-masked]>
Date: Wed, 19 Jun 2024 20:00:28 +0300

> what you're now proposing to do in the Multi path.

I don't agree commit 02d647bbf0 is similar to suggested change.
Commit 02d647bbf0 fixed decision to set
	freeze_xmax = false;
	xmax_already_frozen = true;

Suggested change is for decision to set
	*flags |= FRM_INVALIDATE_XMAX;
	pagefrz->freeze_required = true;
Which leads to
	freeze_xmax = true;

So it is quite different code paths, and one could not be used
to decline or justify other.

More over, certainly test on HEAP_XMAX_INVALID could be used
there in heap_prepare_freeze_tuple to set
	freeze_xmax = true;
Why didn't you do it?

> Why do you think this is a bug?

It is not a bug per se.
But:
- it is missed opportunity for optimization,
- it is inconsistency in data handling.
Inconsistency leads to bugs when people attempt to modify code.

Yes, we changed completely different place mistakenly relying on 
consistent reaction on this "hint", and that leads to bug in our
patch.

> HEAP_XMAX_INVALID is just a hint.

WTF is "just a hint"?
I thought, hint is "yep, you can ignore it. But we already did some job 
and stored its result as this hint. And if you don't ignore this hint, 
then you can skip doing the job we did already".

So every time we ignore hint, we miss opportunity for optimization.
Why the hell we shouldn't optimize when we safely can?

If we couldn't rely on hint, then hint is completely meaningless.

--------

have a nice day

Yura Sokolov

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 19 Jun 2024 19:21:25 +0200

> +               (t_infomask & HEAP_XMAX_INVALID))

Hmm, but why are we calling FreezeMultiXactId at all if the
HEAP_XMAX_INVALID bit is set?  We shouldn't do that.  I think the fix
should appear in heap_prepare_freeze_tuple() to skip work completely if
HEAP_XMAX_INVALID is set.  Then in FreezeMultiXactId you could simply
Assert() that the given tuple does not have HEAP_XMAX_INVALID set.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
                http://smylers.hates-software.com/2005/09/08/1995c749.html