Re: PATCH: Attempt to make dbsize a bit more consistent

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, September 10, 2020 12:51 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

[snip]

> Since we have introduced the table AM api I support going throug it for all
> table accesses, so +1 on the overall idea.
>

Thank you for reviewing! Please find v2 of the patch attached.

In addition to addressing the comments, this patch contains a slightly opinionated approach during describe. In short, only relations that have storage, are returning non null size during when \d*+ commands are emitted. Such an example is a view which can be found in the psql tests. If a view was returning a size of 0 Bytes, it would indicate that it can potentially be non zero, which is of course wrong.

> 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.
>

Agreed and amended.

>
> - 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;

Actually not, the intention is not to handle overflow. The table_relation_size() returns a uint64 and the calling function returns int64.

The Assert() has been placed in order to denote that it is acknowledged that the two functions return different types. I was of the opinion that a run time check will not be needed as even the smaller type can cover more than 9200 PetaByte tables.

If we were to change anything, then I would prefer to change the signature of the pg_*_size family of functions to return uint64 instead.

>
> - 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?

You are absolutely correct, amended.

>
> cheers ./daniel
>