tweak to a few index tests to hits ambuildempty() routine.

On Mon, Nov 29, 2021 at 10:34 AM Amul Sul <sulamul@gmail.com> wrote:

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.

+1 for the idea as it does the better code coverage.

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

--
Rushabh Lathia

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:

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:

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 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&amp;dt=2022-09-25%2001%3A01%3A13
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&amp;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:

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&amp;dt=2022-09-25%2001%3A01%3A13
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&amp;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