Re: [PATCH] Hooks at XactCommand level

Hi,

On 2021-07-30 13:58:51 -0400, Tom Lane wrote:
> Gilles Darold <gilles(at)darold(dot)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