Thread: Question on LWLockMode in dsa.c

Question on LWLockMode in dsa.c

From
Masahiko Sawada
Date:
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.

Regards,

[1] https://www.postgresql.org/message-id/CANWCAZYYzoKp_4%2B1m5mn-TRD62BTwom8iLXLOWMsHkkwFi%3Drzg%40mail.gmail.com

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



Re: Question on LWLockMode in dsa.c

From
Bharath Rupireddy
Date:
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] https://www.postgresql.org/message-id/CANWCAZYYzoKp_4%2B1m5mn-TRD62BTwom8iLXLOWMsHkkwFi%3Drzg%40mail.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



Re: Question on LWLockMode in dsa.c

From
Masahiko Sawada
Date:
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] https://www.postgresql.org/message-id/CANWCAZYYzoKp_4%2B1m5mn-TRD62BTwom8iLXLOWMsHkkwFi%3Drzg%40mail.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



Re: Question on LWLockMode in dsa.c

From
Dilip Kumar
Date:
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