>> So what do you think of the patch?
>
> I would suggest to keep the original logic unless there is a bug.
IIUC, the premise of this path submission is that this codepath is in fact
buggy as it may lead to incorrect results for non-heap relations?
Since we have introduced the table AM api I support going throug it for all
table accesses, so +1 on the overall idea.
Some comments on the patch:
- * Note that this also behaves sanely if applied to an index or toast table;
+ * Note that this also behaves sanely if applied to a toast table;
* those won't have attached toast tables, but they can have multiple forks.
This comment reads a bit odd now and should probably be reworded.
- return size;
+ Assert(size < PG_INT64_MAX);
+
+ return (int64)size;
I assume that this change, and the other ones like that, aim to handle int64
overflow? Using the extra legroom of uint64 can still lead to an overflow,
however theoretical it may be. Wouldn't it be better to check for overflow
before adding to size rather than after the fact? Something along the lines of
checking for headroom left:
rel_size = table_relation_size(..);
if (rel_size > (PG_INT64_MAX - total_size))
< error codepath >
total_size += rel_size;
+ if (rel->rd_rel->relkind != RELKIND_INDEX)
+ {
+ relation_close(rel, AccessShareLock);
+ PG_RETURN_NULL();
+ }
pg_indexes_size is defined as returning the size of the indexes attached to the
specified relation, so this hunk is wrong as it instead requires rel to be an
index?
cheers ./daniel