On 29/9/2023 09:52, Andrei Lepikhov wrote:
On 22/5/2023 22:59, reid.thomp...@crunchydata.com wrote:
Attach patches updated to master.
Pulled from patch 2 back to patch 1 a change that was also pertinent
to patch 1.
+1 to the idea, have doubts on the implementation.
I have a question. I see the feature triggers ERROR on the exceeding of
the memory limit. The superior PG_CATCH() section will handle the error.
As I see, many such sections use memory allocations. What if some
routine, like the CopyErrorData(), exceeds the limit, too? In this case,
we could repeat the error until the top PG_CATCH(). Is this correct
behaviour? Maybe to check in the exceeds_max_total_bkend_mem() for
recursion and allow error handlers to slightly exceed this hard limit?
By the patch in attachment I try to show which sort of problems I'm
worrying about. In some PП_CATCH() sections we do CopyErrorData
(allocate some memory) before aborting the transaction. So, the
allocation error can move us out of this section before aborting. We
await for soft ERROR message but will face more hard consequences.
--
regards,
Andrey Lepikhov
Postgres Professional
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 33975687b3..3f992b8d92 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -291,10 +291,7 @@ _SPI_commit(bool chain)
{
ErrorData *edata;
- /* Save error info in caller's context */
MemoryContextSwitchTo(oldcontext);
- edata = CopyErrorData();
- FlushErrorState();
/*
* Abort the failed transaction. If this fails too, we'll just
@@ -302,6 +299,10 @@ _SPI_commit(bool chain)
*/
AbortCurrentTransaction();
+ /* Save error info in caller's context */
+ edata = CopyErrorData();
+ FlushErrorState();
+
/* ... and start a new one */
StartTransactionCommand();
if (chain)
@@ -383,10 +384,7 @@ _SPI_rollback(bool chain)
{
ErrorData *edata;
- /* Save error info in caller's context */
MemoryContextSwitchTo(oldcontext);
- edata = CopyErrorData();
- FlushErrorState();
/*
* Try again to abort the failed transaction. If this fails
too,
@@ -395,6 +393,10 @@ _SPI_rollback(bool chain)
*/
AbortCurrentTransaction();
+ /* Save error info in caller's context */
+ edata = CopyErrorData();
+ FlushErrorState();
+
/* ... and start a new one */
StartTransactionCommand();
if (chain)
diff --git a/src/backend/replication/logical/reorderbuffer.c
b/src/backend/replication/logical/reorderbuffer.c
index 12edc5772a..f9cf599026 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2565,7 +2565,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
PG_CATCH();
{
MemoryContext ecxt = MemoryContextSwitchTo(ccxt);
- ErrorData *errdata = CopyErrorData();
+ ErrorData *errdata;
/* TODO: Encapsulate cleanup from the PG_TRY and PG_CATCH
blocks */
if (iterstate)
@@ -2579,6 +2579,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
*/
AbortCurrentTransaction();
+ errdata = CopyErrorData();
+
/* make sure there's no cache pollution */
ReorderBufferExecuteInvalidations(txn->ninvalidations,
txn->invalidations);
