Thread: Refactor UnpinBuffer()

Hi hackers,

The proposed patch removes the redundant `fixOwner` argument.

"""
The fixOwner bool argument ended up always being true, so it doesn't do much
anymore. Removing it doesn't necessarily affect the performance a lot, but at
least improves the readability. The procedure is static thus the extension
authors are not going to be upset.
"""

-- 
Best regards,
Aleksander Alekseev

On Wed, Sep 28, 2022 at 08:14:23PM +0300, Aleksander Alekseev wrote:
> +       ResourceOwnerForgetBuffer(CurrentResourceOwner, b);
> +
>         /* not moving as we're likely deleting it soon anyway */
>         ref = GetPrivateRefCountEntry(b, false);
>         Assert(ref != NULL);
> -
> -       if (fixOwner)
> -               ResourceOwnerForgetBuffer(CurrentResourceOwner, b);

Is it safe to move the call to ResourceOwnerForgetBuffer() to before the
call to GetPrivateRefCountEntry()?  From my quick skim of the code, it
seems like it should be safe, but I thought I'd ask the question.
Otherwise, LGTM.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan, Zhang,

Thanks for the review!

> Is it safe to move the call to ResourceOwnerForgetBuffer() to before the
> call to GetPrivateRefCountEntry()? From my quick skim of the code, it
> seems like it should be safe, but I thought I'd ask the question.
>
> Same question, have a look, it doesn’t seem to matter.

Yep, I had some doubts here as well but it seems to be safe.

--
Best regards,
Aleksander Alekseev

On Thu, Sep 29, 2022 at 1:52 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> > Is it safe to move the call to ResourceOwnerForgetBuffer() to before the
> > call to GetPrivateRefCountEntry()? From my quick skim of the code, it
> > seems like it should be safe, but I thought I'd ask the question.
> >
> > Same question, have a look, it doesn’t seem to matter.
>
> Yep, I had some doubts here as well but it seems to be safe.

The commit 2d115e47c861878669ba0814b3d97a4e4c347e8b that removed the
last UnpinBuffer() call with fixOwner as false in ReleaseBuffer().
This commit is pretty old and +1 for removing the unused function
parameter.

Also, it looks like changing the order of GetPrivateRefCountEntry()
and ResourceOwnerForgetBuffer() doesn't have any effect as they are
independent, but do we want to actually do that if there's no specific
reason?

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

Hi Bharath,

> Also, it looks like changing the order of GetPrivateRefCountEntry()
> and ResourceOwnerForgetBuffer() doesn't have any effect as they are
> independent, but do we want to actually do that if there's no specific
> reason?

If we keep the order as it is now the code will become:

```
    ref = GetPrivateRefCountEntry(b, false);
    Assert(ref != NULL);

    ResourceOwnerForgetBuffer(CurrentResourceOwner, b);

    Assert(ref->refcount > 0);
    ref->refcount--;
    if (ref->refcount == 0)
```

I figured it would not hurt to gather all the calls and Asserts
related to `ref` together. This is the only reason why I choose to
rearrange the order of the calls in the patch.

So, no strong opinion in this respect from my side. I'm fine with
keeping the existing order.

-- 
Best regards,
Aleksander Alekseev

I've marked this one as ready-for-committer.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Sep 29, 2022 at 10:35:20AM -0700, Nathan Bossart wrote:
> I've marked this one as ready-for-committer.

UnpinBuffer() is local to bufmgr.c, so it would not be an issue for
external code, and that's 10 callers that don't need to worry about
that anymore.  2d115e4 is from 2015, and nobody has used this option
since, additionally.

Anyway, per the rule of consistency with the surroundings (see
ReleaseBuffer() and ReleaseAndReadBuffer()), it seems to me that there
is a good case for keeping the adjustment of CurrentResourceOwner
before any refcount checks.  I have also kept a mention to
CurrentResourceOwner in the top comment of the function, and applied
that.
--
Michael