Обсуждение: IndexAmRoutine aminsertcleanup function can be NULL?

Hi hackers.

I happened to notice that many contrib module indexes do not implement
an aminsertcleanup function:
e.g. amroutine->aminsertcleanup = NULL;

But, unlike many of the other interface functions, the aminsertcleanup
does not say "/* can be NULL */" in the typedef struct IndexAmRoutine
[1].

What's going on there? Is it just an accidentally missing "/* can be
NULL */" comment?

Same also in the documentation... [2]

======
[1] https://github.com/postgres/postgres/blob/master/src/include/access/amapi.h
[2] https://www.postgresql.org/docs/devel/index-api.html

Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 16 Jul 2025 at 10:08, Peter Smith <smithpb2250@gmail.com> wrote:
> Hi hackers.
>
> I happened to notice that many contrib module indexes do not implement
> an aminsertcleanup function:
> e.g. amroutine->aminsertcleanup = NULL;
>
> But, unlike many of the other interface functions, the aminsertcleanup
> does not say "/* can be NULL */" in the typedef struct IndexAmRoutine
> [1].
>
> What's going on there? Is it just an accidentally missing "/* can be
> NULL */" comment?
>

It appears commit c1ec02be1d79 is missing the comment.

> Same also in the documentation... [2]
>
> ======
> [1] https://github.com/postgres/postgres/blob/master/src/include/access/amapi.h
> [2] https://www.postgresql.org/docs/devel/index-api.html
>

How about asserting the existence of all required callbacks, similar to
GetTableAmRoutine()?

-- 
Regards,
Japin Li

On Thu, Jul 17, 2025 at 01:34:42PM +0800, Japin Li wrote:
> On Wed, 16 Jul 2025 at 10:08, Peter Smith <smithpb2250@gmail.com> wrote:
>> What's going on there? Is it just an accidentally missing "/* can be
>> NULL */" comment?
>
> It appears commit c1ec02be1d79 is missing the comment.

Agreed.  That's user-visible, so backpatched down to v17.

> How about asserting the existence of all required callbacks, similar to
> GetTableAmRoutine()?

Hmm, we could do something like that to enforce a stronger policy,
yes.  At least that seems sensible to me to be able to detect faster a
problem rather than waiting for the backend to report the issue with a
crash when testing a dedicated code path where one of these callbacks
is run.  As a HEAD-only thing, of course.

I was reading the page, and some of the functions in the docs are
documented as optional with the matching NULL description.  It is
the case of aminsertcleanup as well, one paragraph above with the
part about "may define".
--
Michael

On Tue, Jul 22, 2025 at 3:36 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 17, 2025 at 01:34:42PM +0800, Japin Li wrote:
> > On Wed, 16 Jul 2025 at 10:08, Peter Smith <smithpb2250@gmail.com> wrote:
> >> What's going on there? Is it just an accidentally missing "/* can be
> >> NULL */" comment?
> >
> > It appears commit c1ec02be1d79 is missing the comment.
>
> Agreed.  That's user-visible, so backpatched down to v17.
>

Thanks to both!

======
Kind Regards,
Peter Smith.
FUjitsu Australia

On Tue, 22 Jul 2025 at 14:36, Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Jul 17, 2025 at 01:34:42PM +0800, Japin Li wrote:
>> On Wed, 16 Jul 2025 at 10:08, Peter Smith <smithpb2250@gmail.com> wrote:
>>> What's going on there? Is it just an accidentally missing "/* can be
>>> NULL */" comment?
>> 
>> It appears commit c1ec02be1d79 is missing the comment.
>
> Agreed.  That's user-visible, so backpatched down to v17.
>
>> How about asserting the existence of all required callbacks, similar to
>> GetTableAmRoutine()?
>
> Hmm, we could do something like that to enforce a stronger policy,
> yes.  At least that seems sensible to me to be able to detect faster a
> problem rather than waiting for the backend to report the issue with a
> crash when testing a dedicated code path where one of these callbacks
> is run.  As a HEAD-only thing, of course.
>
> I was reading the page, and some of the functions in the docs are
> documented as optional with the matching NULL description.  It is 
> the case of aminsertcleanup as well, one paragraph above with the
> part about "may define".

PFA to assert all required IndexAM callbacks are present.

-- 
Regards,
Japin Li

On Wed, Jul 23, 2025 at 12:07:56PM +0800, Japin Li wrote:
> PFA to assert all required IndexAM callbacks are present.

@@ -42,6 +42,19 @@ GetIndexAmRoutine(Oid amhandler)
         elog(ERROR, "index access method handler function %u did not return an IndexAmRoutine struct",
              amhandler);

+    /* Assert that all required callbacks are present. */
+    Assert(routine->ambuild != NULL);
+    Assert(routine->ambuildempty != NULL);
+    Assert(routine->aminsert != NULL);
+    Assert(routine->ambulkdelete != NULL);
+    Assert(routine->amvacuumcleanup != NULL);
+    Assert(routine->amcostestimate != NULL);
+    Assert(routine->amoptions != NULL);
+    Assert(routine->amvalidate != NULL);
+    Assert(routine->ambeginscan != NULL);
+    Assert(routine->amrescan != NULL);
+    Assert(routine->amendscan != NULL);

Oh.  I like that, and I would like to apply it on HEAD if there are no
objections.
--
Michael

On Thu, Jul 24, 2025 at 01:44:54PM +0900, Michael Paquier wrote:
> Oh.  I like that, and I would like to apply it on HEAD if there are no
> objections.

Done as of 6f22a82a401d.
--
Michael