We have the TODO item:

  * Make sure all block numbers are unsigned to increase maximum table size

I did some research on this and generated the following patch.  I didn't
find much in the way of problems except two vacuum.c fields that should
probably be BlockNumber.  freespace.c also has a numPages field in
FSMRelation that is int.  Should that be BlockNumber?

I am holding the patch until I get some feedback.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.230
diff -c -r1.230 vacuum.c
*** src/backend/commands/vacuum.c	20 Jun 2002 20:29:27 -0000	1.230
--- src/backend/commands/vacuum.c	8 Jul 2002 03:44:12 -0000
***************
*** 60,68 ****
  
  typedef struct VacPageListData
  {
! 	BlockNumber empty_end_pages;	/* Number of "empty" end-pages */
! 	int			num_pages;		/* Number of pages in pagedesc */
! 	int			num_allocated_pages;	/* Number of allocated pages in
  										 * pagedesc */
  	VacPage    *pagedesc;		/* Descriptions of pages */
  } VacPageListData;
--- 60,68 ----
  
  typedef struct VacPageListData
  {
! 	BlockNumber empty_end_pages;		/* Number of "empty" end-pages */
! 	BlockNumber	num_pages;				/* Number of pages in pagedesc */
! 	BlockNumber	num_allocated_pages;	/* Number of allocated pages in
  										 * pagedesc */
  	VacPage    *pagedesc;		/* Descriptions of pages */
  } VacPageListData;
***************
*** 988,994 ****
  				usable_free_size;
  	Size		min_tlen = MaxTupleSize;
  	Size		max_tlen = 0;
- 	int			i;
  	bool		do_shrinking = true;
  	VTupleLink	vtlinks = (VTupleLink) palloc(100 * sizeof(VTupleLinkData));
  	int			num_vtlinks = 0;
--- 988,993 ----
***************
*** 1285,1291 ****
  	 */
  	if (do_shrinking)
  	{
! 		Assert((BlockNumber) fraged_pages->num_pages >= empty_end_pages);
  		fraged_pages->num_pages -= empty_end_pages;
  		usable_free_size = 0;
  		for (i = 0; i < fraged_pages->num_pages; i++)
--- 1284,1292 ----
  	 */
  	if (do_shrinking)
  	{
! 		BlockNumber	i;
! 
! 		Assert(fraged_pages->num_pages >= empty_end_pages);
  		fraged_pages->num_pages -= empty_end_pages;
  		usable_free_size = 0;
  		for (i = 0; i < fraged_pages->num_pages; i++)
***************
*** 1412,1418 ****
  
  	Nvacpagelist.num_pages = 0;
  	num_fraged_pages = fraged_pages->num_pages;
! 	Assert((BlockNumber) vacuum_pages->num_pages >= vacuum_pages->empty_end_pages);
  	vacuumed_pages = vacuum_pages->num_pages - vacuum_pages->empty_end_pages;
  	if (vacuumed_pages > 0)
  	{
--- 1413,1419 ----
  
  	Nvacpagelist.num_pages = 0;
  	num_fraged_pages = fraged_pages->num_pages;
! 	Assert(vacuum_pages->num_pages >= vacuum_pages->empty_end_pages);
  	vacuumed_pages = vacuum_pages->num_pages - vacuum_pages->empty_end_pages;
  	if (vacuumed_pages > 0)
  	{
***************
*** 2332,2342 ****
  			WriteBuffer(buf);
  		}
  
! 		/* now - free new list of reaped pages */
! 		curpage = Nvacpagelist.pagedesc;
! 		for (i = 0; i < Nvacpagelist.num_pages; i++, curpage++)
! 			pfree(*curpage);
! 		pfree(Nvacpagelist.pagedesc);
  	}
  
  	/*
--- 2333,2346 ----
  			WriteBuffer(buf);
  		}
  
! 		{
! 			BlockNumber i;
! 			/* now - free new list of reaped pages */
! 			curpage = Nvacpagelist.pagedesc;
! 			for (i = 0; i < Nvacpagelist.num_pages; i++, curpage++)
! 				pfree(*curpage);
! 			pfree(Nvacpagelist.pagedesc);
! 		}
  	}
  
  	/*
***************
*** 2381,2393 ****
  	Buffer		buf;
  	VacPage    *vacpage;
  	BlockNumber relblocks;
! 	int			nblocks;
  	int			i;
  
  	nblocks = vacuum_pages->num_pages;
  	nblocks -= vacuum_pages->empty_end_pages;	/* nothing to do with them */
  
