Horton

Thread

Commits

  1. Make BufferIsExclusiveLocked and BufferIsDirty work for local buffers.

  1. Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> — 2024-12-05T13:08:16Z

    Hi,

Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?,these 2 functions are called only from XlogRegisterBuffer,AFAIK which will be called only for permanent relations.Please correct me if i am wrong.

Regards,
Srinath Reddy Sadipiralla
Member of Technical Staff
Zoho
 

  2. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Andres Freund <andres@anarazel.de> — 2024-12-05T15:41:42Z

    Hi,

On 2024-12-05 18:38:16 +0530, Srinath Reddy Sadipiralla wrote:
> Why we need to check for local buffers in BufferIsExclusiveLocked and
> BufferIsDirty?,these 2 functions are called only from
> XlogRegisterBuffer,AFAIK which will be called only for permanent
> relations.Please correct me if i am wrong.

That's maybe true for in-core code today, but what guarantees that that's true
for the future? And what about code in extensions?

The gain by not dealing with local buffers in these functions is fairly small
too, so there's not really any reason for a change like yours.

- Andres

  3. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> — 2024-12-06T04:03:30Z

    > ---- On Thu, 05 Dec 2024 21:11:42 +0530 Andres Freund <mailto:andres@anarazel.de> wrote ---



> Hi, 
 
> On 2024-12-05 18:38:16 +0530, Srinath Reddy Sadipiralla wrote: 
>> Why we need to check for local buffers in BufferIsExclusiveLocked and 
>> BufferIsDirty?,these 2 functions are called only from 
>> XlogRegisterBuffer,AFAIK which will be called only for permanent 
>> relations.Please correct me if i am wrong. 
 
> That's maybe true for in-core code today, but what guarantees that that's true 
> for the future? And what about code in extensions? 
 
> The gain by not dealing with local buffers in these functions is fairly small 
> too, so there's not really any reason for a change like yours. 
 
> - Andres 
 
 
hmm got it,if thats the case, for local buffers lockbuffer will skip acquiring content lock, so assert will fail in BufferIsDirty.



Regards,

Srinath Reddy Sadipiralla

Member of Technical Staff

Zoho

  4. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-06T11:10:51Z

    Hi,

On Fri, 6 Dec 2024 at 07:03, Srinath Reddy Sadipiralla
<srinath.reddy@zohocorp.com> wrote:
>
> > ---- On Thu, 05 Dec 2024 21:11:42 +0530 Andres Freund <andres@anarazel.de> wrote ---
>
> > Hi,
>
> > On 2024-12-05 18:38:16 +0530, Srinath Reddy Sadipiralla wrote:
> >> Why we need to check for local buffers in BufferIsExclusiveLocked and
> >> BufferIsDirty?,these 2 functions are called only from
> >> XlogRegisterBuffer,AFAIK which will be called only for permanent
> >> relations.Please correct me if i am wrong.
>
> > That's maybe true for in-core code today, but what guarantees that that's true
> > for the future? And what about code in extensions?
>
> > The gain by not dealing with local buffers in these functions is fairly small
> > too, so there's not really any reason for a change like yours.
>
> > - Andres
>
>
> hmm got it,if thats the case, for local buffers lockbuffer will skip acquiring content lock, so assert will fail in BufferIsDirty.

LGTM.

Adding Daniil to CC as he too started a similar thread [1].

[1] postgr.es/m/CAJDiXggGznOttwREfyZRE4f7oLRz1%3DjTA4xA7u-t6_8CX7j%3D0g%40mail.gmail.com

--
Regards,
Nazir Bilal Yavuz
Microsoft

  5. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> — 2024-12-07T08:17:21Z

    > ---- On Fri, 06 Dec 2024 16:40:51 +0530 Nazir Bilal Yavuz <mailto:byavuz81@gmail.com> wrote ---



 
> LGTM. 

> Regards, 
> Nazir Bilal Yavuz 
> Microsoft 

hi nazir,

sorry i didn't get,what you meant to say is the assert failure which i said is correct and does my patch to this looks good?🤔


