Re: Signed vs. Unsigned (some)

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.

I think changing the vac_update_relstats call I added in 0e69f705cc1a to
InvalidBlockNumber is fine. I didn't verify any other places.

I think storing BlockNumber values >= 2^31 in an int32 catalog column is
asking for trouble. We'll have to fix that at some point.

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/