On Wed, Mar 23, 2022 at 6:28 PM Peter Geoghegan <p...@bowt.ie> wrote: > It would be great if you could take a look v11-0002-*, Robert. Does it > make sense to you?
You're probably not going to love hearing this, but I think you're still explaining things here in ways that are too baroque and hard to follow. I do think it's probably better. But, for example, in the commit message for 0001, I think you could change the subject line to "Allow non-aggressive vacuums to advance relfrozenxid" and it would be clearer. And then I think you could eliminate about half of the first paragraph, starting with "There is no fixed relationship", and all of the third paragraph (which starts with "Later work..."), and I think removing all that material would make it strictly more clear than it is currently. I don't think it's the place of a commit message to speculate too much on future directions or to wax eloquent on theoretical points. If that belongs anywhere, it's in a mailing list discussion. It seems to me that 0002 mixes code movement with functional changes. I'm completely on board with moving the code that decides how much to skip into a function. That seems like a great idea, and probably overdue. But it is not easy for me to see what has changed functionally between the old and new code organization, and I bet it would be possible to split this into two patches, one of which creates a function, and the other of which fixes the problem, and I think that would be a useful service to future readers of the code. I have a hard time believing that if someone in the future bisects a problem back to this commit, they're going to have an easy time finding the behavior change in here. In fact I can't see it myself. I think the actual functional change is to fix what is described in the second paragraph of the commit message, but I haven't been able to figure out where the logic is actually changing to address that. Note that I would be happy with the behavior change happening either before or after the code reorganization. I also think that the commit message for 0002 is probably longer and more complex than is really helpful, and that the subject line is too vague, but since I don't yet understand exactly what's happening here, I cannot comment on how I think it should be revised at this point, except to say that the second paragraph of that commit message looks like the most useful part. I would also like to mention a few things that I do like about 0002. One is that it seems to collapse two different pieces of logic for page skipping into one. That seems good. As mentioned, it's especially good because that logic is abstracted into a function. Also, it looks like it is making a pretty localized change to one (1) aspect of what VACUUM does -- and I definitely prefer patches that change only one thing at a time. Hope that's helpful. -- Robert Haas EDB: http://www.enterprisedb.com
