Обсуждение: show size of DSAs and dshash tables in pg_dsm_registry_allocations

On Tue, Nov 25, 2025 at 07:13:03PM -0500, Robert Haas wrote:
> In my ideal world, it would probably show partially-initialized entires
> in some distinguishable way, like with a null size.

I reevaluated this view, and I think we can do what you suggest here.
Right now, we show NULL for DSAs and dshash tables because 1) we might not
be attached to them and 2) we don't have a pointer to the dsa_area * or
dshash_table * in local memory.  If we introduce a function that looks up
the size of a DSA given its handle (transiently attaching to the control
segment if needed), we can show the size for all successfully-initialized
entries.  Then, we can make NULL mean that the entry was
partially-initialized.

Patch attached.

-- 
nathan

On Thu, Nov 27, 2025 at 01:51:39PM +0530, Rahila Syed wrote:
> Thank you for this improvement. I think it will be very helpful to have
> the ability to report dsa and dshash memory sizes.

Thanks for reviewing!

> 1. As a result of this change, the following comment in
> pg_get_dsm_registry_allocations is incorrect.

Fixed.

> +       LWLockAcquire(&control->lock, LW_SHARED);
> 
> The dsa_get_total_size function takes LW_EXCLUSIVE lock for the same
> purpose. I wonder why that is the case and whether both should be
> consistent.

That function was added by commit ee1b30f, which AFAICT used an exclusive
lock just to stay consistent with the rest of dsa.c [0].  I don't see any
discussion about this in the original DSA thread [1].  Perhaps we could go
through dsa.c and switch to LW_SHARED where appropriate, although I doubt
it makes much difference.

> +size_t
> +dsa_get_total_size_from_handle(dsa_handle handle)
> 
> I believe this function will report the size as long as the dsa control
> structure is created within a dsm segment, since all dsm segments are
> tracked by the global list - dsm_segment_list, regardless of whether the
> dsa is created with dsa_create or dsa_create_in_place. In that case,
> perhaps we should update the comment above to reflect this.

Sorry, I'm not following what you think we should update the comment to
say.

> 4. Since, with this change, the size column will show memory allocation
> regardless of whether it is currently mapped in the local process, I
> think it would be helpful to add a boolean column to display the mapped
> status as a future enhancement.

Maybe, although I'm struggling to think of a scenario where that
information would be useful.

[0] https://postgr.es/m/CAD21AoDoWrbNf%2BK2Fwg2n%3DCZDHigjkndwqy_86BGgXBp9Kbq4Q%40mail.gmail.com
[1] https://postgr.es/m/CAEepm%3D1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA%40mail.gmail.com

-- 
nathan

On Mon, Dec 1, 2025 at 10:29 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > 4. Since, with this change, the size column will show memory allocation
> > regardless of whether it is currently mapped in the local process, I
> > think it would be helpful to add a boolean column to display the mapped
> > status as a future enhancement.
>
> Maybe, although I'm struggling to think of a scenario where that
> information would be useful.

I'd be -1 on that idea. I think it's good to keep this as a view of
the global system state, rather than a view of the state of one
particular process.

The patch itself LGTM. I did a casual review only and did not attempt
to verify that it works properly, but I like both the idea and the
execution.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Dec 01, 2025 at 10:58:55AM -0500, Robert Haas wrote:
> The patch itself LGTM. I did a casual review only and did not attempt
> to verify that it works properly, but I like both the idea and the
> execution.

Thanks for reviewing.  Once Rahila is happy with the patch, I will commit
it.

-- 
nathan

Hi Nathan,

V2 overall looks good to me. I just got a question and a few nit comments.

> On Dec 1, 2025, at 23:29, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> --
> nathan
> <v2-0001-rework-DSM-registry-view.patch>

1 - dsa.c
```
+size_t
+dsa_get_total_size_from_handle(dsa_handle handle)
+{
+    size_t        size;
+    bool        already_attached;
+    dsm_segment *segment;
+    dsa_area_control *control;
+
+    already_attached = dsa_is_attached(handle);
+    if (already_attached)
+        segment = dsm_find_mapping(handle);
+    else
+        segment = dsm_attach(handle);
+
+    if (segment == NULL)
+        ereport(ERROR,
+                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                 errmsg("could not attach to dynamic shared area")));
```

