Hi,
I've been going through the new AIO code as an effort to rebase and
adapt Neon to PG18. In doing so, I found the following
items/curiosities:
1. In aio/README.md, the following code snippet is found:
[...]
pgaio_io_set_handle_data_32(ioh, (uint32 *) buffer, 1);
[...]
I believe it would be clearer if it took a reference to the buffer:
pgaio_io_set_handle_data_32(ioh, (uint32 *) &buffer, 1);
The main reason here is that common practice is to have a `Buffer
buffer;` whereas a Buffer * is more commonly plural.
2. In aio.h, PgAioHandleCallbackID is checked to fit in
PGAIO_RESULT_ID_BITS (though the value of PGAIO_HCB_MAX).
However, the check is off by 1:
```
#define PGAIO_HCB_MAX PGAIO_HCB_LOCAL_BUFFER_READV
StaticAssertDecl(PGAIO_HCB_MAX <= (1 << PGAIO_RESULT_ID_BITS), [...])
```
This static assert will not trigger when PGAIO_HCB_MAX is equal to
2^PGAIO_RESULT_ID_BITS, but its value won't fit in those 6 bits and
instead overflow to 0.
To fix this, I suggest the `<=` is replaced with `<` to make that
work, or the definition of PGAIO_HCB_MAX to be updated to
PGAIO_HCB_LOCAL_BUFFER_READV + 1.
Note that this won't have caused bugs yet because we're not nearing
this limit of 64 callback IDs, but it's just a matter of good code
hygiene.
3. I noticed that there is AIO code for writev-related operations
(specifically, pgaio_io_start_writev is exposed, as is
PGAIO_OP_WRITEV), but no practical way to excercise that code: it's
not called from anywhere in the project, and there is no way for
extensions to register the relevant callbacks required to make writev
work well on buffered contents. Is that intentional?
Relatedly, the rationale for using enum PgAioHandleCallbackID rather
than function pointers in the documentation above its definition says
that function pointer instability across backends is one of the 2 main
reasons.
Is there any example of OS or linker behaviour that does not start
PostgreSQL with stable function pointer addresses across backends of
the same PostgreSQL binary? Or is this designed with extensibility
and/or cross-version EXEC_BACKEND in mind, but with extensibility not
yet been implemented due to $constraints?
I've attached a patch that solves the issues of (1) and (2).
Kind regards,
Matthias van de Meent
Databricks