Regards,

Srinath Reddy Sadipiralla

Member of Technical Staff

Zoho

  6. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-08T12:53:21Z

    Hi Srinath,

On Sat, 7 Dec 2024 at 11:17, Srinath Reddy Sadipiralla
<srinath.reddy@zohocorp.com> wrote:
> > ---- On Fri, 06 Dec 2024 16:40:51 +0530 Nazir Bilal Yavuz <byavuz81@gmail.com> wrote ---
> > LGTM.
>
> sorry i didn't get,what you meant to say is the assert failure which i said is correct and does my patch to this looks good?🤔

Sorry if I was not clear. Yes, I wanted to say what you said is
correct and the patch looks good to me.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  7. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> — 2024-12-09T04:44:06Z

    Hi,



added this bug to commitfest https://commitfest.postgresql.org/51/5428/ 



Regards,

Srinath Reddy Sadipiralla

Member of Technical Staff

Zoho

  8. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> — 2024-12-09T05:09:35Z

    ---- On Sun, 08 Dec 2024 18:23:21 +0530 Nazir Bilal Yavuz <byavuz81@gmail.com> wrote ---



Hi Srinath, 
 
On Sat, 7 Dec 2024 at 11:17, Srinath Reddy Sadipiralla 
<mailto:srinath.reddy@zohocorp.com> wrote: 
> > ---- On Fri, 06 Dec 2024 16:40:51 +0530 Nazir Bilal Yavuz <mailto:byavuz81@gmail.com> wrote --- 
> > LGTM. 
> 
> sorry i didn't get,what you meant to say is the assert failure which i said is correct and does my patch to this looks good?🤔 
 
Sorry if I was not clear. Yes, I wanted to say what you said is 
correct and the patch looks good to me. 
 
-- 
Regards, 
Nazir Bilal Yavuz 
Microsoft

  9. Fwd: Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> — 2024-12-09T05:10:35Z

    ============ Forwarded message ============
From: Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com>
To: "Nazir Bilal Yavuz"<byavuz81@gmail.com>, "pgsql-hackers"<pgsql-hackers@lists.postgresql.org>, "Andres Freund"<andres@anarazel.de>
Date: Mon, 09 Dec 2024 10:14:06 +0530
Subject: Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?
============ Forwarded message ============



Hi,



added this bug to commitfest https://commitfest.postgresql.org/51/5428/ 



Regards,

Srinath Reddy Sadipiralla

Member of Technical Staff

Zoho

  10. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-25T21:50:40Z

    Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> writes:
>> ---- On Thu, 05 Dec 2024 21:11:42 +0530 Andres Freund <mailto:andres@anarazel.de> wrote ---
>> The gain by not dealing with local buffers in these functions is fairly small 
>> too, so there's not really any reason for a change like yours. 
 
> hmm got it,if thats the case, for local buffers lockbuffer will skip acquiring content lock, so assert will fail in BufferIsDirty.

I think you are right about that, but

(1) it seems to be general style to check BufferIsPinned before
checking the content lock, and you've made that out-of-order.
This is easily fixed by moving the Assert(BufferIsPinned(buffer))
to earlier in the function.

(2) I don't think we should touch this but not worry about
BufferIsExclusiveLocked: it's unlikely to behave well on local
buffers either, since we don't initialize content locks for them.
We could either Assert that that's not applied to local buffers,
or act as though their lock is always held.  Given Andres' argument
probably the latter is better.

			regards, tom lane

  11. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-26T14:24:45Z

    as suggested did the changes and attached the patch for the same.

Regards,
Srinath Reddy Sadipiralla,
EDB: http://www.enterprisedb.com

