MIME-Version: 1.0
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> <0e3466eb-ea42-47a6-8662-ca94b95f5a66@postgrespro.ru>
In-Reply-To: <0e3466eb-ea42-47a6-8662-ca94b95f5a66@postgrespro.ru>
From: Peter Geoghegan <pg@bowt.ie>
Date: Wed, 19 Jun 2024 13:30:44 -0400
Message-ID: <CAH2-Wzmj6zR1MhHNO+kdFC9yizAbcQuQre52qGJrWgK65NJ_2Q@mail.gmail.com>
Subject: Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is
 already marked as invalid?
To: Yura Sokolov <y.sokolov@postgrespro.ru>
Cc: Maxim Orlov <orlovmg@gmail.com>, "Anton A. Melnikov" <a.melnikov@postgrespro.ru>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAH2-Wzmj6zR1MhHNO%2BkdFC9yizAbcQuQre52qGJrWgK65NJ_2Q%40mail.gmail.com>
Precedence: bulk

On Wed, Jun 19, 2024 at 1:00=E2=80=AFPM Yura Sokolov <y.sokolov@postgrespro=
.ru> wrote:
> So it is quite different code paths, and one could not be used
> to decline or justify other.

The point is that we shouldn't need to rely on what is formally a
hint. It might be useful to use the hint to decide whether or not
freezing now actually makes sense, but that isn't the same thing as
relying on the hint (we could make the same decision for a number of
different reasons).

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

You might as well ask me why I didn't do any number of other things. I
actually wrote a patch that made FreezeMultiXactId() more aggressive
about this sort of thing (setting HEAP_XMAX_INVALID) that targeted
Postgres 16. That worked by noticing that every member XID was at
least before OldestXmin, even when the MXID itself was >=3D OldestMxact.
That didn't go anywhere, even though it was a perfectly valid
optimization.

It's true that FreezeMultiXactId() optimizations like this are
possible. So what?

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

In what sense is there an inconsistency?

I think that you must mean that we need to do the same thing for the
!MultiXactIdIsValid() case and the already-HEAP_XMAX_INVALID case. But
I don't think that that's any meaningful kind of inconsistency. It's
*also* what we do with plain XIDs. If anything, the problem is that
we're *too* consistent (ideally we *would* treat MultiXacts
differently).

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

Oooops!

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

This is the first email that anybody has used the word "optimization".
We've been discussing this as if it was a bug. You introduced the
topic of optimization 3 seconds ago. Remember?

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

We don't actually trust the hints in any way. We always run checks
inside heap_pre_freeze_checks(), rather than assuming that the hints
are accurate.

--=20
Peter Geoghegan