Wow.. This is embarrassing.. *^^*. At Thu, 21 Nov 2019 16:01:07 +0900 (JST), Kyotaro Horiguchi <horikyota....@gmail.com> wrote in > I should have replied this first. > > At Sun, 17 Nov 2019 20:54:34 -0800, Noah Misch <n...@leadboat.com> wrote in > > On Tue, Nov 05, 2019 at 02:53:35PM -0800, Noah Misch wrote: > > > I started pre-commit editing on 2019-10-28, and comment+README updates > > > have > > > been the largest part of that. I'll check my edits against the things you > > > list here, and I'll share on-list before committing. I've now marked the > > > CF > > > entry Ready for Committer. > > > > Having dedicated many days to that, I am attaching v24nm. I know of two > > remaining defects: > > > > === Defect 1: gistGetFakeLSN() > > > > When I modified pg_regress.c to use wal_level=minimal for all suites, > > src/test/isolation/specs/predicate-gist.spec failed the assertion in > > gistGetFakeLSN(). One could reproduce the problem just by running this > > sequence in psql: > > > > begin; > > create table gist_point_tbl(id int4, p point); > > create index gist_pointidx on gist_point_tbl using gist(p); > > insert into gist_point_tbl (id, p) > > select g, point(g*10, g*10) from generate_series(1, 1000) g; > > > > I've included a wrong-in-general hack to make the test pass. I see two main > > options for fixing this: > > > > (a) Introduce an empty WAL record that reserves an LSN and has no other > > effect. Make GiST use that for permanent relations that are skipping WAL. > > Further optimizations are possible. For example, we could use a > > backend-local > > counter (like the one gistGetFakeLSN() uses for temp relations) until the > > counter is greater a recent real LSN. That optimization is probably too > > clever, though it would make the new WAL record almost never appear. > > > > (b) Exempt GiST from most WAL skipping. GiST index build could still skip > > WAL, but it would do its own smgrimmedsync() in addition to the one done at > > commit. Regular GiST mutations would test RELPERSISTENCE_PERMANENT instead > > of > > RelationNeedsWal(), and we'd need some hack for index_copy_data() and > > possibly > > other AM-independent code that skips WAL. > > > > Overall, I like the cleanliness of (a). The main argument for (b) is that > > it > > ensures we have all the features to opt-out of WAL skipping, which could be > > useful for out-of-tree index access methods. (I think we currently have the > > features for a tableam to do so, but not for an indexam to do so.) > > Overall, I > > lean toward (a). Any other ideas or preferences? > > I don't like (b), too. > > What we need there is any sequential numbers for page LSN but really > compatible with real LSN. Couldn't we use GetXLogInsertRecPtr() in the
> case? Or, I'm not sure but I suppose that nothing happenes when > UNLOGGED GiST index gets turned into LOGGED one. Yes, I just forgot to remove these lines when writing the following. > Rewriting table like SET LOGGED will work but not realistic. > > > === Defect 2: repetitive work when syncing many relations > > > > For deleting relfilenodes, smgrDoPendingDeletes() collects a list for > > smgrdounlinkall() to pass to DropRelFileNodesAllBuffers(), which is > > sophisticated about optimizing the shared buffers scan. Commit 279628a > > introduced that, in 2013. I think smgrDoPendingSyncs() should do likewise, > > to > > further reduce the chance of causing performance regressions. (One could, > > however, work around the problem by raising wal_skip_threshold.) Kyotaro, > > if > > you agree, could you modify v24nm to implement that? > > Seems reasonable. Please wait a minite. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
