Thread: Re: Using read_stream in index vacuum

Hi Andrey,

On Sat, Oct 19, 2024 at 5:39 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> Hi hackers!
>
> On a recent hacking workshop [0] Thomas mentioned that patches using new API would be welcomed.
> So I prototyped streamlining of B-tree vacuum for a discussion.
> When cleaning an index we must visit every index tuple, thus we uphold a special invariant:
> After checking a trailing block, it must be last according to subsequent RelationGetNumberOfBlocks(rel) call.
>
> This invariant does not allow us to completely replace block loop with streamlining. That's why streamlining is done
onlyfor number of blocks returned by first RelationGetNumberOfBlocks(rel) call. A tail is processed with regular
ReadBufferExtended().

I'm wondering why is the case, ISTM that we can do *p.current_blocknum
= scanblkno*
and *p.last_exclusive = num_pages* in each loop of the outer for?

+ /* We only streamline number of blocks that are know at the beginning */
know -> known

+ * However, we do not depent on it much, and in future ths
+ * expetation might change.

depent -> depend
ths -> this
expetation -> expectation

>
> Also, it's worth mentioning that we have to jump to the left blocks from a recently split pages. We also do it with
regularReadBufferExtended(). That's why signature btvacuumpage() now accepts a buffer, not a block number. 
>
>
> I've benchmarked the patch on my laptop (MacBook Air M3) with following workload:
> 1. Initialization
> create unlogged table x as select random() r from generate_series(1,1e7);
> create index on x(r);
> create index on x(r);
> create index on x(r);
> create index on x(r);
> create index on x(r);
> create index on x(r);
> create index on x(r);
> vacuum;
> 2. pgbench with 1 client
> insert into x select random() from generate_series(0,10) x;
> vacuum x;
>
> On my laptop I see ~3% increase in TPS of the the pgbench (~ from 101 to 104), but statistical noise is very
significant,bigger than performance change. Perhaps, a less noisy benchmark can be devised. 
>
> What do you think? If this approach seems worthwhile, I can adapt same technology to other AMs.
>

I think this is a use case where the read stream api fits very well, thanks.

>
> Best regards, Andrey Borodin.
>
> [0] https://rhaas.blogspot.com/2024/08/postgresql-hacking-workshop-september.html
>


--
Regards
Junwang Zhao

> On 19 Oct 2024, at 20:41, Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> I'm wondering why is the case, ISTM that we can do *p.current_blocknum
> = scanblkno*
> and *p.last_exclusive = num_pages* in each loop of the outer for?

Thanks for reviewing!
AFAIK we cannot restart stream if it's finished, so we have a race condition of main loop and callback caller.
Resolving this race condition would make code much more complex for a relatively small benefit.

I'll address typos in next patch version, thank you for looking into this.


Best regards, Andrey Borodin.

> On 20 Oct 2024, at 15:16, Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> I'm not sure if I did not express myself correctly, I didn't mean to
> restart the stream,
> I mean we can create a new stream for each outer loop, I attached a
> refactor 0002
> based on your 0001, correct me if I'm wrong.

I really like how the code looks with this refactoring. But I think we need some input from Thomas.
Is it OK if we start handful of streams for 1 page at the end of vacuum scan? How costly is to start a new scan?


Best regards, Andrey Borodin.

On Sun, Oct 20, 2024 at 10:19 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 20 Oct 2024, at 15:16, Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > I'm not sure if I did not express myself correctly, I didn't mean to
> > restart the stream,
> > I mean we can create a new stream for each outer loop, I attached a
> > refactor 0002
> > based on your 0001, correct me if I'm wrong.
>
> I really like how the code looks with this refactoring. But I think we need some input from Thomas.
> Is it OK if we start handful of streams for 1 page at the end of vacuum scan? How costly is to start a new scan?

You shouldn't need either loop in btvacuumscan().

For the inner loop:

for (; scanblkno < num_pages; scanblkno++)
{
Buffer buf = read_stream_next_buffer(stream, NULL);
btvacuumpage(&vstate, buf);
if (info->report_progress)
pgstat_progress_update_param(PROGRESS_SCAN_BLOCKS_DONE,
scanblkno);
}

you should just be able to be do something like

Buffer buf = read_stream_next_buffer(stream, NULL);
if (BufferIsValid(buf))
      btvacuumpage(&vstate, buf);

