pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-03-28 b7cea58822 453f53821f e3363cda9c amcheck: In verify_heapam, allows tuples with xmin 0.
* pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-24 15:13 Robert Haas <rhaas@postgresql.org>
0 siblings, 1 reply; 7+ messages in thread
From: Robert Haas @ 2023-03-24 15:13 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
In such cases, get_xid_status() doesn't set its output parameter (the
third argument), so we shouldn't fall through to code which will test
the value of that parameter. There are five existing calls to
get_xid_status(), three of which seem to already handle this case
properly. This commit tries to fix the other two.
If we're checking xmin and find that it is invalid (i.e. 0) just
report that as corruption, similar to what's already done in the
three cases that seem correct. If we're checking xmax and find
that's invalid, that's fine: it just means that the tuple hasn't
been updated or deleted.
Thanks to Andres Freund and valgrind for finding this problem, and
also to Andres for having a look at the patch. This bug seems to go
all the way back to where verify_heapam was first introduced, but
wasn't detected until recently, possibly because of the new test cases
added for update chain verification. Back-patch to v14, where this
code showed up.
Discussion: http://postgr.es/m/CA+TgmoZAYzQZqyUparXy_ks3OEOfLD9-bEXt8N-2tS1qghX9gQ@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/e88754a1965c0f40a723e6e46d670cacda9e19bd
Modified Files
--------------
contrib/amcheck/verify_heapam.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-25 22:24 Peter Geoghegan <pg@bowt.ie>
parent: Robert Haas <rhaas@postgresql.org>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Geoghegan @ 2023-03-25 22:24 UTC (permalink / raw)
To: Robert Haas <rhaas@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org
On Fri, Mar 24, 2023 at 8:13 AM Robert Haas <rhaas@postgresql.org> wrote:
> If we're checking xmin and find that it is invalid (i.e. 0) just
> report that as corruption, similar to what's already done in the
> three cases that seem correct. If we're checking xmax and find
> that's invalid, that's fine: it just means that the tuple hasn't
> been updated or deleted.
What about aborted speculative insertions? See
heap_abort_speculative(), which directly sets the speculatively
inserted heap tuple's xmin to InvalidTransactionId/zero.
It probably does make sense to keep something close to this check --
it just needs to account for speculative insertions to avoid false
positive reports of corruption. We could perform cross-checks against
a tuple whose xmin is InvalidTransactionId/zero to verify that it
really is from an aborted speculative insertion, to the extent that
that's possible. For example, such a tuple can't be a heap-only tuple,
and it can't have any xmax value other than InvalidTransactionId/zero.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-27 17:17 Robert Haas <robertmhaas@gmail.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 7+ messages in thread
From: Robert Haas @ 2023-03-27 17:17 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-hackers
On Sat, Mar 25, 2023 at 6:25 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Fri, Mar 24, 2023 at 8:13 AM Robert Haas <rhaas@postgresql.org> wrote:
> > If we're checking xmin and find that it is invalid (i.e. 0) just
> > report that as corruption, similar to what's already done in the
> > three cases that seem correct. If we're checking xmax and find
> > that's invalid, that's fine: it just means that the tuple hasn't
> > been updated or deleted.
>
> What about aborted speculative insertions? See
> heap_abort_speculative(), which directly sets the speculatively
> inserted heap tuple's xmin to InvalidTransactionId/zero.
Oh, dear. I didn't know about that case.
> It probably does make sense to keep something close to this check --
> it just needs to account for speculative insertions to avoid false
> positive reports of corruption. We could perform cross-checks against
> a tuple whose xmin is InvalidTransactionId/zero to verify that it
> really is from an aborted speculative insertion, to the extent that
> that's possible. For example, such a tuple can't be a heap-only tuple,
> and it can't have any xmax value other than InvalidTransactionId/zero.
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-27 18:34 Peter Geoghegan <pg@bowt.ie>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Geoghegan @ 2023-03-27 18:34 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On Mon, Mar 27, 2023 at 10:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > What about aborted speculative insertions? See
> > heap_abort_speculative(), which directly sets the speculatively
> > inserted heap tuple's xmin to InvalidTransactionId/zero.
>
> Oh, dear. I didn't know about that case.
A big benefit of having extensive amcheck coverage is that it
effectively centralizes information about the on-disk format, in an
easy to understand way, and (over time) puts things on a more rigorous
footing. Now it'll be a lot harder for somebody else to overlook that
case in the future, which is good. Things are trending in the right
direction.
> > It probably does make sense to keep something close to this check --
> > it just needs to account for speculative insertions to avoid false
> > positive reports of corruption. We could perform cross-checks against
> > a tuple whose xmin is InvalidTransactionId/zero to verify that it
> > really is from an aborted speculative insertion, to the extent that
> > that's possible. For example, such a tuple can't be a heap-only tuple,
> > and it can't have any xmax value other than InvalidTransactionId/zero.
>
> 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.
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.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-27 20:17 Robert Haas <robertmhaas@gmail.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 7+ messages in thread
From: Robert Haas @ 2023-03-27 20:17 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-hackers
On Mon, Mar 27, 2023 at 2:34 PM Peter Geoghegan <pg@bowt.ie> 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:
0001-amcheck-In-verify_heapam-allows-tuples-with-xmin-0.patch (application/octet-stream, 1.2K) download
From 199b1468a67bdbe6e8493339fc78f5bd1d406241 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
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)
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-27 20:51 Peter Geoghegan <pg@bowt.ie>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Geoghegan @ 2023-03-27 20:51 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On Mon, Mar 27, 2023 at 1:17 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Patch attached.
This is fine, as far as it goes. Obviously it fixes the immediate problem.
> > 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.
I don't think that it's a fringe benefit; it's just not necessarily of
direct benefit to amcheck users.
Before the HOT chain validation patch went in, it was unclear whether
certain conceivable on-disk states should constitute corruption. In
particular, it wasn't clear to anybody whether or not it was okay for
an LP_REDIRECT to point to an LP_DEAD until recently (and probably
other things besides that). I don't think that we should assume that
the easy part is abstractly defining corruption, while the hard part
is writing the tool to check for the corruption. Sometimes it is, but
I think that it's often the other way around.
> 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.
That definitely could be true, but I don't think that it's terribly
much extra effort in most cases.
> 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.
A related way of looking at it (that I also find appealing) is that
it's often easier (far easier) to just have the check, and be done
with it. Of course there is bound to be uncertainty about how useful
any given check might be; we're looking for something that is
theoretically never supposed to happen. Why not just assume that it
might matter if it's not costing very much to check for it?
This is quite a different mentality than the one we bring to core
heapam code, where it's quite natural to just avoid strange corner
cases in the on-disk format like the plague. The risk profile is
totally different for amcheck code. Within amcheck, I'd rather go too
far than not go far enough.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.
@ 2023-03-28 20:28 Robert Haas <robertmhaas@gmail.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 7+ messages in thread
From: Robert Haas @ 2023-03-28 20:28 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-hackers
On Mon, Mar 27, 2023 at 4:52 PM Peter Geoghegan <pg@bowt.ie> wrote:
> This is fine, as far as it goes. Obviously it fixes the immediate problem.
OK, I've committed and back-patched this fix to v14, just like the
erroneous commit that created the issue.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-03-28 20:28 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-24 15:13 pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <rhaas@postgresql.org>
2023-03-25 22:24 ` Peter Geoghegan <pg@bowt.ie>
2023-03-27 17:17 ` Robert Haas <robertmhaas@gmail.com>
2023-03-27 18:34 ` Peter Geoghegan <pg@bowt.ie>
2023-03-27 20:17 ` Robert Haas <robertmhaas@gmail.com>
2023-03-27 20:51 ` Peter Geoghegan <pg@bowt.ie>
2023-03-28 20:28 ` Robert Haas <robertmhaas@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox