Question on LWLockMode in dsa.c

On Tue, Jan 30, 2024 at 6:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

While working on radix tree patch[1], John Naylor found that dsa.c
doesn't already use shared locks even in dsa_dump(). dsa_dump() seems
a pure read-only function so I thought we could use a shared lock mode
there. Is there any reason to use exclusive mode even in dsa_dump()?

check_for_freed_segments_locked which is called from dsa_dump does
mark a few variables of the dsa area to NULL right? Are you suggesting
to take initially a shared lock in dsa_dump and upgrade it to
exclusive mode in the unlikely event of
unlikely(area->freed_segment_counter != freed_segment_counter)
occurring in check_for_freed_segments_locked?

Ultimately, since we're trying to add a new function
dsa_get_total_size() that just returns
dsa_area_control.total_segment_size and therefore would also be a
read-only function, I'd like to find out the correct lock mode there.

[1] /messages/by-id/CANWCAZYYzoKp_4+1m5mn-TRD62BTwom8iLXLOWMsHkkwFi=rzg@mail.gmail.com

Do you see any lock contention or any other issues with exclusive
locks on dsa areas?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 30, 2024 at 4:43 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Tue, Jan 30, 2024 at 6:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

While working on radix tree patch[1], John Naylor found that dsa.c
doesn't already use shared locks even in dsa_dump(). dsa_dump() seems
a pure read-only function so I thought we could use a shared lock mode
there. Is there any reason to use exclusive mode even in dsa_dump()?

check_for_freed_segments_locked which is called from dsa_dump does
mark a few variables of the dsa area to NULL right?

Right. But IIUC these are backend-local variables, and we don't change
any shared variables.

Are you suggesting
to take initially a shared lock in dsa_dump and upgrade it to
exclusive mode in the unlikely event of
unlikely(area->freed_segment_counter != freed_segment_counter)
occurring in check_for_freed_segments_locked?

I thought we could just take a shared lock in dsa_dump().

Ultimately, since we're trying to add a new function
dsa_get_total_size() that just returns
dsa_area_control.total_segment_size and therefore would also be a
read-only function, I'd like to find out the correct lock mode there.

[1] /messages/by-id/CANWCAZYYzoKp_4+1m5mn-TRD62BTwom8iLXLOWMsHkkwFi=rzg@mail.gmail.com

Do you see any lock contention or any other issues with exclusive
locks on dsa areas?

Not particularly for now. But we don't need to unnecessarily take an
exclusive lock and even a shared lock looks safe at a glance. So I
need to convince myself that we need to take an exclusive lock there
in order to add a new dsa_get_total_size() function. And we might need
comments.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 30, 2024 at 6:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

While working on radix tree patch[1], John Naylor found that dsa.c
doesn't already use shared locks even in dsa_dump(). dsa_dump() seems
a pure read-only function so I thought we could use a shared lock mode
there. Is there any reason to use exclusive mode even in dsa_dump()?

Ultimately, since we're trying to add a new function
dsa_get_total_size() that just returns
dsa_area_control.total_segment_size and therefore would also be a
read-only function, I'd like to find out the correct lock mode there.

Doesn't seem like there is any reason for this to be an exclusive lock.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com