Re: tweak to a few index tests to hits ambuildempty() routine.
Alvaro Herrera 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
Re: tweak to a few index tests to hits ambuildempty() routine.
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/
Re: tweak to a few index tests to hits ambuildempty() routine.
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
Re: tweak to a few index tests to hits ambuildempty() routine.
a.kozhemya...@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=2022-09-25%2001%3A01%3A13 > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra=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
Re: tweak to a few index tests to hits ambuildempty() routine.
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=2022-09-25%2001%3A01%3A13 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra=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.kozhemya...@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.
Re: tweak to a few index tests to hits ambuildempty() routine.
On Wed, Sep 21, 2022 at 02:10:42PM +0700, a.kozhemya...@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.
Re: tweak to a few index tests to hits ambuildempty() routine.
On 2022-Sep-21, a.kozhemya...@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)
Re: tweak to a few index tests to hits ambuildempty() routine.
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 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
Re: tweak to a few index tests to hits ambuildempty() routine.
On Wed, Sep 14, 2022 at 12:16 PM 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
Re: tweak to a few index tests to hits ambuildempty() routine.
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.kozhemya...@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.
Re: tweak to a few index tests to hits ambuildempty() routine.
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.
Re: tweak to a few index tests to hits ambuildempty() routine.
On Sun, May 22, 2022 at 04:24:16PM +1200, Thomas Munro wrote: > On Sat, May 21, 2022 at 6:15 PM Noah Misch wrote: > > On Mon, Apr 25, 2022 at 10:05:08AM -0400, Tom Lane wrote: > > > Alvaro Herrera 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.
Re: tweak to a few index tests to hits ambuildempty() routine.
On Sat, May 21, 2022 at 6:15 PM Noah Misch wrote: > On Mon, Apr 25, 2022 at 10:05:08AM -0400, Tom Lane wrote: > > Alvaro Herrera 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.
Re: tweak to a few index tests to hits ambuildempty() routine.
On Mon, Apr 25, 2022 at 10:05:08AM -0400, Tom Lane wrote: > Alvaro Herrera 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. Author: Noah Misch Commit: Noah Misch Use --no-unlogged-table-data in t/027_stream_regress.pl. This removes the need to drop unlogged relations in the src/test/regress suite, like commit dec8ad367e46180f826d5b6dc820fbecba1b71d2 did. Reviewed by FIXME. Discussion: https://postgr.es/m/39945.1650895...@sss.pgh.pa.us diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl index fdb4ea0..7982ac0 100644 --- a/src/test/recovery/t/027_stream_regress.pl +++ b/src/test/recovery/t/027_stream_regress.pl @@ -100,7 +100,8 @@ $node_primary->wait_for_catchup($node_standby_1, 'replay', command_ok( [ '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 ], 'dump primary server'); command_ok(
Re: tweak to a few index tests to hits ambuildempty() routine.
Alvaro Herrera 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
Re: tweak to a few index tests to hits ambuildempty() routine.
On Mon, Apr 25, 2022 at 7:23 PM Alvaro Herrera 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
Re: tweak to a few index tests to hits ambuildempty() routine.
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.8010...@sigaev.ru
Re: tweak to a few index tests to hits ambuildempty() routine.
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 HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: tweak to a few index tests to hits ambuildempty() routine.
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)
Re: tweak to a few index tests to hits ambuildempty() routine.
On Mon, Nov 29, 2021 at 10:34 AM Amul Sul 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
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 v1_index_test_for_ambuildempty.patch Description: Binary data
