Re: Bufmgr possible overflow

Поиск
Список
Период
Сортировка
От Ranier Vilela
Тема Re: Bufmgr possible overflow
Дата
Msg-id CAEudQArHD1OWWP4Twa6JxY7+3sZTkQCz_vWkdW=Hvu7vEP1r2A@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Bufmgr possible overflow  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
Список pgsql-hackers
Em qua., 12 de abr. de 2023 às 22:29, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:
Perhaps it's a good idea to seprate the patch for each issue.

Thanks Kyotaro for taking a look.
 
At Wed, 12 Apr 2023 09:36:14 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in> IMO I think that commit 31966b1
> <https://github.com/postgres/postgres/commit/31966b151e6ab7a6284deab6e8fe5faddaf2ae4c>
> has an oversight.
>
> All the logic of the changes are based on the "extend_by" variable, which
> is a uint32, but in some places it is using "int", which can lead to an
> overflow at some point.

int is nowadays is at least 32 bits, so using int in a loop that
iterates up to a uint32 value won't cause an overflow.
It's never good to mix data types.
int is signed integer type and can carry only half of the positive numbers that "unsigned int" can.

from c.h:
#ifndef HAVE_UINT8
typedef unsigned char uint8; /* == 8 bits */
typedef unsigned short uint16; /* == 16 bits */
typedef unsigned int uint32; /* == 32 bits */
#endif /* not HAVE_UINT8 */

However, the
fix iteself looks good because it unifies the loop variable types in
similar loops.
Yeah.
 

On the other hand, I'm not a fan of changing the signature of
smgr_zeroextend to use uint32. I don't think it improves things and
the other reason is that I don't like using unnatural integer types
unnecessarily in API parameter types.
But ExtendBufferedRelBy calls smgr_zeroextend and carries a uint32 value to int param.
smgr_zeroextend signature must be changed to work with any values from uint32.

ASnyway, the patch causes a type
inconsistency between smgr_zserextend and mdzeroextend.
Yeah, have more inconsistency.
extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
 BlockNumber blocknum, BlockNumber nblocks);

BlockNumber is what integer data type?


> I also take the opportunity to correct another oversight, regarding the
> commit dad50f6
> <https://github.com/postgres/postgres/commit/dad50f677c42de207168a3f08982ba23c9fc6720>
> ,
> for possible duplicate assignment.
> GetLocalBufferDescriptor was called twice.
>
> Taking advantage of this, I promoted a scope reduction for some variables,
> which I thought was opportune.

I like the scope reductions.
Yeah.
 

Regarding the duplicate assignment to existing_hdr, I prefer assigning
it in the definition line, but I don't have a strong opinion on this
matter.
Closer to where the variable is used is preferable if the assignment is not cheap.

regards,
Ranier Vilela

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

Предыдущее
От: Alvaro Herrera
Дата:
Сообщение: Re: Should we remove vacuum_defer_cleanup_age?
Следующее
От: David Rowley
Дата:
Сообщение: Re: Fix incorrect start up costs for WindowAgg paths (bug #17862)