Обсуждение: like pg_shmem_allocations, but fine-grained for DSM registry ?

On Thu, Mar 13, 2025 at 06:54:09PM +0200, Florents Tselai wrote:
> I´ve been working with the DSM registry API.
> I was wondering if it is possible (it doesn´t look like it) or if it has been discussed:
> can we expose a view like pg_shmem_allocations, but fine-grained for every named segment (i.e. created by
GetNamedDSMSegment)?
 
> 
> Currently, there is a "DSM Registry Data" entry in that view,
> but iiuc, it´s only about the top-level hash table the registry uses.

This seems like a generally reasonable idea to me.  In theory, it should be
easy enough to build something that walks through the DSM registry hash
table.

-- 
nathan

On Sat, Mar 15, 2025 at 10:41:15AM +0200, Florents Tselai wrote:
> Here's an updated v3 that fixes some white spaces v2 had-no other changes.

Thanks for the patch.

> I'm wondering though if it also makes sense to expose:
> - backend_pid (who created the segment)
> - is_pinned bool
> - created_at

created_at might be interesting.  I'm not sure the others have much use.
It would be cool to surface which library/function created the segment
IMHO.  But for now, I'd keep the view simple and just show the contents of
the DSM registry's hash table.

+CREATE VIEW pg_dsm_registry AS
+SELECT * FROM pg_get_dsm_registry();

I'd suggest pg_dsm_registry_allocations and
pg_get_dsm_registry_allocations() to match pg_shmem_allocations.

+void
+iterate_dsm_registry(void (*callback)(DSMRegistryEntry *, void *), void *arg);
+void
+iterate_dsm_registry(void (*callback)(DSMRegistryEntry *, void *), void *arg)
+{

I'm not sure what's going on here.  Presumably we could just mark
iterate_dsm_registry() as static.

Taking a step back, I think the loop is quite overengineered.  You have a
function for calling dshash_seq_init/next/term, a callback function for
storing the tuple, and a special struct for some SRF context.  I don't see
any need to abstract things to this extent.  I think we should instead
open-code the loop in pg_get_dsm_registry().

+/* SQL SRF showing DSM registry allocated memory */
+PG_FUNCTION_INFO_V1(pg_get_dsm_registry);

There should be no need to do this.  Its pg_proc.dat entry will
automatically generate the required prototype.

+    if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+        ereport(ERROR, (errmsg("pg_get_dsm_registry must be used in a SRF context")));
+
+    /* Set up tuple descriptor */
+    tupdesc = CreateTemplateTupleDesc(2);
+    TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name", TEXTOID, -1, 0);
+    TupleDescInitEntry(tupdesc, (AttrNumber) 2, "size", INT8OID, -1, 0);

This SRF-related code can be simplified by using InitMaterializedSRF() and
friends.  Check out pg_ls_dir() in genfile.c for an example.

+-- 20 bytes = int (4 bytes) + LWLock (16bytes)
+SELECT * FROM pg_dsm_registry;
+       name        | size 
+-------------------+------
+ test_dsm_registry |   20
+(1 row)

I'm a little worried about the portability of this test.  I would suggest
changing the query to something like

    SELECT size > 0 FROM pg_dsm_registry WHERE name = 'test_dsm_registry';

-- 
nathan

On Tue, Jun 03, 2025 at 10:39:25PM +0300, Florents Tselai wrote:
> Thanks for the detailed review Nathan

Thanks for the updated patch!

+    if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+        ereport(ERROR, (errmsg("pg_get_dsm_registry_allocations must be used in a SRF context")));

InitMaterializedSRF() takes care of this for you.

+typedef struct
+{
+    Tuplestorestate *tupstore;
+    TupleDesc        tupdesc;
+} DSMRegistrySRFContext;

This appears to be unused.

+#include "fmgr.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "utils/builtins.h"

Do we need fmgr.h and miscadmin.h?  Also, please alphabetize these into the
existing list of #includes.

+        values[1] = Int64GetDatum(entry->size);

I think there's a sign mismatch problem here, but it seems implausible in
practice.  pg_get_shmem_allocations() does the same thing.

+ <sect1 id="view-pg-dsm-registry-allocations">
+  <title><structname>pg_dsm_registry_allocations</structname></title>

We need to add an entry into the System Views table in the Overview page,
too.

-- 
nathan

This latest patch set looks pretty good to me.

-- 
nathan

On Wed, Jun 04, 2025 at 11:17:42AM +0300, Florents Tselai wrote:
> Thanks; let's wait a bit to see if there're any objections.

Would you mind combining the patches into just one patch?  That's how I
reviewed it, and that's how it would be committed.

-- 
nathan

> On 4 Jun 2025, at 7:19 PM, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Wed, Jun 04, 2025 at 11:17:42AM +0300, Florents Tselai wrote:
>> Thanks; let's wait a bit to see if there're any objections.
>
> Would you mind combining the patches into just one patch?  That's how I
> reviewed it, and that's how it would be committed.
>

This works ?

On Wed, Jun 04, 2025 at 09:03:03PM +0300, Florents Tselai wrote:
> On 4 Jun 2025, at 7:19 PM, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Would you mind combining the patches into just one patch?  That's how I
>> reviewed it, and that's how it would be committed.
> 
> This works ?

Yes, thanks.

-- 
nathan

I think this will need some rework to deal with the addition of
GetNamedDSA() and GetNamedDSHash() [0].  We probably want to add a "type"
column to show whether the entry is for a DSM, DSA, or dshash table.  And
for DSAs and dshash tables, we probably want to use dsa_get_total_size()
for the "size" column.

[0] https://postgr.es/m/flat/aEC8HGy2tRQjZg_8@nathan

-- 
nathan

On Tue, Jul 08, 2025 at 07:17:54PM +0800, Florents Tselai wrote:
> I can't see how it's possible to get the actual size for the dsa and dsh case,
> without attaching and then using, dsa_get_total_size on the attached dsa.
> And I don't think we wanna do that.
> 
> Instead maybe we can just report NULL for the dsa and dsh cases? 

Yeah, I'm not sure what else we can do about that without a bunch of
refactoring work on the DSA/dshash side, and it's probably not worth the
effort, anyway.

-- 
nathan

> On 8 Jul 2025, at 11:21 PM, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Tue, Jul 08, 2025 at 07:17:54PM +0800, Florents Tselai wrote:
>> I can't see how it's possible to get the actual size for the dsa and dsh case,
>> without attaching and then using, dsa_get_total_size on the attached dsa.
>> And I don't think we wanna do that.
>>
>> Instead maybe we can just report NULL for the dsa and dsh cases?
>
> Yeah, I'm not sure what else we can do about that without a bunch of
> refactoring work on the DSA/dshash side, and it's probably not worth the
> effort, anyway.


I agree. In that case I think v5 should be enough.

Here is what I have staged for commit, which I'm planning to do tomorrow.

-- 
nathan

Committed.

-- 
nathan