Re: Accessing an invalid pointer in BufferManagerRelation structure

Hi,
Postgres allows us to pass BufferManagerRelation structure to
functions in two ways : BMR_REL and BMR_SMGR.
In case we use BMR_REL, the "smgr" field of structure initialized this way :
***
if (bmr.smgr == NULL)
{
bmr.smgr = RelationGetSmgr(bmr.rel);
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
}
***
Thus, we set the "smgr" field only once. But in case of frequent cache
invalidation (for example with debug_discard_caches parameter
enabled),
this pointer may become invalid (because RelationCloseSmgr will be called).
I have not found any places in the current code where this could
happen. But if (just for example) we add acquiring of new lock into
ExtendBufferedRelLocal
or ExtendBufferedRelShared, relation cache will be invalidated (inside
AcceptInvalidationMessages).

I would suggest adding a special macro to access the "smgr" field
(check attached patch for REL_17_STABLE).
What do you think about this?

--
Best regards,
Daniil Davydov

Hi,
I am posting the new v2 patch, which is now rebased on the `master` branch.
Waiting for your feedback :)

--
Best regards,
Daniil Davydov

Hello,

Dmitriy Bondar and I decided to review this patch as part of the CommitFest
Review.

The first thing we both noticed is that the macro calls a function that
won't be available without an additional header. This seems a bit
inconvenient.

I also have a question: is the logic correct that if the relation is valid,
we should fetch it rather than the other way around? Additionally, is
checking only the `rd_isvalid` flag sufficient, or should we also consider
the flag below?

```
bool rd_isvalid; /* relcache entry is valid */
```

Other than that, the tests pass, and there are no issues with code style.
Thanks for the patch - it could indeed help prevent future issues.
Best regards,
Stepan Neretin

вт, 11 мар. 2025 г., 17:32 Daniil Davydov <3danissimo(at)gmail(dot)com>:

> Hi,
> I am posting the new v2 patch, which is now rebased on the `master` branch.
> Waiting for your feedback :)
>
> --
> Best regards,
> Daniil Davydov
>

Hi,

On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf(at)gmail(dot)com> wrote:
>
> The first thing we both noticed is that the macro calls a function that won't be available without an additional header. This seems a bit inconvenient.

Well, I rebased the patch onto the latest `master`
(b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't
need to include `rel.h` in `localbuf.c` directly anymore, because
`#include lmgr.h` was added in memutils.h
I guess it solves this issue. Please, see v3 patch.

> I also have a question: is the logic correct that if the relation is valid, we should fetch it rather than the other way around? Additionally, is checking only the `rd_isvalid` flag sufficient, or should we also consider the flag below?
>
> ```
> bool rd_isvalid; /* relcache entry is valid */
>

I don't think that we should check any Relation's flags here. We are
checking `RelationIsValid((bmr).rel) ?` to decide whether
BufferManagerRelation was created via BMR_REL or BMR_SMGR.
If the `rel` field is not NULL, we can definitely say that BMR_REL was
used, so we should call RelationGetSmgr in order to access smgr.

--
Best regards,
Daniil Davydov

On Mon, Apr 14, 2025 at 1:10 PM Daniil Davydov <3danissimo(at)gmail(dot)com> wrote:

> Hi,
>
> On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf(at)gmail(dot)com> wrote:
> >
> > The first thing we both noticed is that the macro calls a function that
> won't be available without an additional header. This seems a bit
> inconvenient.
>
> Well, I rebased the patch onto the latest `master`
> (b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't
> need to include `rel.h` in `localbuf.c` directly anymore, because
> `#include lmgr.h` was added in memutils.h
> I guess it solves this issue. Please, see v3 patch.
>
> > I also have a question: is the logic correct that if the relation is
> valid, we should fetch it rather than the other way around? Additionally,
> is checking only the `rd_isvalid` flag sufficient, or should we also
> consider the flag below?
> >
> > ```
> > bool rd_isvalid; /* relcache entry is valid */
> >
>
> I don't think that we should check any Relation's flags here. We are
> checking `RelationIsValid((bmr).rel) ?` to decide whether
> BufferManagerRelation was created via BMR_REL or BMR_SMGR.
> If the `rel` field is not NULL, we can definitely say that BMR_REL was
> used, so we should call RelationGetSmgr in order to access smgr.
>
> --
> Best regards,
> Daniil Davydov
>

Hi, now looks good for me.
--
Best regards,
Stepan Neretin

On 2025-Apr-14, Daniil Davydov wrote:

> On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf(at)gmail(dot)com> wrote:

> > The first thing we both noticed is that the macro calls a function
> > that won't be available without an additional header. This seems a
> > bit inconvenient.

I think this is a fact of life. We don't have an automated check for
such patterns anymore, so the users of the macro will have to include
the other file. In any case I would rather they include utils/rel.h
themselves, than forcing rel.h into every single file that includes
bufmgr.h.

> Well, I rebased the patch onto the latest `master`
> (b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't
> need to include `rel.h` in `localbuf.c` directly anymore, because
> `#include lmgr.h` was added in memutils.h
> I guess it solves this issue. Please, see v3 patch.

Not anymore.

I edited your comments a bit. What do you think of this version?

I don't think this is backpatchable material, mainly because you said
you don't see any situations in which relcache invalidations would
occur in the middle of this. Can you can cause a problem to occur by
adding an untimely invalidation somewhere without this patch, which is
fixed by this patch?

BTW how does this interact with SMgrRelation pinning?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees." (E. Dijkstra)

Hi,

On Sun, Oct 19, 2025 at 6:32 PM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> I edited your comments a bit. What do you think of this version?
>

Thanks for looking into this! All added changes LGTM.

> I don't think this is backpatchable material, mainly because you said
> you don't see any situations in which relcache invalidations would
> occur in the middle of this. Can you can cause a problem to occur by
> adding an untimely invalidation somewhere without this patch, which is
> fixed by this patch?

Yep, by now (if I did not miss anything) described problem cannot occur even
with debug_discard_caches parameter on. But one of our clients had faced
this problem, because we have some differences in source code. In the first
letter I described scenario, when the occurrence of a problem becomes possible :
***
if (just for example) we add acquiring of new lock into ExtendBufferedRelLocal
or ExtendBufferedRelShared, relation cache will be invalidated (inside
AcceptInvalidationMessages).
***

>
> BTW how does this interact with SMgrRelation pinning?

Hm, maybe I don't fully understand the question. Smgr pinning is one of the side
effects of BMR_GET_SMGR (in case we use this macro with
invalidated/uninitialized
rd_smgr entry).
Without patch pincount cannot save us from undesirable invalidation,
because it guards
from smgr_destroy, not from smgr_release.

--
Best regards,
Daniil Davydov

On 2025-Oct-21, Daniil Davydov wrote:

> On Sun, Oct 19, 2025 at 6:32 PM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
> >
> > I edited your comments a bit. What do you think of this version?
>
> Thanks for looking into this! All added changes LGTM.

Okay, pushed, thanks.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/