On Thu, 8 Aug 2024 at 23:29, Tomas Vondra <to...@vondra.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, >
