Re: Using read_stream in index vacuum

From: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
To: Melanie Plageman <melanieplageman(at)gmail(dot)com>
Cc: Kirill Reshke <reshkekirill(at)gmail(dot)com>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, Junwang Zhao <zhjwpku(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Subject: Re: Using read_stream in index vacuum
Date: 2025-03-21 12:19:35
Message-ID: A11AFE6D-E565-4D8E-875C-FC06344284D2@yandex-team.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 21 Mar 2025, at 05:54, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>
> On Wed, Mar 19, 2025 at 5:26 AM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
>>
>> So, yes, your change to the test seems correct to me. We can do the test with just one injection point.
>
> Attached 0001 is what I plan to commit first thing tomorrow morning. I
> moved the vacuum_delay_point() so that we would call
> pgstat_progress_update_param() as soon as we were done processing the
> block (instead of doing vacuum_delay_point() first). This also makes
> it consistent with the other vacuum streaming read users. What do we
> even use the progress update for here, though? For heap vacuuming it
> makes sense because we report heap blocks vacuumed in
> pg_stat_progress_vacuum, but we don't do that for index blocks
> vacuumed...

OK, the patch looks good to me. But there is no test for stream restart in current patch. I’m OK with it, just noting.

>
> I plan to commit 0001 (the read stream user) without 0002 and then
> solicit more feedback on 0002.
>
> 0002 is a draft of the test. I want us to actually test the
> backtracking behavior. To do this, I think we need two injection
> points with the current injection point infrastructure. Which I don't
> love. I think if we could pass variables into INJECTION_POINT(), we
> could do the test with one injection point.
>
> I also don't love how "failing" for this test is just it hanging
> because the nbtree-vacuum-page-backtrack wait event never happens
> (there is no specific assertion or anything in the test).

I think timing out in 180s is OK as long as failure is not expected.

>
> I do like that I moved the nbtree-vacuum-page injection point to the
> top of nbtreevacuumpage because I think it can be used for other tests
> in the future.

Cool!

>
> I also think it is worth making a btree directory in src/test instead
> of adding this to src/test/modules/test_misc. In fact it might be
> worth moving the gin and brin tests out of src/test/modules and making
> a new "indexes" directory in src/test with gin, brin, and btree (also
> some spgist tests are in there somewhere) subdirectories.

Previously we groupped injection point tests in small number of modules, because it required additional changes to build files. And we only had like 3 or 5 tests.
I think if we have a handful of tests - it’s time to start organizing them in a structured way.

Thanks!

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Hayato Kuroda (Fujitsu) 2025-03-21 12:28:10 RE: Fix 035_standby_logical_decoding.pl race conditions
Previous Message Yura Sokolov 2025-03-21 12:15:36 Re: [RFC] Lock-free XLog Reservation from WAL