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

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

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