Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?

From: Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru>
To: Peter Geoghegan <pg(at)bowt(dot)ie>, Maxim Orlov <orlovmg(at)gmail(dot)com>
Cc: "Anton A(dot) Melnikov" <a(dot)melnikov(at)postgrespro(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?
Date: 2024-06-19 17:00:28
Message-ID: 0e3466eb-ea42-47a6-8662-ca94b95f5a66@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2024-06-19 17:09:26 Re: generic plans and "initial" pruning
Previous Message Alvaro Herrera 2024-06-19 16:51:31 Re: BitmapHeapScan streaming read user and prelim refactoring