Message-ID: <0e3466eb-ea42-47a6-8662-ca94b95f5a66@postgrespro.ru>
Date: Wed, 19 Jun 2024 20:00:28 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is
 already marked as invalid?
To: Peter Geoghegan <pg@bowt.ie>, Maxim Orlov <orlovmg@gmail.com>
Cc: "Anton A. Melnikov" <a.melnikov@postgrespro.ru>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <21ece835-b671-4656-9f35-008b9f355034@postgrespro.ru>
 <fc43dd15-564b-4239-bc78-d0cc47b114d7@postgrespro.ru>
 <CACG=ezbB2q2wYZseBA5mcNKXZwamPRb2dzzqrRtJ37E=K7do2w@mail.gmail.com>
 <CAH2-WznwPKvmTv_Mo=yOfngcjX-1aus7VhjYWw9LTA5pF4o=yg@mail.gmail.com>
Content-Language: en-US
From: Yura Sokolov <y.sokolov@postgrespro.ru>
In-Reply-To: <CAH2-WznwPKvmTv_Mo=yOfngcjX-1aus7VhjYWw9LTA5pF4o=yg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/0e3466eb-ea42-47a6-8662-ca94b95f5a66%40postgrespro.ru>
Precedence: bulk

18.06.2024 18:47, Peter Geoghegan пишет:
> On Tue, Jun 18, 2024 at 10:29 AM Maxim Orlov <orlovmg@gmail.com> 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.

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.

>> 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.
> 

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