Re: [HACKERS] WAL logging problem in 9.4.3?

On Fri, Oct 25, 2019 at 1:13 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> Hello. Thanks for the comment.
>
> # Sorry in advance for possilbe breaking the thread.
>
> > MarkBufferDirtyHint() writes WAL even when rd_firstRelfilenodeSubid or
> > rd_createSubid is set; see attached test case. It needs to skip WAL whenever
> > RelationNeedsWAL() returns false.
>
> Thanks for pointing out that. And the test patch helped me very much.
>
> Most of callers can tell that to the function, but SetHintBits()
> cannot easily. Rather I think we shouldn't even try to do
> that. Instead, In the attached, MarkBufferDirtyHint() asks storage.c
> for sync-pending state of the relfilenode for the buffer. In the
> attached patch (0003) RelFileNodeSkippingWAL loops over pendingSyncs
> but it is called only at the time FPW is added so I believe it doesn't
> affect performance so much. However, we can use hash for pendingSyncs
> instead of liked list. Anyway the change is in its own file
> v21-0003-Fix-MarkBufferDirtyHint.patch, which will be merged into
> 0002.
>
> AFAICS all XLogInsert is guarded by RelationNeedsWAL() or in the
> non-wal_minimal code paths.
>
> > Cylinder and track sizes are obsolete as user-visible concepts. (They're not
> > onstant for a given drive, and I think modern disks provide no way to read
> > the relevant parameters.) I like the name "wal_skip_threshold", and my second
>
> I strongly agree. Thanks for the draft. I used it as-is. I don't come
> up with an appropriate second description of the GUC so I just removed
> it.
>
> # it was "For rotating magnetic disks, it is around the size of a
> # track or sylinder."
>
> > the relevant parameters.) I like the name "wal_skip_threshold", and
> > my second choice would be "wal_skip_min_size". Possibly documented
> > as follows:
> ..
> > Any other opinions on the GUC name?
>
> I prefer the first candidate. I already used the terminology in
> storage.c and the name fits more to the context.
>
> > * We emit newpage WAL records for smaller size of relations.
> > *
> > * Small WAL records have a chance to be emitted at once along with
> > * other backends' WAL records. We emit WAL records instead of syncing
> > * for files that are smaller than a certain threshold expecting faster
> - * commit. The threshold is defined by the GUC effective_io_block_size.
> + * commit. The threshold is defined by the GUC wal_skip_threshold.

> It's wrong that it also skips changing flags.
> I"ll fix it soon

This is the fixed verison v22.

The attached are:

- v22-0001-TAP-test-for-copy-truncation-optimization.patch
Same as v20, 21

- v22-0002-Fix-WAL-skipping-feature.patch
GUC name changed. Same as v21.

- v22-0003-Fix-MarkBufferDirtyHint.patch
PoC of fixing the function. will be merged into 0002. (New in v21,
fixed in v22)

- v21-0004-Documentation-for-wal_skip_threshold.patch
GUC name and description changed. (Previous 0003, same as v21)

- v21-0005-Additional-test-for-new-GUC-setting.patch
including adjusted version of wal-optimize-noah-tests-v3.patch
Maybe test names need further adjustment. (Previous 0004, same as v21)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center