Cool, thanks! > 2 янв. 2019 г., в 20:35, Heikki Linnakangas <hlinn...@iki.fi> написал(а): > > In patch #1, to do the vacuum scan in physical order: > ... > I think this is ready to be committed, except that I didn't do any testing. > We discussed the need for testing earlier. Did you write some test scripts > for this, or how have you been testing? Please see test I used to check left jumps for v18: 0001-Physical-GiST-scan-in-VACUUM-v18-with-test-modificat.patch 0002-Test-left-jumps-v18.patch
0002-Test-left-jumps-v18.patch
Description: Binary data
Description: Binary dataTo trigger FollowRight GiST sometimes forget to clear follow-right marker simulating crash of an insert. This fills logs with "fixing incomplete split" messages. Search for "REMOVE THIS" to disable these ill-behavior triggers. To trigger NSN jump GiST allocate empty page after every real allocation. In log output I see 2019-01-03 22:27:30.028 +05 [54596] WARNING: RESCAN TRIGGERED BY NSN WARNING: RESCAN TRIGGERED BY NSN 2019-01-03 22:27:30.104 +05 [54596] WARNING: RESCAN TRIGGERED BY FollowRight This means that code paths were really executed (for v18). > > Patch #2: > > * Bitmapset stores 32 bit signed integers, but BlockNumber is unsigned. So > this will fail with an index larger than 2^31 blocks. That's perhaps > academical, I don't think anyone will try to create a 16 TB GiST index any > time soon. But it feels wrong to introduce an arbitrary limitation like that. Looks like bitmapset is unsuitable for collecting block numbers due to the interface. Let's just create custom container for this? Or there is one other option: for each block number throw away sign bit and consider page potentially internal and potentially empty leaf if (blkno & 0x7FFFFFF) is in bitmapset. And then we will have to create at least one 17Tb GiST to check it actually works. > * I was surprised that the bms_make_empty() function doesn't set the 'nwords' > field to match the allocated size. Perhaps that was intentional, so that you > don't need to scan the empty region at the end, when you scan through all > matching bits? Still, seems surprising, and needs a comment at least. Explicitly set nwords to zero. Looking at the code now, I do not see this nwords==0 as a very good idea. Probably, it's effective, but it's hacky, creates implicit expectations in code. > * We're now scanning all internal pages in the 2nd phase. Not only those > internal pages that contained downlinks to empty leaf pages. That's probably > OK, but the comments need updating on that. Adjusted comments. That if before loop > if (vstate.emptyPages > 0) seems superfluous. But I kept it until we solve the problem with 31-bit bitmapset. > * In general, comments need to be fixed and added in several places. For > example, there's no clear explanation on what the "delete XID", stored in > pd_prune_xid, means. (I know what it is, because I'm familiar with the same > mechanism in B-tree, but it's not clear from the code itself.) I've added comment to GistPageSetDeleteXid() * In this check > if (GistPageIsDeleted(page) && > TransactionIdPrecedes(GistPageGetDeleteXid(page), RecentGlobalXmin)) I've switched using RecentGlobalDataXmin to RecentGlobalXmin, because we have done so in similar mechanics for GIN (for uniformity with B-tree). Thanks for working on this! Best regards, Andrey Borodin.
Description: Binary data
Description: Binary data
