Обсуждение: Mis-use of type BlockNumber?

Hi,

While reviewing [1], I noticed several cases where BlockNumber seems to be misused.

Although BlockNumber is currently underlying defined as uint32, it has a special meaning. For example:
```
#define InvalidBlockNumber ((BlockNumber) 0xFFFFFFFF)
#define MaxBlockNumber ((BlockNumber) 0xFFFFFFFE)
```
So my understanding is that BlockNumber should only be used to identify a block.

However, I saw several places where variables of type BlockNumber are actually used as counts. For example:
```
typedef struct LVRelState
{

    BlockNumber blkno;   <== correct usage

    BlockNumber rel_pages; /* total number of pages */  <== mis-use
```
In this example, rel_pages is actually a count. In theory it could just be an int (or some other count type).

Another example:
```
static void
system_time_samplescangetsamplesize(PlannerInfo *root,
                                    RelOptInfo *baserel,
                                    List *paramexprs,
                                    BlockNumber *pages,
                                    double *tuples)
```
Here the parameter pages is also a count indicating the number of pages.

So I want to confirm whether my understanding is correct that these are misuses of BlockNumber. If so, would it be
wortha cleanup patch to fix such cases? 

[1] https://postgr.es/m/81DAFACF-7D55-4A84-ACB0-0425D1669DB4@gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> On Mar 6, 2026, at 15:56, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>
>
>
> On Thu, 5 Mar 2026 at 19:26, Chao Li <li.evan.chao@gmail.com> wrote:
> Hi,
>
> While reviewing [1], I noticed several cases where BlockNumber seems to be misused.
>
> Although BlockNumber is currently underlying defined as uint32, it has a special meaning. For example:
> ```
> #define InvalidBlockNumber ((BlockNumber) 0xFFFFFFFF)
> #define MaxBlockNumber ((BlockNumber) 0xFFFFFFFE)
> ```
> So my understanding is that BlockNumber should only be used to identify a block.
> However, I saw several places where variables of type BlockNumber are actually used as counts. For example:
> ```
> typedef struct LVRelState
> {
>
>     BlockNumber blkno;   <== correct usage
>
>     BlockNumber rel_pages; /* total number of pages */  <== mis-use
> ```
> Actually,  InvalidBlockNumber and MaxBlockNumber are special values, not the BlockNumber itself, it is as you said
underlyinguint32. 
> AFAIk these types for typedef are done so that we understand them in a particular context and not just use them as
anyother uint32. Increases the code readability. 
> There are other such examples too like Bucket in Hash.

Hi Rafia,

Thanks for sharing your opinion. However, I am not fully convinced.

When we use a type, we usually don’t need to care about its underlying actual type. If one day BlockNumber were
redefinedas a structure type, then all usages where it is treated as a counter would break, right? While such a change
maybe unlikely, it is not impossible. For example, there is an ongoing discussion [1] proposing to change Datum into a
structure,while Datum is currently defined as uint64_t. 

To me, this kind of misuse also hurts readability. As I mentioned earlier, I noticed the issue while reviewing a patch:
Iwas confused by the definition of a struct field until I read the implementation and realized that it was actually
beingused as a counter. 

[1] https://postgr.es/m/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On 2026-03-06 11:25:45 +0800, Chao Li wrote:
> While reviewing [1], I noticed several cases where BlockNumber seems to be misused.
> 
> Although BlockNumber is currently underlying defined as uint32, it has a special meaning. For example:
> ```
> #define InvalidBlockNumber ((BlockNumber) 0xFFFFFFFF)
> #define MaxBlockNumber ((BlockNumber) 0xFFFFFFFE)
> ```
> So my understanding is that BlockNumber should only be used to identify a block.
> 
> However, I saw several places where variables of type BlockNumber are actually used as counts. For example:
> ```
> typedef struct LVRelState
> {
> 
>     BlockNumber blkno;   <== correct usage
> 
>     BlockNumber rel_pages; /* total number of pages */  <== mis-use
> ```
> In this example, rel_pages is actually a count. In theory it could just be an int (or some other count type).

> Another example:
> ```
> static void
> system_time_samplescangetsamplesize(PlannerInfo *root,
>                                     RelOptInfo *baserel,
>                                     List *paramexprs,
>                                     BlockNumber *pages,
>                                     double *tuples)
> ```
> Here the parameter pages is also a count indicating the number of pages.
> 
> So I want to confirm whether my understanding is correct that these are misuses of BlockNumber. If so, would it be
wortha cleanup patch to fix such cases?
 

I don't love that we use BlockNumber this way either, but it is fairly
widespread and of long standing.  There is a lot of code in postgres, if you
just go and scour it to find stuff you individually don't like, you will be
busy for a long time (writing the "fixes") and we will be busy for a long time
(dealing with the influx of patches).  Just because some rarely changing
aspect of the code does something mildly ugly doesn't mean you have to
immediately send patches about it.  Sending and processing patches takes time
and energy.

Greetings,

Andres Freund

