Horton

Thread

Commits

  1. Show size of DSAs and dshashes in pg_dsm_registry_allocations.

  2. Add template for adaptive radix tree

  1. show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Nathan Bossart <nathandbossart@gmail.com> — 2025-11-26T22:49:49Z

    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

  2. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Rahila Syed <rahilasyed90@gmail.com> — 2025-11-27T08:21:39Z

    Hi,

Thank you for this improvement. I think it will be very helpful to have the
ability to
report dsa and dshash memory sizes.

Please find below a few comments.

1. As a result of this change, the following comment
in pg_get_dsm_registry_allocations
 is incorrect.
   /*
                 * Since we can't know the size of DSA/dshash entries
without first
                 * attaching to them, return NULL for those.
                 */

2. +
+       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.

3. +/*
+ * Same as dsa_get_total_size(), but accepts a DSA handle.  The area must
have
+ * been created with dsa_create (not dsa_create_in_place).
+ */
+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.

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.

Thank you,
Rahila Syed

  3. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-01T15:29:43Z

    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

  4. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Robert Haas <robertmhaas@gmail.com> — 2025-12-01T15:58:55Z

    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

  5. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-01T16:47:43Z

    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

  6. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Chao Li <li.evan.chao@gmail.com> — 2025-12-01T21:35:34Z

    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” and maybe 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/

  7. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-01T23:04:23Z

    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

  8. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Rahila Syed <rahilasyed90@gmail.com> — 2025-12-02T08:58:29Z

    Hi,

The patch LGTM overall. I had tested the v1 and it worked fine.


> 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.
>

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.


>
> > +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.
>

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.


> > 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.
>
>
Fair enough. I was thinking of a scenario where a user might want
to see how much dsa memory is allocated in the client backend process.
However, I understand now that this view is designed for the entire cluster,
and adding a column which is process-specific could lead to confusion.


Thank you,
Rahila Syed

  9. Re: show size of DSAs and dshash tables in pg_dsm_registry_allocations

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-02T16:31:33Z

    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