Re: HOT chain validation in verify_heapam()

From: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Himanshu Upadhyaya <upadhyaya(dot)himanshu(at)gmail(dot)com>, Aleksander Alekseev <aleksander(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: HOT chain validation in verify_heapam()
Date: 2023-03-07 18:29:47
Message-ID: CA491701-7A8A-4167-A43E-D3A9A2C34371@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Mar 7, 2023, at 10:16 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Mar 6, 2023 at 12:36 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> So it seems that we still don't have a patch where the
>> value of a variable called lp_valid corresponds to whether or not the
>> L.P. is valid.
>
> Here's a worked-over version of this patch. Changes:
>
> - I got rid of the code that sets lp_valid in funny places and instead
> arranged to have check_tuple_visibility() pass up the information on
> the XID status. That's important, because we can't casually apply
> operations like TransactionIdIsCommitted() to XIDs that, for all we
> know, might not even be in the range covered by CLOG. In such cases,
> we should not perform any HOT chain validation because we can't do it
> sensibly; the new code accomplishes this, and also reduces the number
> of CLOG lookups as compared with your version.
>
> - I moved most of the HOT chain checks from the loop over the
> predecessor[] array to the loop over the successor[] array. It didn't
> seem to have any value to put them in the third loop; it forces us to
> expend extra code to distinguish between redirects and tuples,
> information that we already had in the second loop. The only check
> that seems to make sense to do in that last loop is the one for a HOT
> chain that starts with a HOT tuple, which can't be done any earlier.
>
> - I realized that your patch had a guard against setting the
> predecessor[] when it was set already only for tuples, not for
> redirects. That means if a redirect pointed into the middle of a HOT
> chain we might not report corruption appropriately. I fixed this and
> reworded the associated messages a bit.
>
> - Assorted cosmetic and comment changes.
>
> I think this is easier to follow and more nearly correct, but what do
> you (and others) think?

Thanks, Robert. Quickly skimming over this patch, it looks like something reviewable. Your changes to t/004_verify_heapam.pl appear to be consistent with how that test was intended to function.

Note that I have not tried any of this yet.


Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2023-03-07 18:30:14 Re: Time delayed LR (WAS Re: logical replication restrictions)
Previous Message Andres Freund 2023-03-07 18:18:44 Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)