Hi,
Le 19/11/2020 à 21:43, Andres Freund a écrit :
Hi,
On 2020-11-12 11:40:22 +0100, Gilles Darold wrote:
The problem we are encountering is when PostgreSQL is compiled in debug
mode with --enable-cassert. At line 1327 of src/backend/tcop/pquery.c
the following assert fail:
/*
* Clear subsidiary contexts to recover temporary memory.
*/
Assert(portal->portalContext == CurrentMemoryContext);
MemoryContextDeleteChildren(portal->portalContext);
This extension, although it is a risky implementation, works extremely
well when used in a fully controlled environment. It avoid the latency
of the extra communication for the RELEASE+SAVEPOINT usually controlled at
client side. The client is only responsible to issue the "ROLLBACK TO
autosavepoint"
when needed. The extension allow a high performances gain for this feature
that helps customers using Oracle or DB2 to migrate to PostgreSQL.
Actually with the extension the memory context is not CurrentMemoryContext
as expected by the assert.
What is it instead? I don't think you really can safely be in a
different context at this point. There's risks of CurrentMemoryContext
pointing to a deleted context, and risks of memory leaks, depending on
the situation.
This is a PortalContext. Yes this implementation has some risks but
until now I have not met any problem because its use and the environment
are fully controlled.
So my question is should we allow such use through an extension and in
this case what is the change to PostgreSQL code that could avoid the
assert crash? Or perhaps we have missed something in this extension to
be able to make the assert happy but I don't think so.
Without more detail of what you actually are precisely doing, and what
the hooks / integration you'd like would look like, it's hard to comment
usefully here.
We have implemented an extension to allow server side "statement-level
rollback" with what is possible to do now with PG but the objective was
to do the same thing that what was proposed as a core patch submitted by
Takayuki Tsunakawa [1] . This patch will not be included into core and
what I'm trying to do now is to have some facilities to allow this
feature through an extension that does not suffer from the same
limitation of pg_statement_rollback.
Looking that this patch for example, if we have a hook on
finish_xact_command(), finish_xact_command() and
AbortCurrentTransaction() I think we could probably be able to implement
the feature through an extension in a more "safe" way. A hook on
start_xact_command() seems useless as it looks it is executed before the
UtilityProcess and Executor* hooks. See attached patch for an example of
what could be useful for this kind of extension. Unfortunately my
knowledge doesn't allow me to see further and especially if there is
drawbacks. I hope this is more clear, I will work later on a POC to
demonstrate the use case I want to implement.
[1]
https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F6A9286%40G01JPEXMBYT05
--
Gilles Darold
http://www.darold.net/
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 03c553e7ea..81e1df27ef 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -314,6 +314,9 @@ typedef struct SubXactCallbackItem
static SubXactCallbackItem *SubXact_callbacks = NULL;
+/* Hook for plugins to get control of at end of AbortCurrentTransaction */
+AbortCurrentTransaction_hook_type abort_current_transaction_hook = NULL;
+
/* local function prototypes */
static void AssignTransactionId(TransactionState s);
static void AbortTransaction(void);
@@ -3358,6 +3361,9 @@ AbortCurrentTransaction(void)
AbortCurrentTransaction();
break;
}
+
+ if (abort_current_transaction_hook)
+ (*abort_current_transaction_hook) ();
}
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7c5f7c775b..3fff54ff51 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -192,6 +192,14 @@ static void log_disconnections(int code, Datum arg);
static void enable_statement_timeout(void);
static void disable_statement_timeout(void);
+/*
+ * Hook for plugins to get control at end/start of
+ * start_xact_command/finish_xact_command. The hook
+ * on start_xact_command might be useless as it happens
+ * before UtilityProccess and the Executor* hooks.
+ */
+XactCommandStart_hook_type start_xact_command_hook = NULL;
+XactCommandFinish_hook_type finish_xact_command_hook = NULL;
/* ----------------------------------------------------------------
* routines to obtain user input
@@ -2634,6 +2642,7 @@ exec_describe_portal_message(const char *portal_name)
static void
start_xact_command(void)
{
+
if (!xact_started)
{
StartTransactionCommand();
@@ -2649,11 +2658,19 @@ start_xact_command(void)
* not desired, the timeout has to be disabled explicitly.
*/
enable_statement_timeout();
+
+ if (start_xact_command_hook)
+ (*start_xact_command_hook) ();
+
}
+
static void
finish_xact_command(void)
{
+ if (finish_xact_command_hook)
+ (*finish_xact_command_hook) ();
+
/* cancel active statement timeout after each command */
disable_statement_timeout();
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 7320de345c..2e866b2a91 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -467,4 +467,8 @@ extern void EnterParallelMode(void);
extern void ExitParallelMode(void);
extern bool IsInParallelMode(void);
+/* Hook for plugins to get control in start_xact_command() and finish_xact_command() */
+typedef void (*AbortCurrentTransaction_hook_type) (void);
+extern PGDLLIMPORT AbortCurrentTransaction_hook_type abort_current_transaction_hook;
+
#endif /* XACT_H */
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 437642cc72..a8bef2f639 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -42,4 +42,10 @@ extern uint64 PortalRunFetch(Portal portal,
long count,
DestReceiver *dest);
+/* Hook for plugins to get control in start_xact_command() and finish_xact_command() */
+typedef void (*XactCommandStart_hook_type) (void);
+extern PGDLLIMPORT XactCommandStart_hook_type start_xact_command_hook;
+typedef void (*XactCommandFinish_hook_type) (void);
+extern PGDLLIMPORT XactCommandFinish_hook_type finish_xact_command_hook;
+
#endif /* PQUERY_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index fde701bfd4..23ddca9891 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -13,6 +13,7 @@ A_Expr_Kind
A_Indices
A_Indirection
A_Star
+AbortCurrentTransaction_hook_type
AbsoluteTime
AccessMethodInfo
AccessPriv
@@ -2794,6 +2795,8 @@ XPVIV
XPVMG
XactCallback
XactCallbackItem
+XactCommandStart_hook_type
+XactCommandFinish_hook_type
XactEvent
XactLockTableWaitInfo
XidBoundsViolation
