On Wed, 2023-12-20 at 15:36 +0530, Bharath Rupireddy wrote: > Thanks. Attaching remaining patches as v18 patch-set after commits > c3a8e2a7cb16 and 766571be1659.
Comments: I still think the right thing for this patch is to call XLogReadFromBuffers() directly from the callers who need it, and not change WALRead(). I am open to changing this later, but for now that makes sense to me so that we can clearly identify which callers benefit and why. I have brought this up a few times before[1][2], so there must be some reason that I don't understand -- can you explain it? The XLogReadFromBuffersResult is never used. I can see how it might be useful for testing or asserts, but it's not used even in the test module. I don't think we should clutter the API with that kind of thing -- let's just return the nread. I also do not like the terminology "partial hit" to be used in this way. Perhaps "short read" or something about hitting the end of readable WAL would be better? I like how the callers of WALRead() are being more precise about the bytes they are requesting. You've added several spinlock acquisitions to the loop. Two explicitly, and one implicitly in WaitXLogInsertionsToFinish(). These may allow you to read slightly further, but introduce performance risk. Was this discussed? The callers are not checking for XLREADBUGS_UNINITIALIZED_WAL, so it seems like there's a risk of getting partially-written data? And it's not clear to me the check of the wal page headers is the right one anyway. It seems like all of this would be simpler if you checked first how far you can safely read data, and then just loop and read that far. I'm not sure that it's worth it to try to mix the validity checks with the reading of the data. Regards, Jeff Davis [1] https://www.postgresql.org/message-id/4132fe48f831ed6f73a9eb191af5fe475384969c.camel%40j-davis.com [2] https://www.postgresql.org/message-id/2ef04861c0f77e7ae78b703770cc2bbbac3d85e6.ca...@j-davis.com