Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)

Em sex., 30 de jun. de 2023 às 15:48, Karina Litskevich <
litskevichkarina(at)gmail(dot)com> escreveu:

> Hi,
>
> Good catch. Indeed, eb.rel shouldn't be NULL there and the tests should be
> unnecessary.
>
Thanks for the confirmation.

> However, it doesn't follow from the code of these functions.
> From what I can see eb.rel can be NULL in both of these functions. There is
> the following Assert in the beginning of the ExtendBufferedRelTo()
> function:
>
> Assert((eb.rel != NULL) != (eb.smgr != NULL));
>
> And ExtendBufferedRelShared() is only called from ExtendBufferedRelCommon()
> which can be called from ExtendBufferedRelTo() or ExtendBufferedRelBy()
> that
> also has the same Assert(). And none of these functions assigns eb.rel, so
> it can be NULL from the very beginning and it stays the same.
>
>
> And there is the following call in xlogutils.c, which is exactly the case
> when
> eb.rel is NULL:
>
> buffer = ExtendBufferedRelTo(EB_SMGR(smgr, RELPERSISTENCE_PERMANENT),
> forknum,
> NULL,
> EB_PERFORMING_RECOVERY |
> EB_SKIP_EXTENSION_LOCK,
> blkno + 1,
> mode);
>
EB_SMGR and EB_REL are macros for making new structs.
IMO these are buggy, once make new structs without initializing all fields.
Attached a patch to fix this and make more clear when rel or smgr is NULL.

>
>
>
> So as for me calling LockRelationForExtension() and
> UnlockRelationForExtension()
> without testing eb.rel first looks more like a bug here. However, they are
> never
> actually called with eb.rel=NULL because of the EB_* flags, so there is no
> bug
> here. I believe we should add Assert checking that when eb.rel is NULL,
> flags
> are such that we won't use eb.rel. And yes, we can remove unnecessary
> checks
> where the flags value guaranty us that eb.rel is not NULL.
>
Not against these Asserts, but It is very confusing and difficult to
understand them without some comment.

>
> And another minor observation. It seems to me that we don't need a "could
> have
> been closed while waiting for lock" in ExtendBufferedRelShared(), because I
> believe the comment above already explains why updating eb.smgr:
>
> * Note that another backend might have extended the relation by the time
> * we get the lock.
>
Ok, but the first comment still ambiguous, I think that could be:
"/* eb.smgr could have been closed while waiting for lock */"

best regards,
Ranier Vilela