Date: Mon, 1 Feb 2021 14:00:48 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit
 combination
Message-ID: <20210201170048.GA22366@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210124061758.GA11756@nol>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2021-Jan-24, Julien Rouhaud wrote:

> While working on pg14 compatibility for an extension relying on an apparently
> uncommon combination of FOR UPDATE and stored function calls, I hit some new
> Asserts introduced in 866e24d47db (Extend amcheck to check heap pages):
> 
> +	/*
> +	 * Do not allow tuples with invalid combinations of hint bits to be placed
> +	 * on a page.  These combinations are detected as corruption by the
> +	 * contrib/amcheck logic, so if you disable one or both of these
> +	 * assertions, make corresponding changes there.
> +	 */
> +	Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
> +			 (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)));
> 
> 
> I attach a simple self contained script to reproduce the problem, the last
> UPDATE triggering the Assert.

Maybe we should contest the idea that this is a sensible thing to Assert
against.  AFAICS this was originally suggested here:
https://www.postgresql.org/message-id/flat/CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA%40mail.gmail.com#ad215d0ee0606b5f67bbc57d011c96b8
and it appears now to have been a bad idea.  If I recall correctly,
HEAP_KEYS_UPDATED is supposed to distinguish locks/updates that don't
modify the key columns from those that do.  Since SELECT FOR UPDATE
stands for a future update that may modify arbitrary portions of the
tuple (including "key" columns), then it produces that bit, just as said
UPDATE or a DELETE; as opposed to SELECT FOR NO KEY UPDATE which stands
for a future UPDATE that will only change columns that aren't part of
any keys.

So I think that I misspoke earlier in this thread when I said this is a
bug, and that the right fix here is to remove the Assert() and change
amcheck to match.

Separately, maybe it'd also be good to have a test case based on
Julien's SQL snippet that produces this particular infomask combination
(and other interesting combinations) and passes them through VACUUM etc
to see that everything behaves correctly.

You could also argue the HEAP_KEYS_UPDATED is a misnomer and that we'd
do well to change its name, and update README.tuplock.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)