On Thu, Feb 17, 2022 at 05:40:41PM +0800, Julien Rouhaud wrote: > About the patch, it's incorrectly using a hardcoded 8192 block-size rather > than > using the computed :block_size (or computing one when it's not already the > case).
Well, the tests of pageinspect fail would already fail when using a different page size than 8k, like the checksum ones :) Anywa, I agree with your point that if this is not a reason to not make the tests more portable if we can do easily. > I'm also wondering if it wouldn't be better to return NULL rather than > throwing > an error for uninitialized pages. Agreed that this is a sensible choice. NULL would be helpful for the case where one compiles all the checksums of a relation with a full scan based on the relation size, for example. All these behaviors ought to be documented properly, as well. For SRFs, this should just return no rows rather than generating an error. > While at it, it could also be worthwhile to add tests for invalid blkno and > block size in page_checksum(). If we are on that, we could also have tests for the various "input page too small" errors. Now, these are just improvements, so I'd rather treat this part separately of the issue reported, and add the extra tests only on HEAD. I can't help but notice that we should have a similar protection on PageIsNew() in heap_page_items()? Now the code happens to work for an empty page thanks to PageGetMaxOffsetNumber(), but this extra protection would make the code more solid in the long-term IMO for the full-zero case? It is worth noting that, in pageinspect, two of the heap functions are the only ones to not be strict.. Something similar could be said about page_header() in rawpage.c, though it is not wrong to return only zeros for a page full of zeros. Shouldn't we similarly care about bt_metap() and bt_page_stats_internal(), both callers of ReadBuffer(). The root point is that the RBM_NORMAL mode of ReadBufferExtended() considers a page full of zeros as valid, as per its top comments. For the same reason, get_raw_page_internal() would work just fine and return a page full of zeros. Also, instead of an error in get_page_from_raw(), we should make sure that all its callers are able to map with the case of a new page, where we return 8kB worth of zeros from this inner routine. -- Michael
signature.asc
Description: PGP signature
