From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Aleksander Alekseev <aleksander(at)timescale(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Himanshu Upadhyaya <upadhyaya(dot)himanshu(at)gmail(dot)com>, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, Andres Freund <andres(at)anarazel(dot)de> |
Subject: | Re: HOT chain validation in verify_heapam() |
Date: | 2023-03-08 13:35:53 |
Message-ID: | CA+TgmoZoF5ctr=8GtyJQ6yyAn5h05gduR9Ue=9BKyq-UG1HYxw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Mar 8, 2023 at 5:35 AM Aleksander Alekseev
<aleksander(at)timescale(dot)com> wrote:
> I did, both with Meson and Autotools. All in all the patch looks very
> good, but I have a few little nitpicks.
Thank you for the nitpicks.
> + /* HOT chains should not intersect. */
> + if (predecessor[nextoffnum] != InvalidOffsetNumber)
> + {
> + report_corruption(&ctx,
> + psprintf("redirect line pointer
> points to offset %u, but offset %u also points there",
> + (unsigned) nextoffnum,
> (unsigned) predecessor[nextoffnum]));
> + continue;
> + }
> ```
>
> This type of corruption doesn't seem to be test-covered.
Himanshu, would you be able to try to write a test case for this? I
think you need something like this: update a tuple with a lower TID to
produce a tuple with a higher TID, e.g. (0,10) is updated to produce
(0,11). But then have a redirect line pointer that also points to the
result of the update, in this case (0,11).
> ```
> + /*
> + * If the next line pointer is a redirect, or if it's a tuple
> + * but the XMAX of this tuple doesn't match the XMIN of the next
> + * tuple, then the two aren't part of the same update chain and
> + * there is nothing more to do.
> + */
> + if (ItemIdIsRedirected(next_lp))
> + continue;
> ```
>
> lcov shows that the `continue` path is never executed. This is
> probably not a big deal however.
It might be good to have a negative test case for this, though. Let's
say we, e.g. update (0,1) to produce (0,2), but then abort. The page
is HOT-pruned. Then we add insert a new tuple at (0,2), HOT-update it
to produce (0,3), and commit. Then we HOT-prune again. Possibly we
could try to write a test case that verifies that this does NOT
produce any corruption indication.
> ```
> +$node->append_conf('postgresql.conf','max_prepared_transactions=100');
> ```
>
> From what I can tell this line is not needed.
That surprises me, because the new test cases involve preparing a
transaction, and by default max_prepared_transactions=0. So it seems
to me (without testing) that this ought to be required. Did you test
that it works without this setting?
The value of 100 seems a bit excessive, though. Most TAP tests seem to use 10.
--
Robert Haas
EDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | Nazir Bilal Yavuz | 2023-03-08 13:49:37 | Re: meson: Optionally disable installation of test modules |
Previous Message | Robert Haas | 2023-03-08 13:27:29 | Re: [PATCH] Support % wildcard in extension upgrade filenames |