Obviously I am eliding some details and clean-up and such. But your
read stream callback should be responsible for advancing the block
number and thus you shouldn't need to loop like this in
btvacuumscan().

The whole point of the read stream callback provided by the caller is
that the logic to get the next block should be contained there
(read_stream_get_block() is an exception to this).

For the outer loop,  I feel like we have options. For example, maybe
the read stream callback can call RelationGetNumberOfBlocks(). I mean
maybe we don't want to have to take a relation extension lock in a
callback.

So, alternatively, we could add some kind of restartable flag to the
read stream API. So, after the callback returns InvalidBlockNumber and
the read_stream_next_buffer() returns NULL, we could call something
like read_stream_update() or read_stream_restart() or something. We
would have updated the BlockRangeReadStreamPrivate->last_exclusive
value. In your case it might not be substantially different
operationally than making new read streams (since you are not
allocating memory for a per-buffer data structure). But, I think the
code would read much better than making new read stream objects in a
loop.

- Melanie

On Mon, Oct 21, 2024 at 3:34 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> For the outer loop,  I feel like we have options. For example, maybe
> the read stream callback can call RelationGetNumberOfBlocks(). I mean
> maybe we don't want to have to take a relation extension lock in a
> callback.

Also, given this note in btvacuumscan:

     * XXX: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't
     * think the use of the extension lock is still required.

Maybe we can stop requiring the extension lock and then I think it
might be okay to call RelationGetNumberOfBlocks() in the callback.
Probably needs more thought though.

- Melanie

On Mon, Oct 21, 2024 at 4:49 PM Andrei Borodin <x4mmm@yandex-team.ru> wrote:
>
> 21.10.2024, 22:34, "Melanie Plageman" <melanieplageman@gmail.com>:
>
> The whole point of the read stream callback provided by the caller is
> that the logic to get the next block should be there
>
> We must get number of blocks after examining last block. But callback returning EOF might be called before. With
currentAPI we have to restart. 
>
> Removing extension lock will not change this.

I was suggesting you call RelationGetNumberOfBlocks() once
current_block == last_exclusive in the callback itself.

- Melanie

> On 22 Oct 2024, at 00:05, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> I was suggesting you call RelationGetNumberOfBlocks() once
> current_block == last_exclusive in the callback itself.

Consider following sequence of events:

0. We schedule some buffers for IO
1. We call RelationGetNumberOfBlocks() in callback when current_block == last_exclusive and return InvalidBlockNumber
tosignal EOF 
After this:
2. Some page is getting split into new page with number last_exclusive
3. Buffers from IO are returned and vacuumed, but not with number last_exclusive, because it was not scheduled

Maybe I'm missing something...


Best regards, Andrey Borodin.

On Tue, Oct 22, 2024 at 2:30 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 22 Oct 2024, at 00:05, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > I was suggesting you call RelationGetNumberOfBlocks() once
> > current_block == last_exclusive in the callback itself.
>
> Consider following sequence of events:
>
> 0. We schedule some buffers for IO
> 1. We call RelationGetNumberOfBlocks() in callback when current_block == last_exclusive and return InvalidBlockNumber
tosignal EOF 
> After this:
> 2. Some page is getting split into new page with number last_exclusive
> 3. Buffers from IO are returned and vacuumed, but not with number last_exclusive, because it was not scheduled

Ah, right, the callback might return InvalidBlockNumber far before
we've actually read (and vacuumed) the blocks it is specifying.

I ran into something similar when trying to use the read stream API
for index prefetching. I added TIDs from the index to a queue that was
passed to the read stream and available in the callback. When the
queue was empty, I needed to check if there were more index entries
and, if so, add more TIDs to the queue (instead of ending the read
stream). So, I wanted some way for the callback to return
InvalidBlockNumber when there might actually be more blocks to
request. This is a kind of "restarting" behavior.

In that case, though, the reason the callback couldn't get more TIDs
when the queue was empty was because of layering violations and not,
like in the case of btree vacuum, because the index might be in a
different state after vacuuming the "last" block. Perhaps there is a
way to make the read stream restartable, though.

I just can't help wondering if there is a way to refactor the code
(potentially in a more invasive way) to make it more natural to use
the read stream API here. I usually hate when people give me such
unhelpful review feedback, though. So, carry on.

