Hi,
Currently the example uses the following order of calls:
StartTransactionCommand();
SPI_connect();
PushActiveSnapshot(...);
...
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
This could be somewhat misleading. Typically one expects something to be freed
in a reverse order compared to initialization. This creates a false impression
that PushActiveSnapshot(...) _should_ be called after SPI_connect().
The patch changes the order to:
StartTransactionCommand();
PushActiveSnapshot(...);
SPI_connect();
...
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
... and also clarifies that the order of PushActiveSnapshot(...) and
SPI_connect() is not important.
--
Best regards,
Aleksander Alekseev
From 9f20a508c3b52d94455d67e3bdf7872787c63255 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Fri, 12 May 2023 17:15:04 +0300
Subject: [PATCH v1] Slight improvement of worker_spi.c example
Previously the example used the following order of calls:
StartTransactionCommand();
SPI_connect();
PushActiveSnapshot(...);
...
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
This could be somewhat misleading. Typically one expects something to be freed
in a reverse order comparing to initialization. This creates a false impression
that PushActiveSnapshot(...) _should_ be called after SPI_connect().
The patch changes the order to:
StartTransactionCommand();
PushActiveSnapshot(...);
SPI_connect();
...
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
... and also clarifies that the order of PushActiveSnapshot(...) and
SPI_connect() is not important.
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
src/test/modules/worker_spi/worker_spi.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ad491d7722..8045bb3546 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -74,8 +74,8 @@ initialize_worker_spi(worktable *table)
SetCurrentStatementStartTimestamp();
StartTransactionCommand();
- SPI_connect();
PushActiveSnapshot(GetTransactionSnapshot());
+ SPI_connect();
pgstat_report_activity(STATE_RUNNING, "initializing worker_spi schema");
/* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
@@ -222,17 +222,19 @@ worker_spi_main(Datum main_arg)
* preceded by SetCurrentStatementStartTimestamp(), so that statement
* start time is always up to date.
*
- * The SPI_connect() call lets us run queries through the SPI manager,
- * and the PushActiveSnapshot() call creates an "active" snapshot
+ * The PushActiveSnapshot() call creates an "active" snapshot,
* which is necessary for queries to have MVCC data to work on.
+ * The SPI_connect() call lets us run queries through the SPI manager.
+ * The order of PushActiveSnapshot() and SPI_connect() is not really
+ * important.
*
* The pgstat_report_activity() call makes our activity visible
* through the pgstat views.
*/
SetCurrentStatementStartTimestamp();
StartTransactionCommand();
- SPI_connect();
PushActiveSnapshot(GetTransactionSnapshot());
+ SPI_connect();
debug_query_string = buf.data;
pgstat_report_activity(STATE_RUNNING, buf.data);
--
2.40.1
