MIME-Version: 1.0
References: <CAEudQAoSA7saBF7RaoYbRjE+Zojojin0Qc9A4hebPKB8o1J+EQ@mail.gmail.com>
 <CAMbWs4_Jv4p_ib2ZZvUYhgoeSpOABjAW_4chkAhrSU=iW8CRGA@mail.gmail.com>
In-Reply-To: <CAMbWs4_Jv4p_ib2ZZvUYhgoeSpOABjAW_4chkAhrSU=iW8CRGA@mail.gmail.com>
From: Ranier Vilela <ranier.vf@gmail.com>
Date: Mon, 5 Jun 2023 08:06:26 -0300
Message-ID: <CAEudQArD04043ahRO_9mXiYSfhvsac4SSyskUVk5PTm73Ab24A@mail.gmail.com>
Subject: Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)
To: Richard Guo <guofenglinux@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000084a78a05fd5fe5e5"
Archived-At: <https://www.postgresql.org/message-id/CAEudQArD04043ahRO_9mXiYSfhvsac4SSyskUVk5PTm73Ab24A%40mail.gmail.com>
Precedence: bulk

--00000000000084a78a05fd5fe5e5
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Em dom., 4 de jun. de 2023 =C3=A0s 23:37, Richard Guo <guofenglinux@gmail.c=
om>
escreveu:

>
> On Sun, Jun 4, 2023 at 8:42=E2=80=AFPM Ranier Vilela <ranier.vf@gmail.com=
> wrote:
>
>> Hi,
>>
>> Per Coverity.
>>
>> At function ExtendBufferedRelShared, has a always true test.
>> eb.rel was dereferenced one line above, so in
>> if (eb.rel) is always true.
>>
>> I think it's worth removing the test, because Coverity raises dozens of
>> alerts thinking eb.rel might be NULL.
>> Besides, one less test is one less branch.
>>
>
> This also happens in ExtendBufferedRelTo, and the comment there explains
> that the eb.rel 'could have been closed while waiting for lock'.
>
Well, RelationGetSmgr also dereferences eb.rel.
If eb.rel could be closed while waiting for lock,
anyone who references eb.rel below takes a risk?

static inline SMgrRelation
RelationGetSmgr(Relation rel)
{
if (unlikely(rel->rd_smgr =3D=3D NULL))
smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend));
return rel->rd_smgr;
}

regards,
Ranier Vilela

--00000000000084a78a05fd5fe5e5
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail=
_attr">Em dom., 4 de jun. de 2023 =C3=A0s 23:37, Richard Guo &lt;<a href=3D=
"mailto:guofenglinux@gmail.com">guofenglinux@gmail.com</a>&gt; escreveu:<br=
></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;=
border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir=3D"ltr"><=
div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div dir=3D"ltr" class=
=3D"gmail_attr">On Sun, Jun 4, 2023 at 8:42=E2=80=AFPM Ranier Vilela &lt;<a=
 href=3D"mailto:ranier.vf@gmail.com" target=3D"_blank">ranier.vf@gmail.com<=
/a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0=
px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><=
div dir=3D"ltr"><div>Hi,</div><div><br></div><div>Per Coverity.</div><div><=
br></div><div>At function ExtendBufferedRelShared, has a always true test.<=
/div><div>eb.rel was dereferenced one line above, so in <br></div><div>if (=
eb.rel) is always true.</div><div><br></div><div>I think it&#39;s worth rem=
oving the test, because Coverity raises dozens of alerts thinking eb.rel mi=
ght be NULL.</div><div>Besides, one less test is one less branch.</div></di=
v></blockquote><div><br></div><div>This also happens in ExtendBufferedRelTo=
, and the comment there explains<br>that the eb.rel &#39;could have been cl=
osed while waiting for lock&#39;.</div></div></div></blockquote><div>Well, =
RelationGetSmgr also dereferences eb.rel.</div><div>If eb.rel could be clos=
ed while waiting for lock,</div><div>anyone who references eb.rel below tak=
es a risk?<br></div><div>=C2=A0</div><div>static inline SMgrRelation<br>Rel=
ationGetSmgr(Relation rel)<br>{<br>	if (unlikely(rel-&gt;rd_smgr =3D=3D NUL=
L))<br>		smgrsetowner(&amp;(rel-&gt;rd_smgr), smgropen(rel-&gt;rd_locator, =
rel-&gt;rd_backend));<br>	return rel-&gt;rd_smgr;<br>}</div></div><div clas=
s=3D"gmail_quote"><br></div><div class=3D"gmail_quote">regards,</div><div c=
lass=3D"gmail_quote">Ranier Vilela<br></div></div>

--00000000000084a78a05fd5fe5e5--