! 	for (i = 0, vacpage = vacuum_pages->pagedesc; i < nblocks; i++, vacpage++)
  	{
  		CHECK_FOR_INTERRUPTS();
  		if ((*vacpage)->offsets_free > 0)
--- 2385,2398 ----
  	Buffer		buf;
  	VacPage    *vacpage;
  	BlockNumber relblocks;
! 	BlockNumber	nblocks;
! 	BlockNumber	blks;
  	int			i;
  
  	nblocks = vacuum_pages->num_pages;
  	nblocks -= vacuum_pages->empty_end_pages;	/* nothing to do with them */
  
! 	for (blks = 0, vacpage = vacuum_pages->pagedesc; blks < nblocks; blks++, vacpage++)
  	{
  		CHECK_FOR_INTERRUPTS();
  		if ((*vacpage)->offsets_free > 0)
***************
*** 2636,2643 ****
  vac_update_fsm(Relation onerel, VacPageList fraged_pages,
  			   BlockNumber rel_pages)
  {
! 	int			nPages = fraged_pages->num_pages;
! 	int			i;
  	BlockNumber *pages;
  	Size	   *spaceAvail;
  
--- 2641,2648 ----
  vac_update_fsm(Relation onerel, VacPageList fraged_pages,
  			   BlockNumber rel_pages)
  {
! 	BlockNumber	nPages = fraged_pages->num_pages;
! 	BlockNumber	i;
  	BlockNumber *pages;
  	Size	   *spaceAvail;
  
Index: src/backend/storage/freespace/freespace.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/freespace/freespace.c,v
retrieving revision 1.12
diff -c -r1.12 freespace.c
*** src/backend/storage/freespace/freespace.c	20 Jun 2002 20:29:34 -0000	1.12
--- src/backend/storage/freespace/freespace.c	8 Jul 2002 03:44:13 -0000
***************
*** 371,382 ****
  MultiRecordFreeSpace(RelFileNode *rel,
  					 BlockNumber minPage,
  					 BlockNumber maxPage,
! 					 int nPages,
  					 BlockNumber *pages,
  					 Size *spaceAvail)
  {
  	FSMRelation *fsmrel;
! 	int			i;
  
  	LWLockAcquire(FreeSpaceLock, LW_EXCLUSIVE);
  	fsmrel = lookup_fsm_rel(rel);
--- 371,382 ----
  MultiRecordFreeSpace(RelFileNode *rel,
  					 BlockNumber minPage,
  					 BlockNumber maxPage,
! 					 BlockNumber nPages,
  					 BlockNumber *pages,
  					 Size *spaceAvail)
  {
  	FSMRelation *fsmrel;
! 	BlockNumber	i;
  
  	LWLockAcquire(FreeSpaceLock, LW_EXCLUSIVE);
  	fsmrel = lookup_fsm_rel(rel);
Index: src/backend/utils/adt/tid.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/tid.c,v
retrieving revision 1.31
diff -c -r1.31 tid.c
*** src/backend/utils/adt/tid.c	20 Jun 2002 20:29:38 -0000	1.31
--- src/backend/utils/adt/tid.c	8 Jul 2002 03:44:13 -0000
***************
*** 87,93 ****
  	blockNumber = BlockIdGetBlockNumber(blockId);
  	offsetNumber = itemPtr->ip_posid;
  
! 	sprintf(buf, "(%d,%d)", (int) blockNumber, (int) offsetNumber);
  
  	PG_RETURN_CSTRING(pstrdup(buf));
  }
--- 87,93 ----
  	blockNumber = BlockIdGetBlockNumber(blockId);
  	offsetNumber = itemPtr->ip_posid;
  
! 	sprintf(buf, "(%u,%d)", blockNumber, (int) offsetNumber);
  
  	PG_RETURN_CSTRING(pstrdup(buf));
  }
***************
*** 140,146 ****
   *		correspond to the CTID of a base relation.
   */
  static Datum
! currtid_for_view(Relation viewrel, ItemPointer tid) 
  {
  	TupleDesc	att = RelationGetDescr(viewrel);
  	RuleLock	*rulelock;
--- 140,146 ----
   *		correspond to the CTID of a base relation.
   */
  static Datum
! currtid_for_view(Relation viewrel, ItemPointer tid)
  {
  	TupleDesc	att = RelationGetDescr(viewrel);
  	RuleLock	*rulelock;
Index: src/include/storage/freespace.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/storage/freespace.h,v
retrieving revision 1.7
diff -c -r1.7 freespace.h
*** src/include/storage/freespace.h	20 Jun 2002 20:29:52 -0000	1.7
--- src/include/storage/freespace.h	8 Jul 2002 03:44:14 -0000
***************
*** 38,44 ****
  extern void MultiRecordFreeSpace(RelFileNode *rel,
  					 BlockNumber minPage,
  					 BlockNumber maxPage,
! 					 int nPages,
  					 BlockNumber *pages,
  					 Size *spaceAvail);
  extern void FreeSpaceMapForgetRel(RelFileNode *rel);
--- 38,44 ----
  extern void MultiRecordFreeSpace(RelFileNode *rel,
  					 BlockNumber minPage,
  					 BlockNumber maxPage,
! 					 BlockNumber nPages,
  					 BlockNumber *pages,
  					 Size *spaceAvail);
  extern void FreeSpaceMapForgetRel(RelFileNode *rel);
Sun, 7 Jul 2002 23:51:30 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
BlockNumber fixes
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I did some research on this and generated the following patch.  I didn't
> find much in the way of problems except two vacuum.c fields that should
> probably be BlockNumber.  freespace.c also has a numPages field in
> FSMRelation that is int.  Should that be BlockNumber?

Not necessary, since the freespace map will never be large enough to
overflow a signed int (it wouldn't fit in the address space if it were).
I think that your changes in vacuum.c are probably unnecessary for the
same reason.  I am generally wary of changing values from signed to
unsigned without close analysis of how they are used --- did you look
at *every* comparison involving these fields?  How about arithmetic
that might compute a negative result?

			regards, tom lane



Mon, 08 Jul 2002 10:19:08 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: BlockNumber fixes 
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I did some research on this and generated the following patch.  I didn't
> > find much in the way of problems except two vacuum.c fields that should
> > probably be BlockNumber.  freespace.c also has a numPages field in
> > FSMRelation that is int.  Should that be BlockNumber?
> 
> Not necessary, since the freespace map will never be large enough to
> overflow a signed int (it wouldn't fit in the address space if it were).
> I think that your changes in vacuum.c are probably unnecessary for the
> same reason.  I am generally wary of changing values from signed to
> unsigned without close analysis of how they are used --- did you look
> at *every* comparison involving these fields?  How about arithmetic
> that might compute a negative result?

The only computation I saw was:

  vacuumed_pages = vacuum_pages->num_pages - vacuum_pages->empty_end_pages;

vacuumed_pages is signed, the others are unsigned.  However, we print
these values as %u so there is a certain confusion there.

If you say it isn't a problem here, I will just mark the item as done
and that we are handling the block numbers correctly.  The only other
unusual case I saw was tid outputing block number as %d and not %u.  Is
that OK?

    sprintf(buf, "(%d,%d)", (int) blockNumber, (int) offsetNumber);

tidin uses atoi:

    blockNumber = (BlockNumber) atoi(coord[0]);

so at least it is consistent.  ;-)  Doesn't seem right, however.

Also, pg_class.relpages is an int.  We don't have unsigned int columns.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tue, 16 Jul 2002 01:58:19 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: BlockNumber fixes
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The only other
> unusual case I saw was tid outputing block number as %d and not %u.  Is
> that OK?

Seems like it should use %u.  The input side might be wrong too.

> Also, pg_class.relpages is an int.  We don't have unsigned int columns.

Yeah.  I had a todo item to look at all the uses of relpages and make
sure they were being casted to unsigned ...

			regards, tom lane

Tue, 16 Jul 2002 02:25:27 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: BlockNumber fixes 
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > The only other
> > unusual case I saw was tid outputing block number as %d and not %u.  Is
> > that OK?
> 
> Seems like it should use %u.  The input side might be wrong too.
> 

OK, fixed.  Patch attached.  There was also some confusion in the code
of how strtol returns its end pointer as always non-NULL:
	
	test=> insert into x values ('(1,2)');
	INSERT 16591 1
	test=> insert into x values ('(1000000000,2)');
	INSERT 16592 1
	test=> insert into x values ('(3000000000,2)');
	INSERT 16593 1
	test=> select * from x;
	       y        
	----------------
	          (1,2)
	 (1000000000,2)
	 (3000000000,2)
	(3 rows)
	
	test=> insert into x values ('(5000000000,2)');
	ERROR:  tidin: invalid value.
	test=> insert into x values ('(3000000000,200000)');
	ERROR:  tidin: invalid value.
	test=> insert into x values ('(3000000000,20000)');
	INSERT 16595 1

> > Also, pg_class.relpages is an int.  We don't have unsigned int columns.
> 
> Yeah.  I had a todo item to look at all the uses of relpages and make
> sure they were being casted to unsigned ...

They all look OK to me.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Index: src/backend/utils/adt/numutils.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/numutils.c,v
retrieving revision 1.49
diff -c -r1.49 numutils.c
*** src/backend/utils/adt/numutils.c	20 Jun 2002 20:29:38 -0000	1.49
--- src/backend/utils/adt/numutils.c	16 Jul 2002 17:51:06 -0000
***************
*** 46,52 ****
  pg_atoi(char *s, int size, int c)
  {
  	long		l = 0;
! 	char	   *badp = (char *) NULL;
  
  	Assert(s);
  
--- 46,52 ----
  pg_atoi(char *s, int size, int c)
  {
  	long		l = 0;
! 	char	   *badp;
  
  	Assert(s);
  
***************
*** 71,77 ****
  	 */
  	if (errno && errno != EINVAL)
  		elog(ERROR, "pg_atoi: error reading \"%s\": %m", s);
! 	if (badp && *badp && (*badp != c))
  		elog(ERROR, "pg_atoi: error in \"%s\": can\'t parse \"%s\"", s, badp);
  
  	switch (size)
--- 71,77 ----
  	 */
  	if (errno && errno != EINVAL)
  		elog(ERROR, "pg_atoi: error reading \"%s\": %m", s);
! 	if (*badp && *badp != c)
  		elog(ERROR, "pg_atoi: error in \"%s\": can\'t parse \"%s\"", s, badp);
  
  	switch (size)
Index: src/backend/utils/adt/tid.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/tid.c,v
retrieving revision 1.31
diff -c -r1.31 tid.c
*** src/backend/utils/adt/tid.c	20 Jun 2002 20:29:38 -0000	1.31
--- src/backend/utils/adt/tid.c	16 Jul 2002 17:51:06 -0000
***************
*** 18,23 ****
--- 18,27 ----
  
  #include "postgres.h"
  
+ #include <errno.h>
+ #include <math.h>
+ #include <limits.h>
+ 
  #include "access/heapam.h"
  #include "catalog/namespace.h"
  #include "utils/builtins.h"
***************
*** 47,52 ****
--- 51,58 ----
  	ItemPointer result;
  	BlockNumber blockNumber;
  	OffsetNumber offsetNumber;
+ 	char	   *badp;
+ 	int			hold_offset;
  
  	for (i = 0, p = str; *p && i < NTIDARGS && *p != RDELIM; p++)
  		if (*p == DELIM || (*p == LDELIM && !i))
***************
*** 55,62 ****
  	if (i < NTIDARGS)
  		elog(ERROR, "invalid tid format: '%s'", str);
  
! 	blockNumber = (BlockNumber) atoi(coord[0]);
! 	offsetNumber = (OffsetNumber) atoi(coord[1]);
  
  	result = (ItemPointer) palloc(sizeof(ItemPointerData));
  
--- 61,76 ----
  	if (i < NTIDARGS)
  		elog(ERROR, "invalid tid format: '%s'", str);
  
! 	errno = 0;
! 	blockNumber = strtoul(coord[0], &badp, 10);
! 	if (errno || *badp != DELIM)
! 		elog(ERROR, "tidin: invalid value.");
! 
! 	hold_offset = strtol(coord[1], &badp, 10);
! 	if (errno || *badp != RDELIM ||
! 		hold_offset > USHRT_MAX || hold_offset < 0)
! 		elog(ERROR, "tidin: invalid value.");
! 	offsetNumber = hold_offset;
  
  	result = (ItemPointer) palloc(sizeof(ItemPointerData));
  
***************
*** 87,93 ****
  	blockNumber = BlockIdGetBlockNumber(blockId);
  	offsetNumber = itemPtr->ip_posid;
  
! 	sprintf(buf, "(%d,%d)", (int) blockNumber, (int) offsetNumber);
  
  	PG_RETURN_CSTRING(pstrdup(buf));
  }
--- 101,107 ----
  	blockNumber = BlockIdGetBlockNumber(blockId);
  	offsetNumber = itemPtr->ip_posid;
  
! 	sprintf(buf, "(%u,%u)", blockNumber, offsetNumber);
  
  	PG_RETURN_CSTRING(pstrdup(buf));
  }
Tue, 16 Jul 2002 13:54:29 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: BlockNumber fixes