Re: Signed vs. Unsigned (some)

Em sex., 2 de jul. de 2021 às 13:29, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
escreveu:

> On 2021-Jul-02, Justin Pryzby wrote:
>
> > On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote:
>
> > > The use of InvalidBlockNumber with vac_update_relstats() looks a bit
> fishy
> > > to me. We are using in the same call 0 as the default for
> > > num_all_visible_pages, and we generally elsewhere also use 0 as the
> starting
> > > value for relpages, so it's not clear to me why it should be -1 or
> > > InvalidBlockNumber here. I'd rather leave it "slightly wrong" for now
> so it
> > > can be checked again.
>
> > |commit 0e69f705cc1a3df273b38c9883fb5765991e04fe
> > |Author: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> > |Date: Fri Apr 9 11:29:08 2021 -0400
> > |
> > | Set pg_class.reltuples for partitioned tables
> >
> > 3d35 also affects partitioned tables, and 0e69 appears to do the right
> thing by
> > preserving relpages=-1 during auto-analyze.
>
> I suppose the question is what is the value used for. BlockNumber is
> typedef'd uint32, an unsigned variable, so using -1 for it is quite
> fishy. The weird thing is that in vac_update_relstats we cast it to
> (int32) when storing it in the pg_class tuple, so that's quite fishy
> too.
>

> What we really want is for table_block_relation_estimate_size to work
> properly. What that does is get the signed-int32 value from pg_class
> and cast it back to BlockNumber. If that assignment gets -1 again, then
> it's all fine. I didn't test it.
>
It seems to me that it is happening, but it is risky to make comparisons
between different types.

1)
#define InvalidBlockNumber 0xFFFFFFFF

int main()
{
unsigned int num_pages;
int rel_pages;

num_pages = -1;
rel_pages = (int) num_pages;
printf("num_pages = %u\n", num_pages);
printf("rel_pages = %d\n", rel_pages);
printf("(num_pages == InvalidBlockNumber) => %u\n", (num_pages ==
InvalidBlockNumber));
printf("(rel_pages == InvalidBlockNumber) => %u\n", (rel_pages ==
InvalidBlockNumber));
}

num_pages = 4294967295
rel_pages = -1
(num_pages == InvalidBlockNumber) => 1
(rel_pages == InvalidBlockNumber) => 1 /* 17:68: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare] */

If num_pages is promoted to uint64 and rel_pages to int64:
2)
#define InvalidBlockNumber 0xFFFFFFFF

int main()
{
unsigned long int num_pages;
long int rel_pages;

num_pages = -1;
rel_pages = (int) num_pages;
printf("num_pages = %lu\n", num_pages);
printf("rel_pages = %ld\n", rel_pages);
printf("(num_pages == InvalidBlockNumber) => %u\n", (num_pages ==
InvalidBlockNumber));
printf("(rel_pages == InvalidBlockNumber) => %u\n", (rel_pages ==
InvalidBlockNumber));
}

num_pages = 18446744073709551615
rel_pages = -1
(num_pages == InvalidBlockNumber) => 0
(rel_pages == InvalidBlockNumber) => 0 /* 17:68: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare] */

As Kyotaro said:
"they might be different if we forget to widen the constant
when widening the types"

regards,
Ranier Vilela