From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: introduce dynamic shared memory registry
Date: Wed, 20 Dec 2023 11:02:58 +0200
Message-ID: <80cbdd55-8a9d-4472-b360-893309e1f6ef@postgrespro.ru> (raw)
In-Reply-To: <ZYIvcH8R83hEOKKe@paquier.xyz>
References: <20231205034647.GA2705267@nathanxps13>
<d725b5a7-5507-4630-9482-46f28c3bf0d1@postgrespro.ru>
<3dcea2e9-c46d-4e4f-8371-9526d1aebc83@postgrespro.ru>
<CA+Tgmobkqm0EfDw4jjn+Hbc=-5uh6nv8QTxHBj9bMpcQsfXV0g@mail.gmail.com>
<20231219161444.GD831499@nathanxps13>
<ZYIvcH8R83hEOKKe@paquier.xyz>
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.lepikhov@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
Attachments:elaborate_tests.txt (text/plain, 4.0K) downloaddiff --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.plindex 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.sqlindex 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.cindex 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);
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.lepikhov@postgrespro.ru, michael@paquier.xyz, nathandbossart@gmail.com, robertmhaas@gmail.com
Subject: Re: introduce dynamic shared memory registry
In-Reply-To: <80cbdd55-8a9d-4472-b360-893309e1f6ef@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox