Re: PATCH: Add hooks for pg_total_relation_size and pg_indexes_size

On Thu, 8 Aug 2024 at 23:29, Tomas Vondra <tomas(at)vondra(dot)me> wrote:

> 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
> <https://github.com/paradedb/paradedb/issues/1061>. 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,
>