Обсуждение: Re: Replace magic numbers with strategy numbers for B-tree indexes

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.

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

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