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
v1-0001-AIO-README-clarification-assertion-fix.patch
Description: Binary data
