Hi, On 2021-07-30 13:58:51 -0400, Tom Lane wrote: > Gilles Darold <gil...@darold.net> writes: > > [ 00001-startcommand_xact_callback-v2.diff ] > > I've not read this version of the patch, but I see from the cfbot's > results that it's broken postgres_fdw. I recall that postgres_fdw > uses the XactCallback and SubXactCallback mechanisms, so I'm betting > this means that you've changed the semantics of those callbacks in > an incompatible way. That's probably not a great idea. We could > fix postgres_fdw, but there are more than likely some external > modules that would also get broken, and that is supposed to be a > reasonably stable API.
I think this may partially be an issue with the way that postgres_fdw uses the callback than with the patch. It disconnects from the server *regardless* of the XactEvent passed to the callback. That makes it really hard to extend the callback mechanism to further events... Now, I'm also *quite* unconvinced that the placement of the new CallXactStartCommand() in postgres.c is right. On 2021-07-16 11:48:24 +0200, Gilles Darold wrote: > Other calls of start_xact_command() are in exec_bind_message(), > exec_execute_message(), exec_describe_statement_message(), > exec_describe_portal_message and PostgresMain. In my test they are either > not called or generates duplicates calls to the callback with > exec_simple_query() and c_parse_exemessage(). That seems like an issue with your test then. Prepared statements can be parsed in one transaction and bind+exec'ed in another. And you even can execute transaction control statements this way. IMO this'd need tests somewhere that allow us to verify the hook placements do something sensible. It does not seems not great to add a bunch of external function calls into all these routines. For simple queries postgres.c's exec_* functions show up in profiles - doing yet another function call that then also needs to look at various memory locations plausibly will show up. Particularly when used with pipelined queries. I'm *very* unconvinced it makes sense to implement a feature like this in an extension / that we should expose API for that purpose. To me the top-level transaction state is way too tied to our internals for it to be reasonably dealt with in an extension. And I think an in-core version would need to tackle the overhead and internal query execution issues this feature has. Greetings, Andres Freund
