Horton

Thread

  1. Replace magic numbers with strategy numbers for B-tree indexes

    Daniil Davydov <3danissimo@gmail.com> — 2025-06-30T03:21:07Z

    Hi,
I noticed that some asserts and cycles use magic numbers 1 and 0
instead of BTLessStrategyNumber and InvalidStrategy.
At the same time, the BTMaxStrategyNumber macro is used there.
I suggest using appropriate macros for 1 and 0 values.

Please, see attached patch (targeted on the master branch).

--
Best regards,
Daniil Davydov

  2. Re: Replace magic numbers with strategy numbers for B-tree indexes

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-06-30T03:25:45Z

    On Mon, Jun 30, 2025 at 8:51 AM Daniil Davydov <3danissimo@gmail.com> wrote:
>
> Hi,
> I noticed that some asserts and cycles use magic numbers 1 and 0
> instead of BTLessStrategyNumber and InvalidStrategy.
> At the same time, the BTMaxStrategyNumber macro is used there.
> I suggest using appropriate macros for 1 and 0 values.
>
> Please, see attached patch (targeted on the master branch).

IMHO, it makes sense to use macros when it's already present for
consistency.  So +1 or making this change and the attached patch LGTM


-- 
Regards,
Dilip Kumar
Google

  3. Re: Replace magic numbers with strategy numbers for B-tree indexes

    Peter Eisentraut <peter@eisentraut.org> — 2025-07-02T11:24:02Z

    On 30.06.25 05:21, Daniil Davydov wrote:
> Hi,
> I noticed that some asserts and cycles use magic numbers 1 and 0
> instead of BTLessStrategyNumber and InvalidStrategy.
> At the same time, the BTMaxStrategyNumber macro is used there.
> I suggest using appropriate macros for 1 and 0 values.

This code, both the original and your changes, make a lot of assumptions 
about the btree strategy numbers, such as that BTLessStrategyNumber is 
the smallest valid one, that InvalidStrategy is smaller than all of 
them, and that all numbers between the smallest and BTMaxStrategyNumber 
are assigned.

However, some of the code actually does require that, because it fills 
in array fields for consecutive strategy numbers.  So hiding that fact 
by changing 1 to BTLessStrategyNumber introduces more mystery.

I think if we want to abstract all that away, this would need a deeper 
approach somehow.

  4. Re: Replace magic numbers with strategy numbers for B-tree indexes

    Daniil Davydov <3danissimo@gmail.com> — 2025-07-03T07:50:59Z

    Hi,

On Wed, Jul 2, 2025 at 6:24 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 30.06.25 05:21, Daniil Davydov wrote:
> > Hi,
> > I noticed that some asserts and cycles use magic numbers 1 and 0
> > instead of BTLessStrategyNumber and InvalidStrategy.
> > At the same time, the BTMaxStrategyNumber macro is used there.
> > I suggest using appropriate macros for 1 and 0 values.
>
> This code, both the original and your changes, make a lot of assumptions
> about the btree strategy numbers, such as that BTLessStrategyNumber is
> the smallest valid one, that InvalidStrategy is smaller than all of
> them, and that all numbers between the smallest and BTMaxStrategyNumber
> are assigned.
>
> However, some of the code actually does require that, because it fills
> in array fields for consecutive strategy numbers.  So hiding that fact
> by changing 1 to BTLessStrategyNumber introduces more mystery.
>

Thanks for looking into it!

OK, I can agree that the assumption that InvalidStrategy has the
smallest value is a bit too rough.

But BTLessStrategyNumber and BTMaxStrategyNumber literally say that
these are the min/max numbers.
Thus, assertions like "strategynum >= BTLessStrategyNumber" makes much
more sense than "strategynum >= 1"
(especially when the comment says something like "Check that only
allowed strategy numbers exist") and it is easier to maintain.

The same goes for cycles like [BTLessStrategyNumber;
BTMaxStrategyNumber] and [1; BTMaxStrategyNumber].
All arrays working with strategy numbers are initializing with
BTMaxStrategyNumber elements, so we cannot get any error here.
And if we init an array with length = BTMaxStrategyNumber, we must
assume that all numbers are assigned.
Otherwise, I don't understand why we should have "holes" in the numbering?

I still think that we should get rid of magic numbers.
As a compromise, I'm not replacing 0 with Invalid in the second
version of the patch.

What do you think?

--
Best regards,
Daniil Davydov

  5. Re: Replace magic numbers with strategy numbers for B-tree indexes

    Nikita Malakhov <hukutoc@gmail.com> — 2025-09-01T08:27:28Z

    Hi Daniil!

Please correct if I'm wrong, but it seems Peter had another approach in
mind -
magic numbers in separate macros could be easily replaced with enums and
validation functions, which would make code more readable and less
'magical'.
Please check the POC patch in attach.
I've made this just for BT strategies macros and touched only 2 source files
to make a correct but simple example.

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

  6. Re: Replace magic numbers with strategy numbers for B-tree indexes

    Daniil Davydov <3danissimo@gmail.com> — 2025-09-01T14:04:04Z

    Hi,

On Mon, Sep 1, 2025 at 3:27 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
>
> Please correct if I'm wrong, but it seems Peter had another approach in mind -
> magic numbers in separate macros could be easily replaced with enums and
> validation functions, which would make code more readable and less 'magical'.
> Please check the POC patch in attach.
> I've made this just for BT strategies macros and touched only 2 source files
> to make a correct but simple example.
>

I don't think that we can just create different enums for each index strategies.
We have (for example) ScanKey functionality, which can work with different
indexes (and such a functions has a uint16 argument for strategy number).

Or are you talking about a single huge enum for all index types? I don't
mind trying to do something like this, but I'm not sure how
"beautiful" it will be.

--
Best regards,
Daniil Davydov

  7. Re: Replace magic numbers with strategy numbers for B-tree indexes

    Michael Paquier <michael@paquier.xyz> — 2025-09-01T23:51:21Z

    On Mon, Sep 01, 2025 at 09:04:04PM +0700, Daniil Davydov wrote:
> I don't think that we can just create different enums for each index strategies.
> We have (for example) ScanKey functionality, which can work with different
> indexes (and such a functions has a uint16 argument for strategy number).
> 
> Or are you talking about a single huge enum for all index types? I don't
> mind trying to do something like this, but I'm not sure how
> "beautiful" it will be.

+typedef enum BTStrategy
+{
+	BTInvalidStrategy,
+	BTLessStrategy,
+	BTLessEqualStrategy,
+	BTEqualStrategy,
+	BTGreaterEqualStrategy,
+	BTGreaterStrategy,
+	BTNumOfStrategies
+} BTStrategy;
[...]
-	List	   *btree_clauses[BTMaxStrategyNumber + 1],
+	List	   *btree_clauses[BTNumOfStrategies],

Isn't that where you'd want to introduce a separate #define to track
the maximum number in the enum?  Adding the total number inside
BTStrategy would be wrong IMO.  Anyway, the advantage of an enum is
also to be able to initialize the first value, with the next one
following suit.  With most of the code using the strategy numbers in
for loops, we are not taking advantage of an enum structure, which is
relevant for example to find paths with switch/case without default,
where compilers would warn about missing values.  A second one would
be type enforcement in dedicated APIs, and we already have
StrategyNumber for this job in ScanKeyInit(), for example.

+/* Static inline function check */
+static inline bool BTStrategyIsValidFunc(uint16 strat)
+{
+	switch(strat)

Note that this is used nowhere :)
--
Michael