Обсуждение: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)

On Mon, Jun 5, 2023 at 4:24 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela <ranier.vf@gmail.com> escreveu:
>> Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <guofenglinux@gmail.com> escreveu:
>>> On Sun, Jun 4, 2023 at 8:42 PM 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 == NULL))
>> smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend));
>> return rel->rd_smgr;
>> }
>
> Sorry Richard, nevermind.
>
> My fault, I withdraw this patch.

I'm not quite convinced of the reasoning provided; either the reason
is not good enough, or my C is rusty. In either case, I'd like a
resolution.

The code in question:

>       LockRelationForExtension(eb.rel, ExclusiveLock);
>
>       /* could have been closed while waiting for lock */
>       if (eb.rel)
>           eb.smgr = RelationGetSmgr(eb.rel);

eb.rel is being passed by-value at line 1, so even if the relation is
closed, the value of the eb.rel cannot change between line 1 and line
3. So a code verification tool complaining that the 'if' condition
will always be true is quite right, IMO.

To verify my assumptions, I removed those checks and ran `make
{check,check-world,installcheck}`, and all those tests passed.

The only way, that I can think of, the value of eb.rel can change
between lines 1 and 3 is if 'eb' is a shared-memory data structure,
and some other process changed the 'rel' member in shared-memory. And
I don't think 'eb' is in shared memory in this case.

To me, it looks like these checks are a result of code being
copy-pasted from somewhere else, where this check might have been
necessary. The checks are sure not necessary at these spots.

Please see attached v2 of the patch; it includes both occurrences of
the spurious checks identified in this thread.

Best regards,
Gurjeet
http://Gurje.et

On Mon, Jun 12, 2023 at 10:51:24PM -0700, Gurjeet Singh wrote:
> To me, it looks like these checks are a result of code being
> copy-pasted from somewhere else, where this check might have been
> necessary. The checks are sure not necessary at these spots.

I am not completely sure based on my read of the code, but isn't this
check needed to avoid some kind of race condition with a concurrent
backend may have worked on the relation when attempting to get the
lock?
--
Michael

At Tue, 13 Jun 2023 15:11:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Jun 12, 2023 at 10:51:24PM -0700, Gurjeet Singh wrote:
> > To me, it looks like these checks are a result of code being
> > copy-pasted from somewhere else, where this check might have been
> > necessary. The checks are sure not necessary at these spots.
> 
> I am not completely sure based on my read of the code, but isn't this
> check needed to avoid some kind of race condition with a concurrent
> backend may have worked on the relation when attempting to get the
> lock?

Gurjeet has mentioned that eb.rel cannot be modified by another
process since the value or memory is in the local stack, and I believe
he's correct.

