Обсуждение: tweak to a few index tests to hits ambuildempty() routine.

Hi,

Attached patch is doing small changes to brin, gin & gist index tests
to use an unlogged table without changing the original intention of
those tests and that is able to hit ambuildempty() routing which is
otherwise not reachable by the current tests.

-- 
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

On 2021-Nov-29, Amul Sul wrote:

> Attached patch is doing small changes to brin, gin & gist index tests
> to use an unlogged table without changing the original intention of
> those tests and that is able to hit ambuildempty() routing which is
> otherwise not reachable by the current tests.

I added one change to include spgist too, which was uncovered, and
pushed this.

Thanks for the patch!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
               (http://archives.postgresql.org/pgsql-sql/2006-06/msg00265.php)

On 2022-Apr-25, Alvaro Herrera wrote:

> I added one change to include spgist too, which was uncovered, and
> pushed this.

Looking into the recoveryCheck failure in buildfarm.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On 2022-Apr-25, Alvaro Herrera wrote:

> On 2022-Apr-25, Alvaro Herrera wrote:
> 
> > I added one change to include spgist too, which was uncovered, and
> > pushed this.
> 
> Looking into the recoveryCheck failure in buildfarm.

Hmm, so 027_stream_regress.pl is not prepared to deal with any unlogged
tables that may be left in the regression database (which is what my
spgist addition did).  I first tried doing a TRUNCATE of the unlogged
table, but that doesn't work either, and it turns out that the
regression database does not have any UNLOGGED relations.  Maybe that's
something we need to cater for, eventually, but for now dropping the
table suffices.  I have pushed that.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes...  Fixed.
        http://archives.postgresql.org/message-id/482D1632.8010507@sigaev.ru

On Mon, Apr 25, 2022 at 7:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Apr-25, Alvaro Herrera wrote:
>
> > On 2022-Apr-25, Alvaro Herrera wrote:
> >
> > > I added one change to include spgist too, which was uncovered, and
> > > pushed this.
> >

Thanks for the commit with the improvement.

Regards,
Amul

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Hmm, so 027_stream_regress.pl is not prepared to deal with any unlogged
> tables that may be left in the regression database (which is what my
> spgist addition did).  I first tried doing a TRUNCATE of the unlogged
> table, but that doesn't work either, and it turns out that the
> regression database does not have any UNLOGGED relations.  Maybe that's
> something we need to cater for, eventually, but for now dropping the
> table suffices.  I have pushed that.

It does seem like the onus should be on 027_stream_regress.pl to
deal with that, rather than restricting what the core tests can
leave behind.

Maybe we could have it look for unlogged tables and drop them
before making the dumps?  Although I don't understand why
TRUNCATE wouldn't do the job equally well.

            regards, tom lane

On Mon, Apr 25, 2022 at 10:05:08AM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Hmm, so 027_stream_regress.pl is not prepared to deal with any unlogged
> > tables that may be left in the regression database (which is what my
> > spgist addition did).  I first tried doing a TRUNCATE of the unlogged
> > table, but that doesn't work either, and it turns out that the
> > regression database does not have any UNLOGGED relations.  Maybe that's
> > something we need to cater for, eventually, but for now dropping the
> > table suffices.  I have pushed that.
> 
> It does seem like the onus should be on 027_stream_regress.pl to
> deal with that, rather than restricting what the core tests can
> leave behind.

Yeah.  Using "pg_dumpall --no-unlogged-table-data", as attached, suffices.

> Maybe we could have it look for unlogged tables and drop them
> before making the dumps?  Although I don't understand why
> TRUNCATE wouldn't do the job equally well.

After TRUNCATE, one still gets a setval for sequences and a zero-row COPY for
tables.  When dumping a standby or using --no-unlogged-table-data, those
commands are absent.

On Sat, May 21, 2022 at 6:15 PM Noah Misch <noah@leadboat.com> wrote:
> On Mon, Apr 25, 2022 at 10:05:08AM -0400, Tom Lane wrote:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > Hmm, so 027_stream_regress.pl is not prepared to deal with any unlogged
> > > tables that may be left in the regression database (which is what my
> > > spgist addition did).  I first tried doing a TRUNCATE of the unlogged
> > > table, but that doesn't work either, and it turns out that the
> > > regression database does not have any UNLOGGED relations.  Maybe that's
> > > something we need to cater for, eventually, but for now dropping the
> > > table suffices.  I have pushed that.
> >
> > It does seem like the onus should be on 027_stream_regress.pl to
> > deal with that, rather than restricting what the core tests can
> > leave behind.
>
> Yeah.  Using "pg_dumpall --no-unlogged-table-data", as attached, suffices.

                'pg_dumpall', '-f', $outputdir . '/primary.dump',
-               '--no-sync', '-p', $node_primary->port
+               '--no-sync',  '-p', $node_primary->port,
+               '--no-unlogged-table-data'    # if unlogged, standby
has schema only

LGTM, except for the stray extra whitespace.  I tested by reverting
dec8ad36 locally, at which point "gmake check" still passed but "gmake
-C src/test/recovery/ check PROVE_TESTS=t/027_stream_regress.pl
PROVE_FLAGS=-v" failed, and then your change fixed that.

On Sun, May 22, 2022 at 04:24:16PM +1200, Thomas Munro wrote:
> On Sat, May 21, 2022 at 6:15 PM Noah Misch <noah@leadboat.com> wrote:
> > On Mon, Apr 25, 2022 at 10:05:08AM -0400, Tom Lane wrote:
> > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > > Hmm, so 027_stream_regress.pl is not prepared to deal with any unlogged
> > > > tables that may be left in the regression database (which is what my
> > > > spgist addition did).  I first tried doing a TRUNCATE of the unlogged
> > > > table, but that doesn't work either, and it turns out that the
> > > > regression database does not have any UNLOGGED relations.  Maybe that's
> > > > something we need to cater for, eventually, but for now dropping the
> > > > table suffices.  I have pushed that.
> > >
> > > It does seem like the onus should be on 027_stream_regress.pl to
> > > deal with that, rather than restricting what the core tests can
> > > leave behind.
> >
> > Yeah.  Using "pg_dumpall --no-unlogged-table-data", as attached, suffices.
> 
>                 'pg_dumpall', '-f', $outputdir . '/primary.dump',
> -               '--no-sync', '-p', $node_primary->port
> +               '--no-sync',  '-p', $node_primary->port,
> +               '--no-unlogged-table-data'    # if unlogged, standby
> has schema only
> 
> LGTM, except for the stray extra whitespace.

perltidy contributes the prior-line whitespace change.

Hi,

The commit 4fb5c794e5 eliminates the ginbulkdelete() test coverage 
provided by the commit 4c51a2d1e4 two years ago.
With the following Assert added:
@@ -571,7 +571,7 @@ ginbulkdelete(IndexVacuumInfo *info, 
IndexBulkDeleteResult *stats,
         Buffer          buffer;
         BlockNumber rootOfPostingTree[BLCKSZ / (sizeof(IndexTupleData) + 
sizeof(ItemId))];
         uint32          nRoot;
-
+       Assert(0);
         gvs.tmpCxt = AllocSetContextCreate(CurrentMemoryContext,
                                                                          
   "Gin vacuum temporary context",
                                                                          
   ALLOCSET_DEFAULT_SIZES);
I have check-world passed successfully.

Amul Sul писал 2021-11-29 12:04:
> Hi,
> 
> Attached patch is doing small changes to brin, gin & gist index tests
> to use an unlogged table without changing the original intention of
> those tests and that is able to hit ambuildempty() routing which is
> otherwise not reachable by the current tests.

I still wonder, if assert doesn't catch why that place is marked as 
covered here?
https://coverage.postgresql.org/src/backend/access/gin/ginvacuum.c.gcov.html

a.kozhemyakin@postgrespro.ru писал 2022-09-12 15:47:
> Hi,
> 
> The commit 4fb5c794e5 eliminates the ginbulkdelete() test coverage
> provided by the commit 4c51a2d1e4 two years ago.
> With the following Assert added:
> @@ -571,7 +571,7 @@ ginbulkdelete(IndexVacuumInfo *info,
> IndexBulkDeleteResult *stats,
>         Buffer          buffer;
>         BlockNumber rootOfPostingTree[BLCKSZ / (sizeof(IndexTupleData)
> + sizeof(ItemId))];
>         uint32          nRoot;
> -
> +       Assert(0);
>         gvs.tmpCxt = AllocSetContextCreate(CurrentMemoryContext,
> 
>     "Gin vacuum temporary context",
> 
>     ALLOCSET_DEFAULT_SIZES);
> I have check-world passed successfully.
> 
> Amul Sul писал 2021-11-29 12:04:
>> Hi,
>> 
>> Attached patch is doing small changes to brin, gin & gist index tests
>> to use an unlogged table without changing the original intention of
>> those tests and that is able to hit ambuildempty() routing which is
>> otherwise not reachable by the current tests.

On Wed, Sep 14, 2022 at 12:16 PM <a.kozhemyakin@postgrespro.ru> wrote:
>
> I still wonder, if assert doesn't catch why that place is marked as
> covered here?
> https://coverage.postgresql.org/src/backend/access/gin/ginvacuum.c.gcov.html
>

Probably other tests cover that.

Regards,
Amul

After analyzing this, I found out why we don't reach that Assert but we 
have coverage shown - firstly, it reached via another test, vacuum; 
secondly, it depends on the gcc optimization flag. We reach that Assert 
only when using -O0.
If we build with -O2 or -Og that function is not reached (due to 
different results of the heap_prune_satisfies_vacuum() check inside 
heap_page_prune()).
But as  the make checks mostly (including the buildfarm testing) 
performed  with -O2/-Og, it looks like that after 4fb5c794e5 we have 
lost the coverage provided by the 4c51a2d1e4.

Amul Sul писал 2022-09-14 14:28:
> On Wed, Sep 14, 2022 at 12:16 PM <a.kozhemyakin@postgrespro.ru> wrote:
>> 
>> I still wonder, if assert doesn't catch why that place is marked as
>> covered here?
>> https://coverage.postgresql.org/src/backend/access/gin/ginvacuum.c.gcov.html
>> 
> 
> Probably other tests cover that.
> 
> Regards,
> Amul

On 2022-Sep-21, a.kozhemyakin@postgrespro.ru wrote:

> After analyzing this, I found out why we don't reach that Assert but we have
> coverage shown - firstly, it reached via another test, vacuum; secondly, it
> depends on the gcc optimization flag. We reach that Assert only when using
> -O0.
> If we build with -O2 or -Og that function is not reached (due to different
> results of the heap_prune_satisfies_vacuum() check inside
> heap_page_prune()).
> But as  the make checks mostly (including the buildfarm testing) performed
> with -O2/-Og, it looks like that after 4fb5c794e5 we have lost the coverage
> provided by the 4c51a2d1e4.

Hmm, so if we merely revert the change to gin.sql then we still won't
get the coverage back?  I was thinking that a simple change would be to
revert the change from temp to unlogged for that table, and create
another unlogged table; but maybe that's not enough.  Do we need a
better test for GIN vacuuming that works regardless of the optimization
level?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)

On Wed, Sep 21, 2022 at 02:10:42PM +0700, a.kozhemyakin@postgrespro.ru wrote:
> After analyzing this, I found out why we don't reach that Assert but we have
> coverage shown - firstly, it reached via another test, vacuum; secondly, it
> depends on the gcc optimization flag. We reach that Assert only when using
> -O0.
> If we build with -O2 or -Og that function is not reached (due to different
> results of the heap_prune_satisfies_vacuum() check inside
> heap_page_prune()).

With "make check MAX_CONNECTIONS=1", does that difference between -O0 and -O2
still appear?  Compiler optimization shouldn't consistently change pruning
decisions.  It could change pruning decisions probabilistically, by changing
which parallel actions overlap.  If the difference disappears under
MAX_CONNECTIONS=1, the system is likely fine.

Yes, with MAX_CONNECTIONS=1 and -O2 the function ginbulkdelete() is 
reached while the vacuum test.
But my point is that after 4fb5c794e5 for most developer setups and 
buildfarm members, e.g.:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=guaibasaurus&dt=2022-09-25%2001%3A01%3A13
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&dt=2022-09-24%2020%3A40%3A00
the ginbulkdelete() most probably is not tested.
In other words, it seems that we've just lost the effect of 4c51a2d1e4:
Add a test case that exercises vacuum's deletion of empty GIN
posting pages.  Since this is a temp table, it should now work
reliably to delete a bunch of rows and immediately VACUUM.
Before the preceding commit, this would not have had the desired
effect, at least not in parallel regression tests.

Noah Misch писал 2022-09-25 00:20:
> On Wed, Sep 21, 2022 at 02:10:42PM +0700, a.kozhemyakin@postgrespro.ru 
> wrote:
>> After analyzing this, I found out why we don't reach that Assert but 
>> we have
>> coverage shown - firstly, it reached via another test, vacuum; 
>> secondly, it
>> depends on the gcc optimization flag. We reach that Assert only when 
>> using
>> -O0.
>> If we build with -O2 or -Og that function is not reached (due to 
>> different
>> results of the heap_prune_satisfies_vacuum() check inside
>> heap_page_prune()).
> 
> With "make check MAX_CONNECTIONS=1", does that difference between -O0 
> and -O2
> still appear?  Compiler optimization shouldn't consistently change 
> pruning
> decisions.  It could change pruning decisions probabilistically, by 
> changing
> which parallel actions overlap.  If the difference disappears under
> MAX_CONNECTIONS=1, the system is likely fine.

a.kozhemyakin@postgrespro.ru writes:
> But my point is that after 4fb5c794e5 for most developer setups and
> buildfarm members, e.g.:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=guaibasaurus&dt=2022-09-25%2001%3A01%3A13
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&dt=2022-09-24%2020%3A40%3A00
> the ginbulkdelete() most probably is not tested.
> In other words, it seems that we've just lost the effect of 4c51a2d1e4:
> Add a test case that exercises vacuum's deletion of empty GIN
> posting pages.

Yeah.  You can see that the coverage-test animal is not reaching it
anymore:
https://coverage.postgresql.org/src/backend/access/gin/ginvacuum.c.gcov.html

So it seems clear that 4fb5c794e5 made at least some coverage worse
not better.  I think we'd better rejigger it to add some new indexes
not repurpose old ones.

            regards, tom lane

I wrote:
> Yeah.  You can see that the coverage-test animal is not reaching it
> anymore:
> https://coverage.postgresql.org/src/backend/access/gin/ginvacuum.c.gcov.html

That's what it's saying *now*, but after rereading this whole thread
I see that it apparently said something different last week.  So the
coverage is probabilistic, which squares with this discussion and
with some tests I just did locally.  That's not good.  I shudder to
imagine how much time somebody might waste trying to locate a bug
in this area, if a test failure appears and disappears regardless
of code changes they make while chasing it.

I propose that we revert 4fb5c794e and instead add separate test
cases that just create unlogged indexes (I guess they don't actually
need to *do* anything with them?).  Looks like dec8ad367 could be
reverted as well, in view of 2f2e24d90.

            regards, tom lane

On 2022-Sep-25, Tom Lane wrote:

> That's what it's saying *now*, but after rereading this whole thread
> I see that it apparently said something different last week.  So the
> coverage is probabilistic, which squares with this discussion and
> with some tests I just did locally.  That's not good.

Completely agreed.

> I propose that we revert 4fb5c794e and instead add separate test
> cases that just create unlogged indexes (I guess they don't actually
> need to *do* anything with them?).

WFM.  I can do it next week, or feel free to do so if you want.

> Looks like dec8ad367 could be reverted as well, in view of 2f2e24d90.

Yeah, sounds good.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Sep-25, Tom Lane wrote:
>> I propose that we revert 4fb5c794e and instead add separate test
>> cases that just create unlogged indexes (I guess they don't actually
>> need to *do* anything with them?).

> WFM.  I can do it next week, or feel free to do so if you want.

On it now.

            regards, tom lane