On Mon, Jul 11, 2022 at 7:22 PM Andres Freund <and...@anarazel.de> wrote: > I guess I'm not enthused in duplicating the necessary knowledge in evermore > places. We've forgotten one of the magic incantations in the past, and needing > to find all the places that need to be patched is a bit bothersome. > > Perhaps we could add extract helpers out of durable_rename()? > > OTOH, I don't really see what we gain by keeping things out of the critical > section? It does seem good to have the temp-file creation/truncation and write > separately, but after that I don't think it's worth much to avoid a > PANIC. What legitimate issue does it avoid?
OK, so then I think we should just use durable_rename(). Here's a patch that does it that way. I briefly considered the idea of extracting helpers, but it doesn't seem worthwhile to me. There's not that much code in durable_rename() in the first place. In this version, I also removed the struct padding, changed the limit on the number of entries to a nice round 64, and made some comment updates. I considered trying to go further and actually make the file variable-size, so that we never again need to worry about the limit on the number of entries, but I don't actually think that's a good idea. It would require substantially more changes to the code in this file, and that means there's more risk of introducing bugs, and I don't see that there's much value anyway, because if we ever do hit the current limit, we can just raise the limit. If we were going to split up durable_rename(), the only intelligible split I can see would be to have a second version of the function, or a flag to the existing function, that caters to the situation where the old file is already known to have been fsync()'d. -- Robert Haas EDB: http://www.enterprisedb.com
v2-0001-Remove-the-restriction-that-the-relmap-must-be-51.patch
Description: Binary data
