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

Поиск
Список
Период
Сортировка
От Karina Litskevich
Тема Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)
Дата
Msg-id CACiT8iZC3NswchmNipaZN0o7_15e1zQyYONdqBSON1p-gE2-ew@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)  (Ranier Vilela <ranier.vf@gmail.com>)
Ответы Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)  (Gurjeet Singh <gurjeet@singh.im>)
Список pgsql-hackers

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/

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Daniel Gustafsson
Дата:
Сообщение: Re: Avoid overflow with simplehash
Следующее
От: Ranier Vilela
Дата:
Сообщение: Re: Avoid overflow with simplehash