Issue with server side statement-level rollback
Hi,
We're facing some issue in a new extension we use at LzLabs to
emulate server side rollback at statement level in PostgreSQL,
see for full detail https://github.com/lzlabs/pg_statement_rollback/
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.
(gdb) b pquery.c:1327
Breakpoint 1 at 0x55792fd7a04d: file pquery.c, line 1327.
(gdb) c
Continuing.
Breakpoint 1, PortalRunMulti (portal=portal@entry=0x5579316e3e10,
isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x557931755ce8, altdest=altdest@entry=0x557931755ce8,
qc=qc@entry=0x7ffc4aa1f8a0) at pquery.c:1327
1327 Assert(portal->portalContext == CurrentMemoryContext);
(gdb) p portal->sourceText
$1 = 0x557931679c80 "INSERT INTO savepoint_test SELECT 1;"
(gdb) p MemoryContextStats(portal->portalContext)
$2 = void
(gdb)
The memory context dump output from log:
PortalContext: 1024 total in 1 blocks; 704 free (1 chunks); 320
used: <unnamed>
Grand total: 1024 bytes in 1 blocks; 704 free (1 chunks); 320 used
If I naively remove the assert on pquery.c everything works without any
new assert error.
As I said I am aware that this is clearly not a standard PostgreSQL use
but we have no choice. We are emulating DB2 statement-level rollback
behavior and we have chosen to not create a new fork of PostgreSQL and
only work with extensions. As there is no hook or API that could allow a
perfect
server side integration of this feature we have done what is possible to do
in the extension.
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.
Cheers
--
Gilles Darold
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.
As there is no hook or API that could allow a perfect server side
integration of this feature we have done what is possible to do in the
extension.
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.
Greetings,
Andres Freund
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]/messages/by-id/0A3221C70F24FB45833433255569204D1F6A9286@G01JPEXMBYT05 . 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]: /messages/by-id/0A3221C70F24FB45833433255569204D1F6A9286@G01JPEXMBYT05
/messages/by-id/0A3221C70F24FB45833433255569204D1F6A9286@G01JPEXMBYT05
--
Gilles Darold
http://www.darold.net/
Attachments:
hook_statement_level_rollback.patchtext/x-patch; charset=UTF-8; name=hook_statement_level_rollback.patchDownload
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
Le 20/11/2020 à 16:18, Gilles Darold a écrit :
I will work later on a POC to demonstrate the use case I want to
implement.
Hi Andres,
I have created a new version of the pg_statement_rollback extension [1]https://github.com/darold/pg_statement_rollbackv2
to demonstrate the use of the hooks on start_xact_command(),
finish_xact_command() and AbortCurrentTransaction() to implement the
statement-level rollback feature entirely driven at serverside. It
require that the patch [2]https://raw.githubusercontent.com/darold/pg_statement_rollbackv2/main/command-start-finish-hook-v1.patch I've provided be applied on PostgreSQL
source first.
Here is what can be achieved with this patch:
LOAD 'pg_statement_rollback.so';
LOAD
SET pg_statement_rollback.enabled TO on;
SET
CREATE SCHEMA testrsl;
CREATE SCHEMA
SET search_path TO testrsl,public;
SET
BEGIN;
BEGIN
CREATE TABLE tbl_rsl(id integer, val varchar(256));
CREATE TABLE
INSERT INTO tbl_rsl VALUES (1, 'one');
INSERT 0 1
WITH write AS (INSERT INTO tbl_rsl VALUES (2, 'two') RETURNING id,
val) SELECT * FROM write;
id | val
----+-----
2 | two
(1 row)
UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1; -- >>>>> will fail
psql:simple.sql:14: ERROR: invalid input syntax for type integer: "two"
LINE 1: UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1;
^
SELECT * FROM tbl_rsl; -- Should show records id 1 + 2
id | val
----+-----
1 | one
2 | two
(2 rows)
COMMIT;
COMMIT
Actually unlike I've though this is the hook on finish_xact_command()
that is useless. In the extension I'm executing the RELEASE/SAVEPOINT in
the start_xact_command() hook before executing the next statement. The
hook on AbortCurrentTransaction() is used to signal that a ROLLOBACK
TO/SAVEPOINT need to be executed into the start_xact_command() hook
instead of a RELEASE/SAVEPOINT.
This works perfectly and do not crash PG anymore when compiled with
assert. Advanced tests (with triggers, client savepoint, CTE, etc.) are
available in the test/sql/ directory. Use of "make installcheck" allow
to run the regression tests.
Based on this result I really think that these hooks should be included
to be able to extend PostgreSQL for such feature although I have not
though about an other use that this one.
Regards,
I've attached all code for archiving but the current version can be
found here too:
[1]: https://github.com/darold/pg_statement_rollbackv2
[2]: https://raw.githubusercontent.com/darold/pg_statement_rollbackv2/main/command-start-finish-hook-v1.patch
https://raw.githubusercontent.com/darold/pg_statement_rollbackv2/main/command-start-finish-hook-v1.patch
--
Gilles Darold
http://www.darold.net/
Attachments:
command-start-finish-hook-v1.patchtext/x-patch; charset=UTF-8; name=command-start-finish-hook-v1.patchDownload
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