On Thu, Jun 6, 2019 at 10:08 PM Michael Paquier <mich...@paquier.xyz> wrote: > Looks like a neat split.
Thanks. > "allvisfrac", "pages" and "tuples" had better be documented about > which result they represent. A lot of the table AM stuff (and the related slot stuff) lacks function header comments; I don't like that and think it should be improved. However, that's not the job of this patch. I think it's completely correct for this patch to document, as it does, that the arguments have the same meaning as for the estimate_rel_size method, and leave it at that. There is certainly negative value in duplicating the definitions in multiple places, where they might get out of sync. The header comment for table_relation_estimate_size() refers the reader to the comments for estimate_rel_size(), which says: * estimate_rel_size - estimate # pages and # tuples in a table or index * * We also estimate the fraction of the pages that are marked all-visible in * the visibility map, for use in estimation of index-only scans. * * If attr_widths isn't NULL, it points to the zero-index entry of the * relation's attr_widths[] cache; we fill this in if we have need to compute * the attribute widths for estimation purposes. ...which AFAICT constitutes as much documentation of these parameters as we have got. I think this is all a bit byzantine and could probably be made clearer, but (1) fortunately it's not all that hard to guess what these are supposed to mean and (2) I don't feel obliged to do semi-related comment cleanup every time I patch tableam. > Could you explain what's the use cases you have in mind for > usable_bytes_per_page? All AMs using smgr need to have a page header, > which implies that the usable number of bytes is (BLCKSZ - page > header) like heap for tuple data. In the AMs you got to work with, do > you store some extra data in the page which is not used for tuple > storage? I think that makes sense, just wondering about the use > case. Exactly. BLCKSZ - page header is probably the maximum unless you roll your own page format, but you could easily have less if you use the special space. zheap is storing transaction slots there; you could store an epoch to avoid freezing, and there's probably quite a few other reasonable possibilities. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