> On Mar 6, 2026, at 21:16, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2026-03-06 11:25:45 +0800, Chao Li wrote:
>> While reviewing [1], I noticed several cases where BlockNumber seems to be misused.
>>
>> Although BlockNumber is currently underlying defined as uint32, it has a special meaning. For example:
>> ```
>> #define InvalidBlockNumber ((BlockNumber) 0xFFFFFFFF)
>> #define MaxBlockNumber ((BlockNumber) 0xFFFFFFFE)
>> ```
>> So my understanding is that BlockNumber should only be used to identify a block.
>>
>> However, I saw several places where variables of type BlockNumber are actually used as counts. For example:
>> ```
>> typedef struct LVRelState
>> {
>>
>>    BlockNumber blkno;   <== correct usage
>>
>>    BlockNumber rel_pages; /* total number of pages */  <== mis-use
>> ```
>> In this example, rel_pages is actually a count. In theory it could just be an int (or some other count type).
>
>> Another example:
>> ```
>> static void
>> system_time_samplescangetsamplesize(PlannerInfo *root,
>> RelOptInfo *baserel,
>> List *paramexprs,
>> BlockNumber *pages,
>> double *tuples)
>> ```
>> Here the parameter pages is also a count indicating the number of pages.
>>
>> So I want to confirm whether my understanding is correct that these are misuses of BlockNumber. If so, would it be
wortha cleanup patch to fix such cases? 
>
> I don't love that we use BlockNumber this way either, but it is fairly
> widespread and of long standing.  There is a lot of code in postgres, if you
> just go and scour it to find stuff you individually don't like, you will be
> busy for a long time (writing the "fixes") and we will be busy for a long time
> (dealing with the influx of patches).  Just because some rarely changing
> aspect of the code does something mildly ugly doesn't mean you have to
> immediately send patches about it.  Sending and processing patches takes time
> and energy.
>

That’s a fair point and that’s why I didn’t go ahead to work on the patch.

But I think we should avoid to introduce such usages in new code. In other words, while reviewing patches, we should
raisecomments for such mis-usages. Is my understanding correct? 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, Mar 6, 2026 at 9:11 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> But I think we should avoid to introduce such usages in new code. In other words, while reviewing patches, we should
raisecomments for such mis-usages. Is my understanding correct? 

I also don't like how BlockNumber is used this way. However, if you
introduce a new counter and use uint32 instead when all surrounding
counters are BlockNumber, it sticks out as different and is confusing.
If you go and change all instances of BlockNumber being used this way
because you added one new counter, that's a lot of churn. If we decide
to change BlockNumber to a struct, we'll have a good reason and be
willing to pay the price of the churn at that point. I'm not a
stickler for consistency and in fact think it's okay to have some
things inconsistent if it means introducing new patterns that are
better. But having a single counter in a struct of the same unit as a
bunch of existing counters be a different datatype doesn't seem worth
it to me.

- Melanie

Hi,

On 2026-03-06 10:22:10 -0500, Melanie Plageman wrote:
> On Fri, Mar 6, 2026 at 9:11 AM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> > But I think we should avoid to introduce such usages in new code. In other words, while reviewing patches, we
shouldraise comments for such mis-usages. Is my understanding correct?
 
> 
> I also don't like how BlockNumber is used this way. However, if you
> introduce a new counter and use uint32 instead when all surrounding
> counters are BlockNumber, it sticks out as different and is confusing.

FWIW, I don't think uint32 would be a good choice. I think we're eventually
going to have to allow larger relations and a lot of counters in uint32 would
make that a good bit harder than right now, where it's BlockNumber.  So you'd
have to introduce a new BlockCounter type. At which point you ... can just use
BlockNumber, or uint64 (and waste space for now).

Greetings,

Andres Freund

> On Mar 7, 2026, at 03:31, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2026-03-06 10:22:10 -0500, Melanie Plageman wrote:
>> On Fri, Mar 6, 2026 at 9:11 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>>
>>> But I think we should avoid to introduce such usages in new code. In other words, while reviewing patches, we
shouldraise comments for such mis-usages. Is my understanding correct? 
>>
>> I also don't like how BlockNumber is used this way. However, if you
>> introduce a new counter and use uint32 instead when all surrounding
>> counters are BlockNumber, it sticks out as different and is confusing.
>
> FWIW, I don't think uint32 would be a good choice. I think we're eventually
> going to have to allow larger relations and a lot of counters in uint32 would
> make that a good bit harder than right now, where it's BlockNumber.  So you'd
> have to introduce a new BlockCounter type. At which point you ... can just use
> BlockNumber, or uint64 (and waste space for now).
>

Thanks for sharing your thoughts.

IMHO, the range of a counter does not have to match the range of BlockNumber, that really depends on the context. Even
ifI were to do such a refactoring, I was never thinking of replacing BlockNumber with uint32. To me, the right choice
shouldbe an ordinary numeric type selected according to the semantics of the value and the required range. 

Similarly, there are many places that count the number of tuples, but PG does not have a dedicated type for that, e.g.
somethinglike TupleCount. That suggests, at least to me, that the question here is simply which ordinary numeric type
bestmatches the meaning of the value, rather than reusing BlockNumber by default. 

All in all, I think there is at least agreement that using BlockNumber for a block counter is not semantically ideal,
andthat doing a wholesale refactoring is probably not worthwhile. Where we seem to differ is that I think we should
avoidintroducing new cases like this, and perhaps fix existing ones opportunistically when a patch is already touching
therelevant code. It sounds like the preference is to leave it as-is for now. 

I have noted this, and perhaps raise it again if there is a better opportunity in the future.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/