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

Hey Georgios,

Thanks for looking for more avenues to invoke tableAM APIS! Please find
my review below:

On Tue, Oct 13, 2020 at 6:28 AM <gkokolatos(at)pm(dot)me> wrote:

1.

> /*
> - * heap size, including FSM and VM
> + * table size, including FSM and VM
> */

We should not mention FSM and VM in dbsize.c at all as these are
heap AM specific. We can say:
table size, excluding TOAST relation

2.

> /*
> * Size of toast relation
> */
> if (OidIsValid(rel->rd_rel->reltoastrelid))
> - size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);
> + {
> + Relation toastRel;
> +
> + toastRel = relation_open(rel->rd_rel->reltoastrelid, AccessShareLock);

We can replace the OidIsValid check with relation_needs_toast_table()
and then have the OidIsValid() check in an Assert. Perhaps, that will
make things more readable.

3.

> + if (rel->rd_rel->relkind == RELKIND_RELATION ||
> + rel->rd_rel->relkind == RELKIND_TOASTVALUE ||
> + rel->rd_rel->relkind == RELKIND_MATVIEW)
> + size = calculate_table_size(rel);
> + else
> + {
> + relation_close(rel, AccessShareLock);
> + PG_RETURN_NULL();
> + }

This leads to behavioral changes:

I am talking about the case where one calls pg_table_size() on an index.
W/o your patch, it returns the size of the index. W/ your patch it
returns NULL. Judging by the documentation, this function should not
ideally apply to indexes but it does! I have a sinking feeling that lots
of users use it for this purpose, as there is no function to calculate
the size of a single specific index (except pg_relation_size()).
The same argument I have made above applies to sequences. Users may have
trial-and-errored their way into finding that pg_table_size() can tell
them the size of a specific index/sequence! I don't know how widespread
the use is in the user community, so IMO maybe we should be conservative
and not introduce this change? Alternatively, we could call out that
pg_table_size() is only for tables by throwing an error if anything
other than a table is passed in.

If we decide to preserve the existing behavior of the pg_table_size():
It seems that for things not backed by the tableAM (indexes and
sequences), they should still go through calculate_relation_size().
We can call table_relation_size() based on if relkind is
RELKIND_RELATION, RELKIND_TOASTVALUE or RELKIND_MATVIEW. Perhaps it
might be worthwhile making a new macro RELKIND_HAS_TABLE_STORAGE to
capture these three cases (See RELKIND_HAS_STORAGE). This also ensures
that we return 0 for things that don't qualify as RELKIND_HAS_STORAGE,
such as a partitioned table (Currently w/ the patch applied, we return
NULL for those cases, which is another behavior change)

4.

> @@ -3776,10 +3776,24 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
> gettext_noop("Access Method"));
>
> /*
> + * As of PostgreSQL 14, do not use pg_table_size() for indexes and
> + * sequences as it does not behave sanely for those.
> + *
> * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
> * size of a table, including FSM, VM and TOAST tables.
> */
> - if (pset.sversion >= 90000)
> + if (pset.sversion >= 140000)
> + appendPQExpBuffer(&buf,
> + ",\n CASE"
> + " WHEN c.relkind in ("CppAsString2(RELKIND_INDEX)", "
> + CppAsString2(RELKIND_PARTITIONED_INDEX)", "
> + CppAsString2(RELKIND_SEQUENCE)") THEN"
> + " pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid))"
> + " ELSE"
> + " pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid))"
> + " END as \"%s\"",
> + gettext_noop("Size"));
> + else if (pset.sversion >= 90000)
> appendPQExpBuffer(&buf,
> ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
> gettext_noop("Size"));

Following on from point 3, if we decide to preserve the existing behavior,
we wouldn't need this change, as it would be internalized within
pg_table_size().

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

Changing the signature would be the ideal change for all of this here.
But Postgres does not have support for an unsigned 64bit integer (bigint
is signed). One would need to turn to extensions such as [1]. Thus, +1
to what Daniel said above.

5.
> @@ -415,7 +384,7 @@ calculate_table_size(Relation rel)
> static int64
> calculate_indexes_size(Relation rel)
> {
> - int64 size = 0;
> + uint64 size = 0;
>
> /*
> * Aggregate all indexes on the given relation
> @@ -444,7 +413,9 @@ calculate_indexes_size(Relation rel)
> list_free(index_oids);
> }
>
> - return size;
> + Assert(size < PG_INT64_MAX);
> +
> + return (int64)size;
> }

I don't think we would need these changes as nothing changed in this
function.

Regards,
Soumyadeep

[1] https://github.com/petere/pguint