Why do you error out instead of reporting 0/NULL usage here? When users call this function as shown in the test script:
```
CREATE VIEW pg_dsm_registry_allocations AS
SELECT * FROM pg_get_dsm_registry_allocations();
```

User doesn’t pass in a handle, if the DSA has been released, it should treat a missing mapping as “no size available”
andmaybe return NULL.  


2
```
+        else if (entry->type == DSMR_ENTRY_TYPE_DSH &&
+                 entry->dsh.dsa_handle !=DSA_HANDLE_INVALID)
```

Missing a white space after !=.

3
```
-       Size of the allocation in bytes.  NULL for entries of type
-       <literal>area</literal> and <literal>hash</literal>.
+       Size of the allocation in bytes.  NULL for entries that failed
+       initialization.
```

“Failed initialization”, I guess “to” is needed after “failed”.

4
```
-SELECT name, type, size IS DISTINCT FROM 0 AS size
+SELECT name, type, size > 0 AS size
```

As you changed the third column from “size” to “size > 0”, now it’s a bool column, so maybe rename the column alias
from“size” to something indicating a bool type, such as “size_ok”, “has_size”, etc. 


Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, Dec 02, 2025 at 05:35:34AM +0800, Chao Li wrote:
> V2 overall looks good to me. I just got a question and a few nit comments.

Thanks for reviewing.

> Why do you error out instead of reporting 0/NULL usage here?

If attaching to the control segment fails, something has gone horribly
wrong.  I don't think returning 0 for the size is appropriate in that case.
Also note that other functions in this file ERROR when attaching fails
(e.g., dsa_attach()).

> ```
> +        else if (entry->type == DSMR_ENTRY_TYPE_DSH &&
> +                 entry->dsh.dsa_handle !=DSA_HANDLE_INVALID)
> ```
> 
> Missing a white space after !=.

I agree, but for some reason, pgindent insists on removing that space.  I'm
leaving that for another thread.

> ```
> -       Size of the allocation in bytes.  NULL for entries of type
> -       <literal>area</literal> and <literal>hash</literal>.
> +       Size of the allocation in bytes.  NULL for entries that failed
> +       initialization.
> ```
> 
> “Failed initialization”, I guess “to” is needed after “failed”.

IMHO it's fine as-is.  We could say "failed to initialize", but I'm not
sure that's materially better at conveying the information.

> ```
> -SELECT name, type, size IS DISTINCT FROM 0 AS size
> +SELECT name, type, size > 0 AS size
> ```
> 
> As you changed the third column from “size” to “size > 0”, now it’s a
> bool column, so maybe rename the column alias from “size” to something
> indicating a bool type, such as “size_ok”, “has_size”, etc.

It was already a bool column, but your point still stands.  I changed it to
"size_ok".

-- 
nathan

On Tue, Dec 02, 2025 at 02:28:29PM +0530, Rahila Syed wrote:
> Thank you for highlighting the discussions. I'm unsure about the best
> approach here, but I think it would be safe to stay consistent with the
> rest of the code in dsa.c, especially since it's unclear that the use of
> LW_EXCLUSIVE for reading values in dsa is a mistake.

Okay.  I switched to LW_EXCLUSIVE and will consider starting a new thread
to use LW_SHARED when possible in dsa.c.

> Sorry for the confusion, I am trying to say that we can change the
> following comment
> 
> + *The area must have
> + * been created with dsa_create (not dsa_create_in_place).
> 
> to say this:
> 
> "The area must have been created using dsm_segments"
> 
> Since, this function can report the size of an area created with
> dsa_create_in_place too, as long as the area is created using
> dsm_segments.

It cannot report the size of in-place areas, at least not without some
changes, because (AFAICT) there's no way to get a dsa_handle for an
in-place segment.

I've now committed the patch.  Thanks everyone for reviewing!

-- 
nathan