Horton

Thread

Commits

  1. Add assertions for all the required index AM callbacks

  2. doc: Inform about aminsertcleanup optional NULLness

  3. Reuse BrinDesc and BrinRevmap in brininsert

  1. IndexAmRoutine aminsertcleanup function can be NULL?

    Peter Smith <smithpb2250@gmail.com> — 2025-07-16T00:08:07Z

    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

  2. Re: IndexAmRoutine aminsertcleanup function can be NULL?

    Japin Li <japinli@hotmail.com> — 2025-07-17T05:34:42Z

    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

  3. Re: IndexAmRoutine aminsertcleanup function can be NULL?

    Michael Paquier <michael@paquier.xyz> — 2025-07-22T05:36:37Z

    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

  4. Re: IndexAmRoutine aminsertcleanup function can be NULL?

    Peter Smith <smithpb2250@gmail.com> — 2025-07-22T07:36:29Z

    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

  5. Re: IndexAmRoutine aminsertcleanup function can be NULL?

    Japin Li <japinli@hotmail.com> — 2025-07-23T04:07:56Z

    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

  6. Re: IndexAmRoutine aminsertcleanup function can be NULL?

    Michael Paquier <michael@paquier.xyz> — 2025-07-24T04:44:54Z

    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

  7. Re: IndexAmRoutine aminsertcleanup function can be NULL?

    Michael Paquier <michael@paquier.xyz> — 2025-07-27T08:51:35Z

    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