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

> 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.

> Not against these Asserts, but It is very confusing and difficult to
> understand them without some comment.
>

I'm not familiar enough with the code to write any comment that makes any
additional meaning. Assert by itself means "when the function is called with
eb.rel == NULL, then flags are supposed to contain EB_SKIP_EXTENSION_LOCK
and
to not contain EB_CREATE_FORK_IF_NEEDED". I can guess that it's because with
any of these flags we have to lock the relation and we can't do it if we
don't
know what is the relation. But it's my speculation, I won't write a comment
based on it. We better wait for someone who knows this code.

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 */"
>

It doesn't make a big difference for me, so you can add "eb.smgr" if you
want to.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/