public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Robert Haas <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Himanshu Upadhyaya <[email protected]>
Cc: Mark Dilger <[email protected]>
Subject: Re: HOT chain validation in verify_heapam()
Date: Thu, 23 Mar 2023 13:36:56 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoZAYzQZqyUparXy_ks3OEOfLD9-bEXt8N-2tS1qghX9gQ@mail.gmail.com>
References: <[email protected]>
<CAJ7c6TP5CJOkNfM5=Qt4+YmiryL0_UCYtw0LRXr6HcUJ956KTg@mail.gmail.com>
<CA+TgmoZoF5ctr=8GtyJQ6yyAn5h05gduR9Ue=9BKyq-UG1HYxw@mail.gmail.com>
<CAPF61jAtC-GuMakGHPE8jAQt2DoYU7sWinwOMBYNC+wa5iYP=w@mail.gmail.com>
<CAPF61jCar6CoQKZAL95K6ae=xUCOFKdGq+AxoO8T9628v7xoSw@mail.gmail.com>
<CAJ7c6TMtmtXy1Du54p77=ychkqen8e=NP829EO21hRe=ftZVDQ@mail.gmail.com>
<CA+TgmoZub-bCg=ORmbcCCbteQ2UQUc14T2GK+v+6-VLd-8yK4g@mail.gmail.com>
<[email protected]>
<[email protected]>
<CA+TgmoZAYzQZqyUparXy_ks3OEOfLD9-bEXt8N-2tS1qghX9gQ@mail.gmail.com>
Hi,
On 2023-03-23 15:37:15 -0400, Robert Haas wrote:
> On Wed, Mar 22, 2023 at 8:38 PM Andres Freund <[email protected]> wrote:
> > skink / valgrind reported in a while back and found another issue:
> >
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2023-03-22%2021%3A53%3A41
> >
> > ==2490364== VALGRINDERROR-BEGIN
> > ==2490364== Conditional jump or move depends on uninitialised value(s)
> > ==2490364== at 0x11D459F2: check_tuple_visibility (verify_heapam.c:1379)
> ...
> > ==2490364== Uninitialised value was created by a stack allocation
> > ==2490364== at 0x11D45325: check_tuple_visibility (verify_heapam.c:994)
>
> OK, so this is an interesting one. It's complaining about switch
> (xmax_status), because the get_xid_status(xmax, ctx, &xmax_status)
> used in the previous switch might not actually initialize xmax_status,
> and apparently didn't in this case. get_xid_status() does not set
> xmax_status except when it returns XID_BOUNDS_OK, and the previous
> switch falls through both in that case and also when get_xid_status()
> returns XID_INVALID. That seems like it must be the issue here. As far
> as I can see, this isn't related to any of the recent changes but has
> been like this since this code was introduced, so I'm a little
> confused about why it's only causing a problem now.
Could it be that the tests didn't exercise the path before?
> Nonetheless, here's a patch. I notice that there's a similar problem
> in another place, too. get_xid_status() is called a total of five
> times and it looks like only three of them got it right. I suppose
> that if this is correct we should back-patch it.
Yea, I think you're right.
> + report_corruption(ctx,
> + pstrdup("xmin is invalid"));
Not a correctnes issue: Nearly all callers to report_corruption() do a
psprintf(), the remaining a pstrdup(), as here. Seems like it'd be cleaner to
just make report_corruption() accept a format string?
Greetings,
Andres Freund
view thread (3+ 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], [email protected], [email protected], [email protected]
Subject: Re: HOT chain validation in verify_heapam()
In-Reply-To: <[email protected]>
* 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