Thread

Re: like pg_shmem_allocations, but fine-grained for DSM registry ?

Florents Tselai <florents.tselai@gmail.com> — 2025-06-03T19:39:25Z

Thanks for the detailed review Nathan

> On 3 Jun 2025, at 4:52 PM, Nathan Bossart <nathandbossart@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';
> 
> -- 
> nathan

Attaching a v4 which resolves these and also adds a doc entry.