- Melanie

On Wed, Oct 23, 2024 at 9:32 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 22 Oct 2024, at 16:42, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > Ah, right, the callback might return InvalidBlockNumber far before
> > we've actually read (and vacuumed) the blocks it is specifying.
>
> I've discussed the issue with Thomas on PGConf.eu and he proposed to use stream reset.
> PFA v3.

Yeah, read_stream_reset fits better.

The patch LGTM, thanks.

>
>
> Best regards, Andrey Borodin.



--
Regards
Junwang Zhao

On Wed, Oct 23, 2024 at 9:32 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 22 Oct 2024, at 16:42, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > Ah, right, the callback might return InvalidBlockNumber far before
> > we've actually read (and vacuumed) the blocks it is specifying.
>
> I've discussed the issue with Thomas on PGConf.eu and he proposed to use stream reset.

That approach seems promising.

> PFA v3.

Note that you don't check if buf is valid here and break out of the
inner loop when it is invalid.

for (; scanblkno < num_pages; scanblkno++)
{
Buffer buf = read_stream_next_buffer(stream, NULL);
btvacuumpage(&vstate, buf);
...
}

By doing read_stream_reset() before you first invoke
read_stream_next_buffer(), seems like you invalidate the distance set
in read_stream_begin_relation()

if (flags & READ_STREAM_FULL)
stream->distance = Min(max_pinned_buffers, io_combine_limit);

->

stream->distance = 1 in read_stream_reset()

I still don't really like the inner loop using scanblkno:

for (; scanblkno < num_pages; scanblkno++)
{
Buffer buf = read_stream_next_buffer(stream, NULL);
btvacuumpage(&vstate, buf);
if (info->report_progress)
pgstat_progress_update_param(PROGRESS_SCAN_BLOCKS_DONE,
scanblkno);
}

Since you already advance a block number in the callback, I find it
confusing to also use the block number as a loop condition here. I
think it would be clearer to loop on read_stream_next_buffer()
returning a valid buffer (and then move the progress reporting into
btvacuumpage()).

But, I think I would need to study this btree code more to do a more
informed review of the patch.

- Melanie

On Wed, Oct 23, 2024 at 4:29 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 23 Oct 2024, at 20:57, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> >
> > I'll think how to restructure flow there...
>
> OK, I've understood how it should be structured. PFA v5. Sorry for the noise.

I think this would be a bit nicer:

while (BufferIsValid(buf = read_stream_next_buffer(stream, NULL)))
{
       block = btvacuumpage(&vstate, buf);
       if (info->report_progress)
              pgstat_progress_update_param(PROGRESS_SCAN_BLOCKS_DONE, block);
}

Maybe change btvacuumpage() to return the block number to avoid the
extra BufferGetBlockNumber() calls (those add up).

While looking at this, I started to wonder if it isn't incorrect that
we are not calling pgstat_progress_update_param() for the blocks that
we backtrack and read in btvacuumpage() too (on master as well).
btvacuumpage() may actually have scanned more than one block, so...

Unrelated to code review, but btree index vacuum has the same issues
that kept us from committing streaming heap vacuum last release --
interactions between the buffer access strategy ring buffer size and
the larger reads -- one of which is an increase in the number of WAL
syncs and writes required. Thomas mentions it here [1] and here [2] is
the thread where he proposes adding vectored writeback to address some
of these issues.

- Melanie

[1]
https://www.postgresql.org/message-id/CA%2BhUKGKN3oy0bN_3yv8hd78a4%2BM1tJC9z7mD8%2Bf%2ByA%2BGeoFUwQ%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/flat/CA%2BhUKGK1in4FiWtisXZ%2BJo-cNSbWjmBcPww3w3DBM%2BwhJTABXA%40mail.gmail.com

> On 25 Oct 2024, at 00:55, Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> While writing this email, I realized I evicted buffers for the table
> and not the index. I will perform the test again. However,
> I would like to know your opinion on whether this looks like
> a valid test.

Well, yes, kind of, you need drop caches from index. And, perhaps, you can have more indexes. You also can disable
autovaccumand just restart postgres instead of iterating through buffer caches. 

I've asked Thomas about performance implications and he told me that converting stuff to streamlined API is not
expectedto have better performance. It's needed to have decent perfromance when DIRECT_IO will be involved. 

Thanks!


Best regards, Andrey Borodin.

Hi!

On Fri, 25 Oct 2024 at 17:01, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 25 Oct 2024, at 00:55, Rahila Syed <rahilasyed90@gmail.com> wrote:
> >
> > While writing this email, I realized I evicted buffers for the table
> > and not the index. I will perform the test again. However,
> > I would like to know your opinion on whether this looks like
> > a valid test.
>
> Well, yes, kind of, you need drop caches from index. And, perhaps, you can have more indexes. You also can disable
autovaccumand just restart postgres instead of iterating through buffer caches.
 
>
> I've asked Thomas about performance implications and he told me that converting stuff to streamlined API is not
expectedto have better performance. It's needed to have decent perfromance when DIRECT_IO will be involved.
 
>
> Thanks!
>
>
> Best regards, Andrey Borodin.
>

I noticed CI failure for this patch. This does not look like a flap.

[0] https://cirrus-ci.com/task/4527545259917312
[1]
https://api.cirrus-ci.com/v1/artifact/task/4527545259917312/log/src/test/modules/test_misc/tmp_check/log/regress_log_007_vacuum_btree

-- 
Best regards,
Kirill Reshke

On Mon, 18 Nov 2024 at 16:34, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 2 Nov 2024, at 02:36, Kirill Reshke <reshkekirill@gmail.com> wrote:
> >
> > I noticed CI failure for this patch. This does not look like a flap.
>
> Seems like vacuum did not start index cleanup. I’ve added "index_cleanup on".
> Thanks!
>
>
> Best regards, Andrey Borodin.

Hi!

0001 Looks mature. Some comments:
1)
>+# This ensures autovacuum do not run
>+$node->append_conf('postgresql.conf', 'autovacuum = off');
The other option is to set `autovacuum = off `in relation DDL. I'm not
sure which option is better.

2) Are these used?
my $psql_err = '';
my $psql_out = '';


Should we add tap testing to 0002 & 0003 like 0001 already has?


--
Best regards,
Kirill Reshke

On Thu, Nov 28, 2024 at 10:29 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> 0001 Looks mature. Some comments:
> 1)
> >+# This ensures autovacuum do not run
> >+$node->append_conf('postgresql.conf', 'autovacuum = off');
> The other option is to set `autovacuum = off `in relation DDL. I'm not
> sure which option is better.

Either is fine. Though perhaps it is better to turn it off globally in
this case since we might as well avoid wasting any resources.

> 2) Are these used?
> my $psql_err = '';
> my $psql_out = '';

They don't seem to be.

I'm looking at 0001 with the intent of committing it soon. Today I've
just been studying the test with the injection points.

My main thought is that you should rename the injection points to
something more descriptive. We want to make it clear why there are
two. Also nbtree-vacuum-2 is actually where we wait the first
iteration of the loop, so that is confusing.

Perhaps we also ought to pass parallel false in the vacuum command.

I spent time thinking about if there was a way to exercise the actual
backtracking code and test that index entries that should have been
cleaned up were left behind because they were moved from a page we
didn't process before the split started to one we did. I thought maybe
we could do something with pageinspect. But getting the timing right
seems too hard.

Anyway, it seems worth it to have a bit of coverage for needing
multiple passes in btvacuumscan(). And given the injection points
infrastructure, perhaps other btree vacuum tests could be added to
this file in the future.

- Melanie

On Tue, Mar 18, 2025 at 11:12 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> I'm looking at 0001 with the intent of committing it soon. Today I've
> just been studying the test with the injection points.

Now, I've reviewed 0001. I manually validated it does read combining etc.

Few comments:

I know I was the one that advocated calling read_stream_next_buffer()
in the while loop, but I've realized that I think we have to change it
so that we can call vacuum_delay_point() before calling
read_stream_next_buffer(). Otherwise we hold the buffer pin during
vacuum_delay_point(). We'll want to remove it from the top of
btvacuumpage() and add it in this loop:

        /* Iterate over pages, then loop back to recheck relation length */
        while(BufferIsValid(buf = read_stream_next_buffer(stream, NULL)))
        {
            BlockNumber current_block = btvacuumpage(&vstate, buf);
            if (info->report_progress)
                pgstat_progress_update_param(PROGRESS_SCAN_BLOCKS_DONE,
                                             current_block);
        }


and before the call to ReadBufferExtended() in the backtrack code path.

        buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL,
                                info->strategy);


Also, I think this comment could use a slight update.

/*
 * btvacuumpage --- VACUUM one page
 *
 * This processes a single page for btvacuumscan().  In some cases we must
 * backtrack to re-examine and VACUUM pages that were the scanblkno during
 */

scanblkno is a local variable but the comment was written when it was
a parameter. I think it would be more clear to refer to the passed in
"buf" variable instead.

- Melanie

> On 18 Mar 2025, at 20:37, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> I've reviewed 0001

Thanks!
I've added all suggested fixes as a separate patch step.
Except I did not rename injection points... I can't figure out descriptive names.
And delay point is now after the page is processed, not before.

FWIW I do not insist on committing the test, it was mostly necessary to validate that backtracking still works. I could
notcheck it manually. But other injection tests, surprisingly, seem to be stable enough across buildfarm. 


Best regards, Andrey Borodin.

On Tue, Mar 18, 2025 at 11:12 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> I'm looking at 0001 with the intent of committing it soon. Today I've
> just been studying the test with the injection points.
>
> My main thought is that you should rename the injection points to
> something more descriptive. We want to make it clear why there are
> two. Also nbtree-vacuum-2 is actually where we wait the first
> iteration of the loop, so that is confusing.

I actually think you could do the test with one injection point and
just wait on it twice

diff --git a/src/backend/access/nbtree/nbtree.c
b/src/backend/access/nbtree/nbtree.c
index fe7084edacd..79f4323f887 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -1091,7 +1091,6 @@ btvacuumscan(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,

         /* In 007_vacuum_btree test we need to coordinate two
distinguishable points here */
         INJECTION_POINT("nbtree-vacuum-1");
-        INJECTION_POINT("nbtree-vacuum-2");

         p.last_exclusive = num_pages;

diff --git a/src/test/modules/test_misc/t/007_vacuum_btree.pl
b/src/test/modules/test_misc/t/007_vacuum_btree.pl
index 2c69d2b477d..6d5f474b99a 100644
--- a/src/test/modules/test_misc/t/007_vacuum_btree.pl
+++ b/src/test/modules/test_misc/t/007_vacuum_btree.pl
@@ -37,7 +37,7 @@ $node->safe_psql('postgres', 'CREATE EXTENSION
injection_points;');

 # From this point, vacuum worker will wait at startup.
 $node->safe_psql('postgres',
-    "SELECT injection_points_attach('nbtree-vacuum-2', 'wait');");
+    "SELECT injection_points_attach('nbtree-vacuum-1', 'wait');");

 my $psql_session = $node->background_psql('postgres');

@@ -51,10 +51,7 @@ $psql_session->query_until(
     ));

 # Wait until an vacuum worker starts.
-$node->wait_for_event('client backend', 'nbtree-vacuum-2');
-
-$node->safe_psql('postgres',
-    "SELECT injection_points_attach('nbtree-vacuum-1', 'wait');");
+$node->wait_for_event('client backend', 'nbtree-vacuum-1');

 # Here's the key point of a test: during vacuum we add some page splits.
 # This will force vacuum into doing another scan thus reseting read stream.
@@ -62,18 +59,16 @@ $node->safe_psql('postgres',
     "insert into a select x from generate_series(1,3000) x;");

 $node->safe_psql('postgres',
-    "SELECT injection_points_detach('nbtree-vacuum-2');");
-$node->safe_psql('postgres',
-    "SELECT injection_points_wakeup('nbtree-vacuum-2');");
+    "SELECT injection_points_wakeup('nbtree-vacuum-1');");

-# Observe that second scan is reached.
 $node->wait_for_event('client backend', 'nbtree-vacuum-1');

-$node->safe_psql('postgres',
-    "SELECT injection_points_detach('nbtree-vacuum-1');");
 $node->safe_psql('postgres',
     "SELECT injection_points_wakeup('nbtree-vacuum-1');");

+$node->safe_psql('postgres',
+    "SELECT injection_points_detach('nbtree-vacuum-1');");
+
 ok($psql_session->quit);

 done_testing();

> On 18 Mar 2025, at 23:21, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> I actually think you could do the test with one injection point and
> just wait on it twice

