public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: Peter Geoghegan <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
Date: Mon, 27 Mar 2023 16:17:32 -0400
Message-ID: <CA+Tgmobi9X1UX8sfLQ4SS5u4TfJgg3-v5CX2HDs2Z73oHGr_Aw@mail.gmail.com> (raw)
In-Reply-To: <CAH2-WzmD0qbJpFNihJ5t-ESZBA5j6ukmO=VG1POG=nE_O82h-g@mail.gmail.com>
References: <[email protected]>
<CAH2-WzmEabzcPTxSY-NXKH6Qt3FkAPYHGQSe2PtvGgj17ZQkCw@mail.gmail.com>
<CA+TgmobHjMdp+TJKpxAyjqMAuQ5iKfE9HNhaSua2054rbcofdA@mail.gmail.com>
<CAH2-WzmD0qbJpFNihJ5t-ESZBA5j6ukmO=VG1POG=nE_O82h-g@mail.gmail.com>
On Mon, Mar 27, 2023 at 2:34 PM Peter Geoghegan <[email protected]> wrote:
> > Since this was back-patched, I think it's probably better to just
> > remove the error. We can introduce new validation if we want, but that
> > should probably be master-only.
>
> That makes sense.
Patch attached.
> I don't think that it's particularly likely that having refined
> aborted speculative insertion amcheck coverage will make a critical
> difference to any user, at any time. But "amcheck as documentation of
> the on-disk format" is reason enough to have it.
Sure, if someone feels like writing the code. I have to admit that I
have mixed feelings about this whole direction. In concept, I agree
with you entirely: a fringe benefit of having checks that tell us
whether or not a page is valid is that it helps to make clear what
page states we think are valid. In practice, however, the point you
raise in your first sentence weighs awfully heavily with me. Spending
a lot of energy on checks that are unlikely to catch practical
problems feels like it may not be the best use of time. I'm not sure
exactly where to draw the line, but it seems highly likely to be that
there are things we could deduce about the page that wouldn't be worth
the effort. For example, would we bother checking that a tuple with an
in-progress xmin does not have a smaller natts value than a tuple with
a committed xmin? Or that natts values are non-decreasing across a HOT
chain? I suspect there are even more obscure examples of things that
should be true but might not really be worth worrying about in the
code.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] 0001-amcheck-In-verify_heapam-allows-tuples-with-xmin-0.patch (1.2K, ../CA+Tgmobi9X1UX8sfLQ4SS5u4TfJgg3-v5CX2HDs2Z73oHGr_Aw@mail.gmail.com/2-0001-amcheck-In-verify_heapam-allows-tuples-with-xmin-0.patch)
download | inline diff:
From 199b1468a67bdbe6e8493339fc78f5bd1d406241 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Mon, 27 Mar 2023 14:36:03 -0400
Subject: [PATCH] amcheck: In verify_heapam, allows tuples with xmin 0.
Commit e88754a1965c0f40a723e6e46d670cacda9e19bd caused that case
to be reported as corruption, but Peter Geoghegan pointed out that
it can legitimately happen in the case of a speculative insertion
that aborts, so we'd better not flag it as corruption after all.
---
contrib/amcheck/verify_heapam.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index ce3114f4e6..f50ee9d29f 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1032,8 +1032,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok,
switch (get_xid_status(xmin, ctx, &xmin_status))
{
case XID_INVALID:
- report_corruption(ctx,
- pstrdup("xmin is invalid"));
+ /* Could be the result of a speculative insertion that aborted. */
return false;
case XID_BOUNDS_OK:
*xmin_commit_status_ok = true;
--
2.37.1 (Apple Git-137.1)
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
In-Reply-To: <CA+Tgmobi9X1UX8sfLQ4SS5u4TfJgg3-v5CX2HDs2Z73oHGr_Aw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox