Thanks for the detailed review Nathan
> On 3 Jun 2025, at 4:52 PM, Nathan Bossart <nathandboss...@gmail.com> wrote:
>
> 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';
>
> --
> nathanAttaching a v4 which resolves these and also adds a doc entry.
v4-0001-First-attempt-towards-a-pg_dsm_registry-view.-Ite.patch
Description: Binary data
v4-0002-Rename-view-to-pg_dsm_registry_allocations-and-fu.patch
Description: Binary data
v4-0003-Add-doc-entry-for-pg_shmem_allocations_numa.patch
Description: Binary data
