On Sat, Jun 27, 2026 at 6:41 AM Andrey Borodin <[email protected]> wrote: > v3 set applies cleanly on REL_18_STABLE.
Oh, I didn't realize this was based on the back-branch code. Thanks. I have reviewed 0001 and 0002 and in general I think this looks good. There is possibly room for some concern about performance regressions with 0002 since, as you point out in some of the added comments, we will now keep the VM buffer locked for longer. It might be worth benchmarking that to see how bad it looks, but I don't know that we really have any choice. Correctness bugs are bad. One problem is that I think the newly-registered VM blocks will get fed to heap_mask() if wal_consistency_checking is turned on, but it's really only meant to work with heap pages. Other comments follow. 0001. I personally don't find this to be an improvement. Even it is, I feel like a back-patched commit is not a good place to invent a new convention for identifying block reference indexes. However, it's ultimately a matter of style, so if I lose this particular argument, c'est la vie. I just feel like if we introduce this for every WAL record, it's going to be a lot of new #defines that are going to be long and clunky, and if we don't, then we have a weird mix of styles. 0002. In heap_insert, the local variable called 'all_visible_cleared' seems like it needs a bit of work. You'd expect such a variable to be set at the point where we clear the all-visible bit, but it's actually set before that, when we lock the buffer. Later, you write in the comments "We locked vmbuffer if all_visible_cleared was true regardless of whether or not we ended up modifying it," which similarly implies that the variable is now misnamed. However, PageClearAllVisible() is called if and only if all_visible_cleared is true, so that comment doesn't seem to be entirely accurate. My first instinct is to rename the variable to clear_all_visible and delete the comment, but there might be something better. Looks like heap_delete has the same issue, and heap_update has a similar issue multiplied by two (all_visible_cleared vs. all_visible_cleared_new). In heap_update, the comment /* clear PD_ALL_VISIBLE flags, reset all visibilitymap bits */ has been lost, and the code has been reordered slightly in that area. This is fine, but keeping the existing comment and/or code ordering could be worth considering. In heap_lock_tuple, I think it might be a good idea to set unlock_vmbuffer = false when we release the lock, and then just after the out_unlocked: label, Assert(!unlock_vmbuffer). That way, if someone adds an improvident goto in a future patch, it has a chance of tripping an Assert. I think the comment for heap_xlog_vm_clear could be better: instead of saying that it "handles" something, say what it actually does. Likewise "use it for redo" inside the function is a little imprecise. If I'm understanding the purpose of this machinery correctly, the point here is that any FPIs included for the VM block references need to be applied, and otherwise (if there's no FPI or the block reference is not included at all) we need to just clear the relevant bit. That seems correct, but a little more work on the comments would make it clearer to future readers, I feel. In heap_xlog_multi_insert, you delete the comment "The visibility map may need to be fixed even if the heap page is already up-to-date," and replace it with a completely new comment that seems to be addresing someone different concerns, but the identical comments in other functions in the file are retained. Consider adding to the comment instead of replacing it. Also, I am not sure I understand the point of the new comment. Perhaps try to make it clearer. I find the phrasing of the comment addition at the top of visibilitymap_clear to be a little harder to understand than necessary. Maybe: "Most callers should use visibilitymap_clear_locked rather than this function. It is usually necessary to register the VM buffer in the WAL record, and this necessitates holding the lock for longer than it is held here. However, we retain this function for backward compatibility, and for a few callers that don't have this requirement." The redo routines ignore the return value of visibilitymap_clear_locked, which means they set the page LSN even if it isn't modified. Apparently there is other precedent for that treatment, but maybe it's best avoided anyway. I think that the commit messages could use some polishing. For 0001, if we keep it, I'd suggest that the subject line should use language like "introduce macros for" instead of "alias" because nobody's going to know what "alias" means without reading the commit itself. For 0002, I'd suggest that the subject line should be something like "Fix heap WAL records to register VM buffers when clearing VM bits". I think your goals here should be (1) to maximize the chance that somebody can understand what the commit does if they just read the one-line summary, or at most the whole commit message, and (2) to minimize the amount of work it will take to turn your commit message into a release note entry. I'm not saying your existing commit messages are bad, but they read a bit as if they were written to summarize work that the reader has already seen. -- Robert Haas EDB: http://www.enterprisedb.com
