Thread: 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
Attachment
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
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@gmail.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@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
Attachment
On Mon, Apr 14, 2025 at 1:10 PM Daniil Davydov <3danissimo@gmail.com> wrote:
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 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