If the pointed Relation had been blown out, eb.rel would be left
dangling, not nullified. However, I don't believe this situation
happens (or it shouldn't happen) as the entire relation should already
be locked.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 13 Jun 2023 08:21:20 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Em ter., 13 de jun. de 2023 às 02:51, Gurjeet Singh <gurjeet@singh.im>
> escreveu:
>
> > Please see attached v2 of the patch; it includes both occurrences of
> > the spurious checks identified in this thread.
> >
> +1
> LGTM.


>          LockRelationForExtension(eb.rel, ExclusiveLock);
>
> -        /* could have been closed while waiting for lock */
> -        if (eb.rel)
> -            eb.smgr = RelationGetSmgr(eb.rel);
> +        eb.smgr = RelationGetSmgr(eb.rel);

(It seems to me) The removed comment does refer to smgr, so we could
consider keeping it.  There are places instances where the function
calls are accompanied by similar comments and others where they
aren't. However, personally, I inclined towards its removal. That's
because our policy is to call RelationGetSmgr() each time before using
smgr, and this is well documented in the function's comment.

If we decide to remove it, the preceding blank line seems to be a
separator from the previous function call. So, we might want to
consider removing that blank line, too.

Otherwise it LGTM.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 14 Jun 2023 10:01:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> If we decide to remove it, the preceding blank line seems to be a
> separator from the previous function call. So, we might want to

Mmm. that is a bit short. Anyway I meant that the blank will become
useless after removing the comment.

> consider removing that blank line, too.
> 
> Otherwise it LGTM.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jun 14, 2023 at 5:12 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em qua., 14 de jun. de 2023 às 06:51, Richard Guo <guofenglinux@gmail.com> escreveu:
>>
>>
>> On Tue, Jun 13, 2023 at 3:39 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>>>
>>> Gurjeet has mentioned that eb.rel cannot be modified by another
>>> process since the value or memory is in the local stack, and I believe
>>> he's correct.
>>>
>>> If the pointed Relation had been blown out, eb.rel would be left
>>> dangling, not nullified. However, I don't believe this situation
>>> happens (or it shouldn't happen) as the entire relation should already
>>> be locked.
>>
>>
>> Yeah, Gurjeet is right.  I had a thinko here.  eb.rel should not be NULL
>> pointer in any case.  And as we've acquired the lock for it, it should
>> not have been closed.  So I think we can remove the check for eb.rel in
>> the two places.
>
> Ok,
> As there is a consensus on removing the tests and the comment is still relevant,
> here is a new version for analysis.

LGTM.

Best regards,
Gurjeet
http://Gurje.et

On Thu, Jul 6, 2023 at 8:01 AM Karina Litskevich
<litskevichkarina@gmail.com> wrote:
>
>
>> 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.
>
>
> As long as a structure is initialized, its fields that are not present in
> initialization are initialized to zeros and NULLs depending on their types.
> See C99 Standard 6.7.8.21 and 6.7.8.10. This behaviour is quite well known,
> so I don't think this place is buggy. Anyway, if someone else says the code
> is more readable with these fields initialized explicitly, then go on.

Even though I am not a fan of the Designated Initializers feature, I
agree with Karina. Per the standard, the unmentioned fields get
initialized to zeroes/NULLs, so the explicit initialization to
zero/null that this additional patch does is unnecessary. Moreover, I
feel that it makes the code less pleasant to read.

C99, 6.7.8.21:
> If there are fewer initializers in a brace-enclosed list than there are
> elements or members of an aggregate, or fewer characters in a string literal
> used to initialize an array of known size than there are elements in the array,
> the remainder of the aggregate shall be initialized implicitly the same as
> objects that have static storage duration.

C99, 6.7.8.10:
> If an object that has automatic storage duration is not initialized explicitly,
> its value is indeterminate. If an object that has static storage duration is
> not initialized explicitly, then:
> - if it has pointer type, it is initialized to a null pointer;
> - if it has arithmetic type, it is initialized to (positive or unsigned) zero;
> - if it is an aggregate, every member is initialized (recursively) according to these rules;
> - if it is a union, the first named member is initialized (recursively) according to these rules.


Best regards,
Gurjeet
http://Gurje.et

On 30.06.23 20:48, Karina Litskevich wrote:
> 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.
> 
> 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.
> 
> I attached the new version of the patch as I see it.

This patch version looks the most sensible to me.  But as commented 
further downthread, some explanation around the added assertions would 
be good.

On Mon, 4 Sept 2023 at 21:40, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 30.06.23 20:48, Karina Litskevich wrote:
> > 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.
> >
> > 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.
> >
> > I attached the new version of the patch as I see it.
>
> This patch version looks the most sensible to me.  But as commented
> further downthread, some explanation around the added assertions would
> be good.

The patch which you submitted has been awaiting your attention for
quite some time now.  As such, we have moved it to "Returned with
Feedback" and removed it from the reviewing queue. Depending on
timing, this may be reversible.  Kindly address the feedback you have
received, and resubmit the patch to the next CommitFest.

Regards,
Vignesh

On Fri, Feb 02, 2024 at 12:04:39AM +0530, vignesh C wrote:
> The patch which you submitted has been awaiting your attention for
> quite some time now.  As such, we have moved it to "Returned with
> Feedback" and removed it from the reviewing queue. Depending on
> timing, this may be reversible.  Kindly address the feedback you have
> received, and resubmit the patch to the next CommitFest.

Even with that, it seems to me that this is not required now that
21d9c3ee4ef7 outlines better how long SMgrRelation pointers should
live, no?
--
Michael