On Sun, Jan 26, 2025 at 3:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Srinath Reddy Sadipiralla <srinath.reddy@zohocorp.com> writes:
> >> ---- On Thu, 05 Dec 2024 21:11:42 +0530 Andres Freund <mailto:
> andres@anarazel.de> wrote ---
> >> The gain by not dealing with local buffers in these functions is fairly
> small
> >> too, so there's not really any reason for a change like yours.
>
> > hmm got it,if thats the case, for local buffers lockbuffer will skip
> acquiring content lock, so assert will fail in BufferIsDirty.
>
> I think you are right about that, but
>
> (1) it seems to be general style to check BufferIsPinned before
> checking the content lock, and you've made that out-of-order.
> This is easily fixed by moving the Assert(BufferIsPinned(buffer))
> to earlier in the function.
>
> (2) I don't think we should touch this but not worry about
> BufferIsExclusiveLocked: it's unlikely to behave well on local
> buffers either, since we don't initialize content locks for them.
> We could either Assert that that's not applied to local buffers,
> or act as though their lock is always held.  Given Andres' argument
> probably the latter is better.
>
>                         regards, tom lane
>

  12. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-26T16:19:57Z

    Srinath Reddy <srinath2133@gmail.com> writes:
> as suggested did the changes and attached the patch for the same.

Uh ... what in the world is the point of changing
BufferIsExclusiveLocked's signature?

			regards, tom lane

  13. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-26T16:54:50Z

    On Sun, Jan 26, 2025 at 9:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Srinath Reddy <srinath2133@gmail.com> writes:
> > as suggested did the changes and attached the patch for the same.
>
> Uh ... what in the world is the point of changing
> BufferIsExclusiveLocked's signature?
>
>                         regards, tom lane
>

as there was repeated code between BufferIsExclusiveLocked and
BufferIsDirty to check if buffer is pinned and its locked exclusively,i
thought it would be nice to move that repeated code into
BufferIsExclusiveLocked and as we need bufHdr in BufferIsDirty which is
assigned in BufferIsExclusiveLocked,so I had to change the signature of
BufferIsExclusiveLocked by adding (BufferDesc **bufHdr).

Regards,
Srinath Reddy Sadipiralla,
EDB: http://www.enterprisedb.com

  14. Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-27T14:56:48Z

    On Sun, Jan 26, 2025 at 10:24 PM Srinath Reddy <srinath2133@gmail.com>
wrote:

>
> as there was repeated code between BufferIsExclusiveLocked and
> BufferIsDirty to check if buffer is pinned and its locked exclusively,i
> thought it would be nice to move that repeated code into
> BufferIsExclusiveLocked and as we need bufHdr in BufferIsDirty which is
> assigned in BufferIsExclusiveLocked,so I had to change the signature of
> BufferIsExclusiveLocked by adding (BufferDesc **bufHdr).
>

Hi Tom,
if this is not the answer you are expecting ,please let me know.I am open
for suggestions.

Regards,

  15. Fwd: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-29T01:41:49Z

    On Sun, Jan 26, 2025 at 9:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Srinath Reddy <srinath2133@gmail.com> writes:
> > as suggested did the changes and attached the patch for the same.
>
> Uh ... what in the world is the point of changing
> BufferIsExclusiveLocked's signature?
>
>
as suggested did not change the BufferIsExclusiveLocked's signature and
here's the patch for the same.

Regards,
Srinath Reddy Sadipiralla,
EDB: https://www.enterprisedb.com <http://www.enterprisedb.com/>

  16. Re: Fwd: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-29T18:32:05Z

    Srinath Reddy <srinath2133@gmail.com> writes:
> as suggested did not change the BufferIsExclusiveLocked's signature and
> here's the patch for the same.

Yeah, that's better.  The whole point of changing these is to make
them do what extensions could reasonably expect, so a new API does
not seem like what's wanted.

If we cared about the speed of that assertion in XLogRegisterBuffer,
I think the thing to do would be to invent a single new function that
tests for both dirty and exclusive-locked.  Really BufferIsDirty
does that already, at least in assertion builds, so we could just do

-        Assert(BufferIsExclusiveLocked(buffer) && BufferIsDirty(buffer));
+        Assert(BufferIsDirty(buffer));

But I don't think we actually care that much about the speed of an
assertion, and it might be confusing since this doesn't quite
match the comment above it.  So I'm inclined to leave that alone.

I pushed your patch after fooling with the comments a bit.

			regards, tom lane