On Thu, Jun 6, 2019 at 10:08 PM Michael Paquier <michael@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