pgsql-io / multicorn2

http://multicorn2.org
Other
73 stars 16 forks source link

Fix DELETE & UPDATE fetching `rowid_column` correctly in PG14+ #41

Closed mfenniak closed 2 months ago

mfenniak commented 2 months ago

In Postgres 14, the requirement for FDW Routines for Updating Foreign Tables were changed to require using add_row_identity_var (https://www.postgresql.org/docs/14/fdw-callbacks.html#FDW-CALLBACKS-UPDATE) to identify the row ID, rather than adding to parsetree->targetList (https://www.postgresql.org/docs/13/fdw-callbacks.html#FDW-CALLBACKS-UPDATE). This PR updates multicornAddForeignUpdateTargets to do the same.

Before this patch, when running a DELETE operation against multicorn2, I was receiving (?) as the identifying value. This came from rowidAttno being 0 (InvalidAttrNumber), causing ExecGetJunkAttribute to get a null Datum, causing datumToPtyhon to just return a "?" string. (https://github.com/mfenniak/dynamodb_fdw/issues/14#issuecomment-2094562737).

After this patch, the rowid_column is correctly added to the query results so that the delete operation can fetch it, and pass it on.

I've only tested these updates with DELETE and INSERT statements since that's all the FDW I'm working on supports, and only on PostgreSQL 15.6, but seems to work fine in that environment.

I suspect this would fix #15, but haven't tested that FDW. It does fix https://github.com/mfenniak/dynamodb_fdw/issues/14.

ShaheedHaque commented 2 months ago

This looks great, and I will try to test #15 against it. Quick question: I see the PG docs explicitly call out add_row_identity_var but I don't see that entry point in a PG header file. So of course, that results in a compilation warning:

src/multicorn.c: In function ‘multicornAddForeignUpdateTargets’:
src/multicorn.c:642:9: warning: implicit declaration of function ‘add_row_identity_var’ [-Wimplicit-function-declaration]
  642 |         add_row_identity_var(root, var, parsetree->resultRelation, strdup(attrname));
      |         ^~~~~~~~~~~~~~~~~~~~

Did I miss a header file, or maybe a fwd declaration should be added to suppress the warning?

mfenniak commented 2 months ago

@ShaheedHaque Thanks for pointing that out -- I didn't notice as my build environment was sweeping that output away. 55c0fb569aa135ca494fbc00428e038d4a6d662e should fix it; optimizer/appendinfo.h contains the fwd declaration.

ShaheedHaque commented 2 months ago

@mfenniak: I can confirm this resolves #15, and will merge on that basis. Thanks!