Обсуждение: Vectored I/O in bulk_write.c
Hi, I was trying to learn enough about the new bulk_write.c to figure out what might be going wrong over at [1], and finished up doing this exercise, which is experiment quality but passes basic tests. It's a bit like v1-0013 and v1-0014's experimental vectored checkpointing from [2] (which themselves are not currently proposed, that too was in the experiment category), but this usage is a lot simpler and might be worth considering. Presumably both things would eventually finish up being done by (not yet proposed) streaming write, but could also be done directly in this simple case. This way, CREATE INDEX generates 128kB pwritev() calls instead of 8kB pwrite() calls. (There's a magic 16 in there, we'd probably need to think harder about that.) It'd be better if bulk_write.c's memory management were improved: if buffers were mostly contiguous neighbours instead of being separately palloc'd objects, you'd probably often get 128kB pwrite() instead of pwritev(), which might be marginally more efficient. This made me wonder why smgrwritev() and smgrextendv() shouldn't be backed by the same implementation, since they are essentially the same operation. The differences are some assertions which might as well be moved up to the smgr.c level as they must surely apply to any future smgr implementation too, right?, and the segment file creation policy which can be controlled with a new argument. I tried that here. An alternative would be for md.c to have a workhorse function that both mdextendv() and mdwritev() call, but I'm not sure if there's much point in that. While thinking about that I realised that an existing write-or-extend assertion in master is wrong because it doesn't add nblocks. Hmm, it's a bit weird that we have nblocks as int or BlockNumber in various places, which I think should probably be fixed. [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGK%2B5DOmLaBp3Z7C4S-Yv6yoROvr1UncjH2S1ZbPT8D%2BZg%40mail.gmail.com [2] https://www.postgresql.org/message-id/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com
Вложения
Slightly better version, adjusting a few obsoleted comments, adjusting error message to distinguish write/extend, fixing a thinko in smgr_cached_nblocks maintenance.
Вложения
Here also is a first attempt at improving the memory allocation and memory layout. I wonder if bulk logging should trigger larger WAL writes in the "Have to write it ourselves" case in AdvanceXLInsertBuffer(), since writing 8kB of WAL at a time seems like an unnecessarily degraded level of performance, especially with wal_sync_method=open_datasync. Of course the real answer is "make sure wal_buffers is high enough for your workload" (usually indirectly by automatically scaling from shared_buffers), but this problem jumps out when tracing bulk_writes.c with default settings. We write out the index 128kB at a time, but the WAL 8kB at a time.
Вложения
One more observation while I'm thinking about bulk_write.c... hmm, it writes the data out and asks the checkpointer to fsync it, but doesn't call smgrwriteback(). I assume that means that on Linux the physical writeback sometimes won't happen until the checkpointer eventually calls fsync() sequentially, one segment file at a time. I see that it's difficult to decide how to do that though; unlike checkpoints, which have rate control/spreading, bulk writes could more easily flood the I/O subsystem in a burst. I expect most non-Linux systems' write-behind heuristics to fire up for bulk sequential writes, but on Linux where most users live, there is no write-behind heuristic AFAIK (on the most common file systems anyway), so you have to crank that handle if you want it to wake up and smell the coffee before it hits internal limits, but then you have to decide how fast to crank it. This problem will come into closer focus when we start talking about streaming writes. For the current non-streaming bulk_write.c coding, I don't have any particular idea of what to do about that, so I'm just noting the observation here. Sorry for the sudden wall of text/monologue; this is all a sort of reaction to bulk_write.c that I should perhaps have sent to the bulk_write.c thread, triggered by a couple of debugging sessions.
(Replying to all your messages in this thread together) > This made me wonder why smgrwritev() and smgrextendv() shouldn't be > backed by the same implementation, since they are essentially the same > operation. +1 to the general idea of merging the write and extend functions. > The differences are some assertions which might as well be > moved up to the smgr.c level as they must surely apply to any future > smgr implementation too, right?, and the segment file creation policy > which can be controlled with a new argument. I tried that here. Currently, smgrwrite/smgrextend just calls through to mdwrite/mdextend. I'd like to keep it that way. Otherwise we need to guess what a hypothetical smgr implementation might look like. For example this assertion: > /* This assert is too expensive to have on normally ... */ > #ifdef CHECK_WRITE_VS_EXTEND > Assert(blocknum >= mdnblocks(reln, forknum)); > #endif I think that should continue to be md.c's internal affair. For example, imagine that you had a syscall like write() but which always writes to the end of the file and also returns the offset that the data was written to. You would want to assert the returned offset instead of the above. So -1 on moving up the assertions to smgr.c. Let's bite the bullet and merge the smgrwrite and smgrextend functions at the smgr level too. I propose the following signature: #define SWF_SKIP_FSYNC 0x01 #define SWF_EXTEND 0x02 #define SWF_ZERO 0x04 void smgrwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const void **buffer, BlockNumber nblocks, int flags); This would replace smgwrite, smgrextend, and smgrzeroextend. The mdwritev() function would have the same signature. A single 'flags' arg looks better in the callers than booleans, because you don't need to remember what 'true' or 'false' means. The callers would look like this: /* like old smgrwrite(reln, MAIN_FORKNUM, 123, buf, false) */ smgrwritev(reln, MAIN_FORKNUM, 123, buf, 1, 0); /* like old smgrwrite(reln, MAIN_FORKNUM, 123, buf, true) */ smgrwritev(reln, MAIN_FORKNUM, 123, buf, 1, SWF_SKIP_FSYNC); /* like old smgrextend(reln, MAIN_FORKNUM, 123, buf, true) */ smgrwritev(reln, MAIN_FORKNUM, 123, buf, 1, SWF_EXTEND | SWF_SKIP_FSYNC); /* like old smgrzeroextend(reln, MAIN_FORKNUM, 123, 10, true) */ smgrwritev(reln, MAIN_FORKNUM, 123, NULL, 10, SWF_EXTEND | SWF_ZERO | SWF_SKIP_FSYNC); > While thinking about that I realised that an existing write-or-extend > assertion in master is wrong because it doesn't add nblocks. > > Hmm, it's a bit weird that we have nblocks as int or BlockNumber in > various places, which I think should probably be fixed. +1 > Here also is a first attempt at improving the memory allocation and > memory layout. > ... > +typedef union BufferSlot > +{ > + PGIOAlignedBlock buffer; > + dlist_node freelist_node; > +} BufferSlot; > + If you allocated the buffers in one large contiguous chunk, you could often do one large write() instead of a gathered writev() of multiple blocks. That should be even better, although I don't know much of a difference it makes. The above layout wastes a fair amount memory too, because 'buffer' is I/O aligned. > I wonder if bulk logging should trigger larger WAL writes in the "Have > to write it ourselves" case in AdvanceXLInsertBuffer(), since writing > 8kB of WAL at a time seems like an unnecessarily degraded level of > performance, especially with wal_sync_method=open_datasync. Of course > the real answer is "make sure wal_buffers is high enough for your > workload" (usually indirectly by automatically scaling from > shared_buffers), but this problem jumps out when tracing bulk_writes.c > with default settings. We write out the index 128kB at a time, but > the WAL 8kB at a time. Makes sense. On 12/03/2024 23:38, Thomas Munro wrote: > One more observation while I'm thinking about bulk_write.c... hmm, it > writes the data out and asks the checkpointer to fsync it, but doesn't > call smgrwriteback(). I assume that means that on Linux the physical > writeback sometimes won't happen until the checkpointer eventually > calls fsync() sequentially, one segment file at a time. I see that > it's difficult to decide how to do that though; unlike checkpoints, > which have rate control/spreading, bulk writes could more easily flood > the I/O subsystem in a burst. I expect most non-Linux systems' > write-behind heuristics to fire up for bulk sequential writes, but on > Linux where most users live, there is no write-behind heuristic AFAIK > (on the most common file systems anyway), so you have to crank that > handle if you want it to wake up and smell the coffee before it hits > internal limits, but then you have to decide how fast to crank it. > > This problem will come into closer focus when we start talking about > streaming writes. For the current non-streaming bulk_write.c coding, > I don't have any particular idea of what to do about that, so I'm just > noting the observation here. It would be straightforward to call smgrwriteback() from sgmr_bulk_flush every 1 GB of written data for example. It would be nice to do it in the background though, and not stall the bulk writing for it. With the AIO patches, I presume we could easily start an asynchronous writeback and not wait for it to finish. > Sorry for the sudden wall of text/monologue; this is all a sort of > reaction to bulk_write.c that I should perhaps have sent to the > bulk_write.c thread, triggered by a couple of debugging sessions. Thanks for looking! This all makes sense. The idea of introducing the bulk write interface was that now we have a natural place to put all these heuristics and optimizations in. That seems to be a success, AFAICS none of the things discussed here will change the bulk_write API, just the implementation. -- Heikki Linnakangas Neon (https://neon.tech)
On Wed, Mar 13, 2024 at 9:57 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Let's bite the bullet and merge the smgrwrite and smgrextend functions > at the smgr level too. I propose the following signature: > > #define SWF_SKIP_FSYNC 0x01 > #define SWF_EXTEND 0x02 > #define SWF_ZERO 0x04 > > void smgrwritev(SMgrRelation reln, ForkNumber forknum, > BlockNumber blocknum, > const void **buffer, BlockNumber nblocks, > int flags); > > This would replace smgwrite, smgrextend, and smgrzeroextend. The That sounds pretty good to me. > > Here also is a first attempt at improving the memory allocation and > > memory layout. > > ... > > +typedef union BufferSlot > > +{ > > + PGIOAlignedBlock buffer; > > + dlist_node freelist_node; > > +} BufferSlot; > > + > > If you allocated the buffers in one large contiguous chunk, you could > often do one large write() instead of a gathered writev() of multiple > blocks. That should be even better, although I don't know much of a > difference it makes. The above layout wastes a fair amount memory too, > because 'buffer' is I/O aligned. The patch I posted has an array of buffers with the properties you describe, so you get a pwrite() (no 'v') sometimes, and a pwritev() with a small iovcnt when it wraps around: pwrite(...) = 131072 (0x20000) pwritev(...,3,...) = 131072 (0x20000) pwrite(...) = 131072 (0x20000) pwritev(...,3,...) = 131072 (0x20000) pwrite(...) = 131072 (0x20000) Hmm, I expected pwrite() alternating with pwritev(iovcnt=2), the latter for when it wraps around the buffer array, so I'm not sure why it's 3. I guess the btree code isn't writing them strictly monotonically or something... I don't believe it wastes any memory on padding (except a few bytes wasted by palloc_aligned() before BulkWriteState): (gdb) p &bulkstate->buffer_slots[0] $4 = (BufferSlot *) 0x15c731cb4000 (gdb) p &bulkstate->buffer_slots[1] $5 = (BufferSlot *) 0x15c731cb6000 (gdb) p sizeof(bulkstate->buffer_slots[0]) $6 = 8192
On 13/03/2024 12:18, Thomas Munro wrote: > On Wed, Mar 13, 2024 at 9:57 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> Here also is a first attempt at improving the memory allocation and >>> memory layout. >>> ... >>> +typedef union BufferSlot >>> +{ >>> + PGIOAlignedBlock buffer; >>> + dlist_node freelist_node; >>> +} BufferSlot; >>> + >> >> If you allocated the buffers in one large contiguous chunk, you could >> often do one large write() instead of a gathered writev() of multiple >> blocks. That should be even better, although I don't know much of a >> difference it makes. The above layout wastes a fair amount memory too, >> because 'buffer' is I/O aligned. > > The patch I posted has an array of buffers with the properties you > describe, so you get a pwrite() (no 'v') sometimes, and a pwritev() > with a small iovcnt when it wraps around: Oh I missed that it was "union BufferSlot". I thought it was a struct. Never mind then. -- Heikki Linnakangas Neon (https://neon.tech)
Alright, here is a first attempt at merging all three interfaces as you suggested. I like it! I especially like the way it removes lots of duplication. I don't understand your argument about the location of the write-vs-extent assertions. It seems to me that these are assertions about what the *public* smgrnblocks() function returns. In other words, we assert that the caller is aware of the current relation size (and has some kind of interlocking scheme for that to be possible), according to the smgr implementation's public interface. That's not an assertion about internal details of the smgr implementation, it's part of the "contract" for the API.
Вложения
On 13/03/2024 23:12, Thomas Munro wrote: > Alright, here is a first attempt at merging all three interfaces as > you suggested. I like it! I especially like the way it removes lots > of duplication. > > I don't understand your argument about the location of the > write-vs-extent assertions. It seems to me that these are assertions > about what the *public* smgrnblocks() function returns. In other > words, we assert that the caller is aware of the current relation size > (and has some kind of interlocking scheme for that to be possible), > according to the smgr implementation's public interface. That's not > an assertion about internal details of the smgr implementation, it's > part of the "contract" for the API. I tried to say that smgr implementation might have better ways to assert that than calling smgrnblocks(), so it would be better to leave it to the implementation. But what bothered me most was that smgrwrite() had a different signature than mdwrite(). I'm happy with the way you have it in the v4 patch. -- Heikki Linnakangas Neon (https://neon.tech)
On Thu, Mar 14, 2024 at 10:49 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I tried to say that smgr implementation might have better ways to assert > that than calling smgrnblocks(), so it would be better to leave it to > the implementation. But what bothered me most was that smgrwrite() had a > different signature than mdwrite(). I'm happy with the way you have it > in the v4 patch. Looking for other things that can be hoisted up to smgr.c level because they are part of the contract or would surely need to be duplicated by any implementation: I think the check that you don't exceed the maximum possible block number could be there too, no? Here's a version like that. Does anyone else want to object to doing this for 17? Probably still needs some cleanup eg comments etc that may be lurking around the place and another round of testing. As for the overall idea, I'll leave it a few days and see if others have feedback. My take is that this is what bulk_write.c was clearly intended to do, it's just that smgr let it down by not allowing vectored extension yet. It's a fairly mechanical simplification, generalisation, and net code reduction to do so by merging, like this.
Вложения
I canvassed Andres off-list since smgrzeroextend() is his invention, and he wondered if it was a good idea to blur the distinction between the different zero-extension strategies like that. Good question. My take is that it's fine: mdzeroextend() already uses fallocate() only for nblocks > 8, but otherwise writes out zeroes, because that was seen to interact better with file system heuristics on common systems. That preserves current behaviour for callers of plain-old sgmrextend(), which becomes a wrapper for smgrwrite(..., 1, _ZERO | _EXTEND). If some hypothetical future caller wants to be able to call smgrwritev(..., NULL, 9 blocks, _ZERO | _EXTEND) directly without triggering the fallocate() strategy for some well researched reason, we could add a new flag to say so, ie adjust that gating. In other words, we have already blurred the semantics. To me, it seems nicer to have a single high level interface for the same logical operation, and flags to select strategies more explicitly if that is eventually necessary.
Hi, On 2024-03-16 12:27:15 +1300, Thomas Munro wrote: > I canvassed Andres off-list since smgrzeroextend() is his invention, > and he wondered if it was a good idea to blur the distinction between > the different zero-extension strategies like that. Good question. My > take is that it's fine: > > mdzeroextend() already uses fallocate() only for nblocks > 8, but > otherwise writes out zeroes, because that was seen to interact better > with file system heuristics on common systems. That preserves current > behaviour for callers of plain-old sgmrextend(), which becomes a > wrapper for smgrwrite(..., 1, _ZERO | _EXTEND). If some hypothetical > future caller wants to be able to call smgrwritev(..., NULL, 9 blocks, > _ZERO | _EXTEND) directly without triggering the fallocate() strategy > for some well researched reason, we could add a new flag to say so, ie > adjust that gating. > > In other words, we have already blurred the semantics. To me, it > seems nicer to have a single high level interface for the same logical > operation, and flags to select strategies more explicitly if that is > eventually necessary. I don't think zeroextend on the one hand and and on the other hand a normal write or extend are really the same operation. In the former case the content is hard-coded in the latter it's caller provided. Sure, we can deal with that by special-casing NULL content - but at that point, what's the benefit of combinding the two operations? I'm not dead-set against this, just not really convinced that it's a good idea to combine the operations. Greetings, Andres Freund
On Sun, Mar 17, 2024 at 8:10 AM Andres Freund <andres@anarazel.de> wrote: > I don't think zeroextend on the one hand and and on the other hand a normal > write or extend are really the same operation. In the former case the content > is hard-coded in the latter it's caller provided. Sure, we can deal with that > by special-casing NULL content - but at that point, what's the benefit of > combinding the two operations? I'm not dead-set against this, just not really > convinced that it's a good idea to combine the operations. I liked some things about that, but I'm happy to drop that part. Here's a version that leaves smgrzeroextend() alone, and I also gave up on moving errors and assertions up into the smgr layer for now to minimise the change. So to summarise, this gives us smgrwritev(..., flags), where flags can include _SKIP_FSYNC and _EXTEND. This way we don't have to invent smgrextendv(). The old single block smgrextend() still exists as a static inline wrapper.
Вложения
Then I would make the trivial change to respect the new io_combine_limit GUC that I'm gearing up to commit in another thread. As attached.
Вложения
Here's a rebase. I decided against committing this for v17 in the end. There's not much wrong with it AFAIK, except perhaps an unprincipled chopping up of writes with large io_combine_limit due to simplistic flow control, and I liked the idea of having a decent user of smgrwritev() in the tree, and it probably makes CREATE INDEX a bit faster, but... I'd like to try something more ambitious that streamifies this and also the "main" writeback paths. I shared some patches for that that are counterparts to this, over at[1]. [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGK1in4FiWtisXZ%2BJo-cNSbWjmBcPww3w3DBM%2BwhJTABXA%40mail.gmail.com
Вложения
On Tue, Apr 09, 2024 at 04:51:52PM +1200, Thomas Munro wrote: > Here's a rebase. I decided against committing this for v17 in the > end. There's not much wrong with it AFAIK, except perhaps an > unprincipled chopping up of writes with large io_combine_limit due to > simplistic flow control, and I liked the idea of having a decent user > of smgrwritev() in the tree, and it probably makes CREATE INDEX a bit > faster, but... I'd like to try something more ambitious that > streamifies this and also the "main" writeback paths. I see this in the CF as Needs Review since 2024-03-10, but this 2024-04-09 message sounds like you were abandoning it. Are you still commissioning a review of this thread's patches, or is the CF entry obsolete?