Обсуждение: BlockNumber fixes
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);
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
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, Pennsylvania19026
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
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));
}