Re: Bufmgr possible overflow

Em qua., 12 de abr. de 2023 às 22:29, Kyotaro Horiguchi <
horikyota(dot)ntt(at)gmail(dot)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(dot)vf(at)gmail(dot)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