Hi,
On Wed, Jul 2, 2025 at 6:24 PM Peter Eisentraut <pe...@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
From 7f2fdff65795f1da4fbc923d9d8a2ed1f3277d4b Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davy...@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
