Le 14/07/2021 à 21:26, Tom Lane a écrit :
Gilles Darold <gil...@darold.net> writes:
I have renamed the patch and the title of this proposal registered in
the commitfest "Xact/SubXact event callback at command start" to reflect
the last changes that do not include new hooks anymore.
Hmm, it doesn't seem like this has addressed my concern at all.
The callbacks are still effectively defined as "run during
start_xact_command", so they're not any less squishy semantically
than they were before. The point of my criticism was that you
should move the call site to someplace that's more organically
connected to execution of commands.
Another thing I'm not too pleased with in this formulation is that it's
very unclear what the distinction is between XACT_EVENT_COMMAND_START
and SUBXACT_EVENT_COMMAND_START. AFAICS, *every* potential use-case
for this would have to hook into both callback chains, and most likely
would treat the two events alike.
Please find in attachment the new version v2 of the patch, I hope this
time I have well understood your advices. Myapologies for this waste of
time.
I have moved the call to the callback out of start_xact_command() and
limit his call into exec_simple_query() and c_parse_exemessage(). There
is other call to start_xact_command() elsewhere but actually these two
places areenough for what I'm doing with the extensions. I have updated
the extension test cases to check the behavior when autocommit is on or
off, error in execute of prepared statement and error in update where
current of cursor. But there is certainly a case that I have missed.
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().
Also CallXactStartCommand() will only use one event
XACT_EVENT_COMMAND_START and only do a single call:
CallXactCallbacks(XACT_EVENT_COMMAND_START);
Plus, as you note, the goalposts have suddenly been moved for the
amount of overhead it's okay to have in an XactCallback or SubXactCallback
function. So that might cause problems for unrelated code. It's probably
better to not try to re-use that infrastructure.
About this maybe I was not clear in my bench, the overhead is not
introduced by the patch on the callback, there is no overhead. But by
the rollback at statement level extension. In case this was clear but
you think that we must not reuse this callback infrastructure do you
mean that I should fallback to a hook?
Best regard,
--
Gilles Darold
http://www.darold.net/
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 441445927e..7384345a48 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -6118,3 +6118,27 @@ MarkSubTransactionAssigned(void)
CurrentTransactionState->assigned = true;
}
+
+/*
+ * CallXactStartCommand
+ *
+ * Wrapper over CallXactCallbacks called in postgres.c after the call to
+ * start_xact_command(). It allows to user-defined code to be executed
+ * for the start of any command through a xact registered callback. This
+ * function do nothing if a transaction is not started.
+ *
+ * The related XactEvent is XACT_EVENT_COMMAND_START.
+ */
+void
+CallXactStartCommand(void)
+{
+ TransactionState s = CurrentTransactionState;
+
+ if (s->blockState == TBLOCK_DEFAULT || s->blockState == TBLOCK_STARTED)
+ return;
+
+ /*
+ * Call start-of-xact callbacks with start command event
+ */
+ CallXactCallbacks(XACT_EVENT_COMMAND_START);
+}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8cea10c901..97a1a19d10 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -989,6 +989,13 @@ exec_simple_query(const char *query_string)
*/
start_xact_command();
+ /*
+ * Instruct registered callbacks on xact that a new command is to be
+ * executed. It allows to execute user-defined code before any new
+ * statement is executed.
+ */
+ CallXactStartCommand();
+
/*
* Zap any pre-existing unnamed statement. (While not strictly necessary,
* it seems best to define simple-Query mode as if it used the unnamed
@@ -1082,6 +1089,13 @@ exec_simple_query(const char *query_string)
/* Make sure we are in a transaction command */
start_xact_command();
+ /*
+ * Instruct registered callbacks on xact that a new command is to be
+ * executed. It allows to execute user-defined code before any new
+ * statement is executed.
+ */
+ CallXactStartCommand();
+
/*
* If using an implicit transaction block, and we're not already in a
* transaction block, start an implicit block to force this statement
@@ -1361,6 +1375,13 @@ exec_parse_message(const char *query_string, /* string to execute */
*/
start_xact_command();
+ /*
+ * Instruct registered callbacks on xact that a new command is to be
+ * executed. It allows to execute user-defined code before any new
+ * statement is executed.
+ */
+ CallXactStartCommand();
+
/*
* Switch to appropriate context for constructing parsetrees.
*
@@ -2708,6 +2729,7 @@ start_xact_command(void)
!get_timeout_active(CLIENT_CONNECTION_CHECK_TIMEOUT))
enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
client_connection_check_interval);
+
}
static void
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 134f6862da..994e00e3b0 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -119,7 +119,8 @@ typedef enum
XACT_EVENT_PREPARE,
XACT_EVENT_PRE_COMMIT,
XACT_EVENT_PARALLEL_PRE_COMMIT,
- XACT_EVENT_PRE_PREPARE
+ XACT_EVENT_PRE_PREPARE,
+ XACT_EVENT_COMMAND_START
} XactEvent;
typedef void (*XactCallback) (XactEvent event, void *arg);
@@ -467,4 +468,6 @@ extern void EnterParallelMode(void);
extern void ExitParallelMode(void);
extern bool IsInParallelMode(void);
+extern void CallXactStartCommand(void);
+
#endif /* XACT_H */
