Re: PATCH: Add hooks for pg_total_relation_size and pg_indexes_size - Mailing list pgsql-hackers

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

Hi all,

Here's my follow-up based on the feedback received:

1. Compilation Issue:
   I didn’t encounter any errors when compiling on my machine, but I’ll review the environment and ensure fmgr.h is included where necessary to avoid the issue.

2. Prefix "Pg_":
   I’ll remove the "Pg_" prefix as I see now that it’s unnecessary.

3. Return Value Change:
   The +1 in the return value was part of a test that I forgot to remove. I’ll clean that up in the next version of the patch.

4. Tabs and Spaces:
   I’ll correct the indentation to use tabs consistently, as per the project’s coding standards.

5. Motivation:
   The hooks are intended to add support for calculating the Tantivy index size, in line with the need outlined in this issue. This will allow us to integrate additional index sizes into PostgreSQL’s built-in size functions.

6. Additional Hooks:
   I’ll add hooks for pg_relation_size() and pg_table_size() for consistency.

I’ll make these changes and resubmit the patch soon. Thanks again for your guidance!

Best regards,