On 20/12/2023 07:04, Michael Paquier wrote:
On Tue, Dec 19, 2023 at 10:14:44AM -0600, Nathan Bossart wrote:
On Tue, Dec 19, 2023 at 10:49:23AM -0500, Robert Haas wrote:
On Mon, Dec 18, 2023 at 3:32 AM Andrei Lepikhov
<a.lepik...@postgrespro.ru> wrote:
2. I think a separate file for this feature looks too expensive.
According to the gist of that code, it is a part of the DSA module.
-1. I think this is a totally different thing than DSA. More files
aren't nearly as expensive as the confusion that comes from smushing
unrelated things together.
Agreed. I think there's a decent chance that more functionality will be
added to this registry down the line, in which case it will be even more
important that this stuff stays separate from the tools it is built with.
+1 for keeping a clean separation between both.
Thanks, I got the reason.
In that case, maybe change the test case to make it closer to real-life
usage - with locks and concurrent access (See attachment)?
--
regards,
Andrei Lepikhov
Postgres Professional
diff --git a/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
b/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
index 4ad6097d1c..762489f3dd 100644
--- a/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
+++ b/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
@@ -13,12 +13,27 @@ $node->init;
$node->start;
$node->safe_psql('postgres', "CREATE DATABASE test;");
-$node->safe_psql('postgres', "CREATE EXTENSION test_dsm_registry;");
-$node->safe_psql('postgres', "SELECT set_val_in_shmem(1236);");
-
$node->safe_psql('test', "CREATE EXTENSION test_dsm_registry;");
-my $result = $node->safe_psql('test', "SELECT get_val_in_shmem();");
-is($result, "1236", "check shmem val");
+
+my ($initial_value, $result, $custom_script);
+
+$initial_value = $node->safe_psql('test', "SELECT get_val_in_shmem();");
+
+$custom_script = File::Temp->new();
+append_to_file($custom_script, q{
+ \set val random(0, 1999)
+ SELECT increment_val_in_shmem(:val);
+ SELECT increment_val_in_shmem(-(:val));
+});
+
+$node->command_ok([ 'pgbench', '-i', 'test' ], 'Init database');
+$node->command_ok([ 'pgbench', '-t', '31', '-c', '3', '-j', '3',
+ '-f', "$custom_script", 'test' ],
+ 'Change shared value simlutaneously');
+
+$result = $node->safe_psql('test', "SELECT get_val_in_shmem();");
+print("res $initial_value, $result");
+is($result, $initial_value, "Check concurrent access");
$node->stop;
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
index 8c55b0919b..0144845afa 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -3,7 +3,7 @@
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION test_dsm_registry" to load this file. \quit
-CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
+CREATE FUNCTION increment_val_in_shmem(val INT) RETURNS VOID
AS 'MODULE_PATHNAME' LANGUAGE C;
CREATE FUNCTION get_val_in_shmem() RETURNS INT
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.c
b/src/test/modules/test_dsm_registry/test_dsm_registry.c
index 8f78012e52..eed29a9f34 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry.c
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -13,33 +13,48 @@
#include "postgres.h"
#include "fmgr.h"
+#include "common/pg_prng.h"
#include "port/atomics.h"
#include "storage/dsm_registry.h"
+#include "storage/lwlock.h"
PG_MODULE_MAGIC;
-static pg_atomic_uint32 *val;
+typedef struct
+{
+ LWLock lock;
+ uint32 value;
+} SharedState;
+
+static SharedState *extstate;
static void
-init_val(void *ptr)
+init_dsm(void *ptr)
{
- pg_atomic_init_u32(ptr, 0);
+ SharedState *state = (SharedState *) ptr;
+
+ LWLockInitialize(&state->lock, LWLockNewTrancheId());
+ state->value = pg_prng_uint32(&pg_global_prng_state);
}
static void
dsm_registry_attach(void)
{
- dsm_registry_init_or_attach("test_dsm_registry", (void **) &val,
-
sizeof(pg_atomic_uint32), init_val);
+ dsm_registry_init_or_attach("test_dsm_registry", (void **) &extstate,
+
sizeof(SharedState), init_dsm);
}
-PG_FUNCTION_INFO_V1(set_val_in_shmem);
+PG_FUNCTION_INFO_V1(increment_val_in_shmem);
Datum
-set_val_in_shmem(PG_FUNCTION_ARGS)
+increment_val_in_shmem(PG_FUNCTION_ARGS)
{
+ uint32 increment = PG_GETARG_UINT32(0);
+
dsm_registry_attach();
- (void) pg_atomic_exchange_u32(val, PG_GETARG_UINT32(0));
+ LWLockAcquire(&extstate->lock, LW_EXCLUSIVE);
+ extstate->value += increment;
+ LWLockRelease(&extstate->lock);
PG_RETURN_VOID();
}
@@ -48,7 +63,13 @@ PG_FUNCTION_INFO_V1(get_val_in_shmem);
Datum
get_val_in_shmem(PG_FUNCTION_ARGS)
{
+ uint32 result;
+
dsm_registry_attach();
- PG_RETURN_UINT32(pg_atomic_fetch_add_u32(val, 0));
+ LWLockAcquire(&extstate->lock, LW_SHARED);
+ result = extstate->value;
+ LWLockRelease(&extstate->lock);
+
+ PG_RETURN_UINT32(result);
}
