Replace magic numbers with strategy numbers for B-tree indexes
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
Attachments:
v1-0001-Get-rid-of-magic-numbers.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Get-rid-of-magic-numbers.patchDownload
From 67d2125240021f3f0f25d56630dad1d70940b40d Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Mon, 30 Jun 2025 10:10:24 +0700
Subject: [PATCH v1] Get rid of magic numbers
---
src/backend/access/brin/brin_minmax.c | 4 ++--
src/backend/access/brin/brin_minmax_multi.c | 4 ++--
src/backend/access/nbtree/nbtpreprocesskeys.c | 4 ++--
src/backend/access/nbtree/nbtvalidate.c | 2 +-
src/backend/partitioning/partprune.c | 4 +++-
5 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index d21ab3a668c..19e953a4fd4 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -263,7 +263,7 @@ minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
MinmaxOpaque *opaque;
- Assert(strategynum >= 1 &&
+ Assert(strategynum >= BTLessStrategyNumber &&
strategynum <= BTMaxStrategyNumber);
opaque = (MinmaxOpaque *) bdesc->bd_info[attno - 1]->oi_opaque;
@@ -277,7 +277,7 @@ minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
uint16 i;
- for (i = 1; i <= BTMaxStrategyNumber; i++)
+ for (i = BTLessStrategyNumber; i <= BTMaxStrategyNumber; i++)
opaque->strategy_procinfos[i - 1].fn_oid = InvalidOid;
opaque->cached_subtype = subtype;
}
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 0d1507a2a36..f1dc40b10e7 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -2901,7 +2901,7 @@ minmax_multi_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
MinmaxMultiOpaque *opaque;
- Assert(strategynum >= 1 &&
+ Assert(strategynum >= BTLessStrategyNumber &&
strategynum <= BTMaxStrategyNumber);
opaque = (MinmaxMultiOpaque *) bdesc->bd_info[attno - 1]->oi_opaque;
@@ -2915,7 +2915,7 @@ minmax_multi_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
uint16 i;
- for (i = 1; i <= BTMaxStrategyNumber; i++)
+ for (i = BTLessStrategyNumber; i <= BTMaxStrategyNumber; i++)
opaque->strategy_procinfos[i - 1].fn_oid = InvalidOid;
opaque->cached_subtype = subtype;
}
diff --git a/src/backend/access/nbtree/nbtpreprocesskeys.c b/src/backend/access/nbtree/nbtpreprocesskeys.c
index a136e4bbfdf..b030833e370 100644
--- a/src/backend/access/nbtree/nbtpreprocesskeys.c
+++ b/src/backend/access/nbtree/nbtpreprocesskeys.c
@@ -360,7 +360,7 @@ _bt_preprocess_keys(IndexScanDesc scan)
Assert(OidIsValid(orderproc->fn_oid));
}
- for (j = BTMaxStrategyNumber; --j >= 0;)
+ for (j = BTMaxStrategyNumber; --j >= InvalidStrategy;)
{
ScanKey chk = xform[j].inkey;
@@ -438,7 +438,7 @@ _bt_preprocess_keys(IndexScanDesc scan)
* sufficient to form an unbroken series of "=" constraints on all
* attrs prior to the attr from the final scan->keyData[] key.
*/
- for (j = BTMaxStrategyNumber; --j >= 0;)
+ for (j = BTMaxStrategyNumber; --j >= InvalidStrategy;)
{
if (xform[j].inkey)
{
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index 817ad358f0c..03729892cde 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -140,7 +140,7 @@ btvalidate(Oid opclassoid)
Form_pg_amop oprform = (Form_pg_amop) GETSTRUCT(oprtup);
/* Check that only allowed strategy numbers exist */
- if (oprform->amopstrategy < 1 ||
+ if (oprform->amopstrategy < BTLessStrategyNumber ||
oprform->amopstrategy > BTMaxStrategyNumber)
{
ereport(INFO,
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 48a35f763e9..0d9ac307b17 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1526,7 +1526,9 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
* combinations of expressions of different keys, which
* get_steps_using_prefix takes care of for us.
*/
- for (strat = 1; strat <= BTMaxStrategyNumber; strat++)
+ for (strat = BTLessStrategyNumber;
+ strat <= BTMaxStrategyNumber;
+ strat++)
{
foreach(lc, btree_clauses[strat])
{
--
2.43.0
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
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 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
Attachments:
v2-0001-Get-rid-of-magic-numbers.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Get-rid-of-magic-numbers.patchDownload
From 7f2fdff65795f1da4fbc923d9d8a2ed1f3277d4b Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Thu, 3 Jul 2025 14:38:20 +0700
Subject: [PATCH v2] Get rid of magic numbers
---
src/backend/access/brin/brin_minmax.c | 4 ++--
src/backend/access/brin/brin_minmax_multi.c | 4 ++--
src/backend/access/nbtree/nbtvalidate.c | 2 +-
src/backend/partitioning/partprune.c | 4 +++-
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index d21ab3a668c..19e953a4fd4 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -263,7 +263,7 @@ minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
MinmaxOpaque *opaque;
- Assert(strategynum >= 1 &&
+ Assert(strategynum >= BTLessStrategyNumber &&
strategynum <= BTMaxStrategyNumber);
opaque = (MinmaxOpaque *) bdesc->bd_info[attno - 1]->oi_opaque;
@@ -277,7 +277,7 @@ minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
uint16 i;
- for (i = 1; i <= BTMaxStrategyNumber; i++)
+ for (i = BTLessStrategyNumber; i <= BTMaxStrategyNumber; i++)
opaque->strategy_procinfos[i - 1].fn_oid = InvalidOid;
opaque->cached_subtype = subtype;
}
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 0d1507a2a36..f1dc40b10e7 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -2901,7 +2901,7 @@ minmax_multi_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
MinmaxMultiOpaque *opaque;
- Assert(strategynum >= 1 &&
+ Assert(strategynum >= BTLessStrategyNumber &&
strategynum <= BTMaxStrategyNumber);
opaque = (MinmaxMultiOpaque *) bdesc->bd_info[attno - 1]->oi_opaque;
@@ -2915,7 +2915,7 @@ minmax_multi_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
{
uint16 i;
- for (i = 1; i <= BTMaxStrategyNumber; i++)
+ for (i = BTLessStrategyNumber; i <= BTMaxStrategyNumber; i++)
opaque->strategy_procinfos[i - 1].fn_oid = InvalidOid;
opaque->cached_subtype = subtype;
}
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index 817ad358f0c..03729892cde 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -140,7 +140,7 @@ btvalidate(Oid opclassoid)
Form_pg_amop oprform = (Form_pg_amop) GETSTRUCT(oprtup);
/* Check that only allowed strategy numbers exist */
- if (oprform->amopstrategy < 1 ||
+ if (oprform->amopstrategy < BTLessStrategyNumber ||
oprform->amopstrategy > BTMaxStrategyNumber)
{
ereport(INFO,
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 48a35f763e9..0d9ac307b17 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1526,7 +1526,9 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
* combinations of expressions of different keys, which
* get_steps_using_prefix takes care of for us.
*/
- for (strat = 1; strat <= BTMaxStrategyNumber; strat++)
+ for (strat = BTLessStrategyNumber;
+ strat <= BTMaxStrategyNumber;
+ strat++)
{
foreach(lc, btree_clauses[strat])
{
--
2.43.0
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/
Attachments:
v3-0001-poc-get-rid-of-magic-numbers.patchapplication/octet-stream; name=v3-0001-poc-get-rid-of-magic-numbers.patchDownload
From 7d0440cba17bfa22ef8a2e790c0208e6d963d57f Mon Sep 17 00:00:00 2001
From: Nikita Malakhov <n.malakhov@postgrespro.ru>
Date: Sun, 31 Aug 2025 23:14:22 +0300
Subject: [PATCH] Introduce enum for BTStrategy with validation macro/function
instead of each value defined by macro.
---
src/backend/access/nbtree/nbtvalidate.c | 3 +--
src/backend/partitioning/partprune.c | 21 ++++++++++++----
src/include/access/stratnum.h | 33 +++++++++++++++++++++++++
3 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index 817ad358f0c..b863475599f 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -140,8 +140,7 @@ btvalidate(Oid opclassoid)
Form_pg_amop oprform = (Form_pg_amop) GETSTRUCT(oprtup);
/* Check that only allowed strategy numbers exist */
- if (oprform->amopstrategy < 1 ||
- oprform->amopstrategy > BTMaxStrategyNumber)
+ if (!BTStrategyIsValid(oprform->amopstrategy))
{
ereport(INFO,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 48a35f763e9..4792c614c3a 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1414,7 +1414,7 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
{
PartitionScheme part_scheme = context->rel->part_scheme;
List *opsteps = NIL;
- List *btree_clauses[BTMaxStrategyNumber + 1],
+ List *btree_clauses[BTNumOfStrategies],
*hash_clauses[HTMaxStrategyNumber + 1];
int i;
ListCell *lc;
@@ -1509,11 +1509,22 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
case PARTITION_STRATEGY_LIST:
case PARTITION_STRATEGY_RANGE:
{
- List *eq_clauses = btree_clauses[BTEqualStrategyNumber];
- List *le_clauses = btree_clauses[BTLessEqualStrategyNumber];
- List *ge_clauses = btree_clauses[BTGreaterEqualStrategyNumber];
+ List *eq_clauses = btree_clauses[BTEqualStrategy];
+ List *le_clauses = btree_clauses[BTLessEqualStrategy];
+ List *ge_clauses = btree_clauses[BTGreaterEqualStrategy];
int strat;
+/*
+typedef enum
+{
+ BTInvalidStrategy,
+ BTLessStrategy,
+ BTLessEqualStrategy,
+ BTEqualStrategy,
+ BTGreaterEqualStrategy,
+ BTGreaterStrategy
+} BTStrategy;
+*/
/*
* For each clause under consideration for a given strategy,
* we collect expressions from clauses for earlier keys, whose
@@ -1526,7 +1537,7 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
* combinations of expressions of different keys, which
* get_steps_using_prefix takes care of for us.
*/
- for (strat = 1; strat <= BTMaxStrategyNumber; strat++)
+ for (strat = BTLessStrategy; strat < BTNumOfStrategies; strat++)
{
foreach(lc, btree_clauses[strat])
{
diff --git a/src/include/access/stratnum.h b/src/include/access/stratnum.h
index ee036d2b62f..6c8042e1285 100644
--- a/src/include/access/stratnum.h
+++ b/src/include/access/stratnum.h
@@ -23,6 +23,39 @@ typedef uint16 StrategyNumber;
#define InvalidStrategy ((StrategyNumber) 0)
+typedef enum BTStrategy
+{
+ BTInvalidStrategy,
+ BTLessStrategy,
+ BTLessEqualStrategy,
+ BTEqualStrategy,
+ BTGreaterEqualStrategy,
+ BTGreaterStrategy,
+ BTNumOfStrategies
+} BTStrategy;
+
+/* Macro check, like most such checks in Pg */
+#define BTStrategyIsValid(strat) \
+ ((bool) (strat == BTLessStrategy || strat == BTLessEqualStrategy || strat == BTEqualStrategy || strat == BTGreaterEqualStrategy || strat == BTGreaterStrategy))
+
+/* Static inline function check */
+static inline bool BTStrategyIsValidFunc(uint16 strat)
+{
+ switch(strat)
+ {
+ case BTLessStrategy:
+ case BTLessEqualStrategy:
+ case BTEqualStrategy:
+ case BTGreaterEqualStrategy:
+ case BTGreaterStrategy:
+ return true;
+ case BTInvalidStrategy:
+ case BTNumOfStrategies:
+ default:
+ return false;
+ }
+}
+
/*
* Strategy numbers for B-tree indexes.
*/
--
2.34.1
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