Re: PATCH: Add hooks for pg_total_relation_size and pg_indexes_size

On 8/8/24 14:18, Abdoulaye Ba wrote:
> Hello PostgreSQL Hackers,
>
> I am submitting a patch to add hooks for the functions
> pg_total_relation_size and pg_indexes_size. These hooks allow for custom
> behaviour to be injected into these functions, which can be useful for
> extensions and other custom PostgreSQL modifications.
>
> Patch details: 
>
> * Adds pg_total_relation_size_hook and pg_indexes_size_hook 
> * Modifies pg_total_relation_size and pg_indexes_size to call these
> hooks if they are set 
> * Adds necessary type definitions and extern declarations
>
> This feature is useful because it allows for more flexible and
> customizable behaviour in relation size calculations, which can be
> particularly valuable for extensions that need to account for additional
> storage outside of the standard PostgreSQL mechanisms.
>
> The patch is attached. 
>
> Thank you for considering this patch. I look forward to your feedback.
>

Hi,

Thanks for the patch. A couple comments:

1) Unfortunately, it doesn't compile - it fails with errors like this:

In file included from ../../../../src/include/access/tableam.h:25,
from detoast.c:18:
../../../../src/include/utils/rel.h:711:51: error: unknown type name
‘FunctionCallInfo’
711 | typedef Datum
(*Pg_total_relation_size_hook_type)(FunctionCallInfo fcinfo);

which happens because rel.h references FunctionCallInfo without
including fmgr.h. I wonder how you tested the patch ...

2) Also, I'm not sure why you have the "Pg_" at the beginning.

3) I'm not sure why the patch first changes the return to add +1 and
then undoes that:

PG_RETURN_INT64(size + 1);

That seems quite unnecessary. I wonder how you created the patch, seems
you just concatenated multiple patches.

4) The patch has a mix of tabs and spaces. We don't do that.

5) It would be really helpful if you could explain the motivation for
this. Not just "make this customizable" but what exactly you want to do
in the hooks and why (presumably you have an extension).

6) Isn't it a bit strange the patch modifies pg_total_relation_size()
but not pg_relation_size() or pg_table_size()? Am I missing something?

7) You should add the patch to the next commitfest (2024-09) at

https://commitfest.postgresql.org

regards

--
Tomas Vondra