I was not sure we can safely wake up on injection point and stop and the same point on next iteration.
But, apparently, it is safe, due to wait_counts in InjectionPointSharedState.

So, yes, your change to the test seems correct to me. We can do the test with just one injection point.


Best regards, Andrey Borodin.

On Wed, Mar 19, 2025 at 5:26 AM Andrey Borodin <x4mmm@yandex-team.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...

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 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.

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.

- Melanie

> On 21 Mar 2025, at 05:54, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Wed, Mar 19, 2025 at 5:26 AM Andrey Borodin <x4mmm@yandex-team.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
buildfiles. 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!

On Fri, Mar 21, 2025 at 8:19 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 21 Mar 2025, at 05:54, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > 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
buildfiles. 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.

There are tests with injection points in all different test
directories now. I think regress is the only one where it would be too
much of a pain.

I've committed the btree and gist read stream users. I think we can
come back to the test after feature freeze and make sure it is super
solid.

Looking at the spgist read stream user, I see you didn't convert
spgprocesspending(). It seems like you could write a callback that
uses the posting list and streamify this as well.

- Melanie

On Fri, Mar 21, 2025 at 3:23 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> I've committed the btree and gist read stream users. I think we can
> come back to the test after feature freeze and make sure it is super
> solid.

I've now committed the spgist vacuum user as well. I'll mark the CF
entry as completed.
I wonder if we should do GIN?

> Looking at the spgist read stream user, I see you didn't convert
> spgprocesspending(). It seems like you could write a callback that
> uses the posting list and streamify this as well.

It's probably not worth it -- since we process the pending list for
each page of the index.

- Melanie

> On 22 Mar 2025, at 00:23, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
>
> I've committed the btree and gist read stream users.

Cool! Thanks!

> I think we can
> come back to the test after feature freeze and make sure it is super
> solid.

+1.


> On 22 Mar 2025, at 02:54, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Fri, Mar 21, 2025 at 3:23 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>
>> I've committed the btree and gist read stream users. I think we can
>> come back to the test after feature freeze and make sure it is super
>> solid.
>
> I've now committed the spgist vacuum user as well. I'll mark the CF
> entry as completed.

That's great! Thank you!

> I wonder if we should do GIN?

GIN vacuum is a logical scan. Back in 2017 I was starting to work on it, but made some mistakes, that were reverted by
fd83c83from the released version. And I decided to back off for some time. Perhaps, now I can implement physical scan
forGIN, that could benefit from read stream. But I doubt I will find committer for this in 19, let alone 18. 

We can add some support for read stream for hashbulkdelete(): it's not that linear as B-tree, GiST and SP-GiST, it
scansonly beginning of hash buckets, but if buckets are small it might be more efficient. 

>> Looking at the spgist read stream user, I see you didn't convert
>> spgprocesspending(). It seems like you could write a callback that
>> uses the posting list and streamify this as well.
>
> It's probably not worth it -- since we process the pending list for
> each page of the index.

My understanding is that pending lists should be small on real workloads.

Thank you!


Best regards, Andrey Borodin.

> On 22 Mar 2025, at 02:54, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> committed

There's a BF failure with just these changes [0]. But IMO it's unrelated.
There are 2 failed tests:
1. 'activeslot slot invalidation is logged with vacuum on pg_authid' is very similar to what is discussed here [1]
2. '+ERROR: tuple concurrently deleted' in injection_points/isolation seems to be discussed here [2]

Thanks!


Best regards, Andrey Borodin.

[0] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-03-21%2019%3A09%3A59
[1] https://www.postgresql.org/message-id/flat/386386.1737736935%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/flat/20250304033442.11.nmisch%40google.com#050112012649a0e5077897a6a051d024

On Sun, Mar 23, 2025 at 1:02 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> There's a BF failure with just these changes [0]. But IMO it's unrelated.
> There are 2 failed tests:
> 1. 'activeslot slot invalidation is logged with vacuum on pg_authid' is very similar to what is discussed here [1]
> 2. '+ERROR: tuple concurrently deleted' in injection_points/isolation seems to be discussed here [2]

Yep, I saw the recovery/035_standby_logical_decoding skink failure and
concluded it was caused by a pre-existing issue that is already being
discussed. I hadn't yet investigated the injection point isolation
test failure. So, it is good to know it seems like it is a known
issue. Thanks for being on the lookout.

- Melanie