Обсуждение: Re: Accessing an invalid pointer in BufferManagerRelation structure

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@gmail.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
wayaround? 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 2025-Apr-14, Daniil Davydov wrote:

> On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf@gmail.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@kurilemu.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@kurilemu.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/