pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Álvaro Herrera <alvherre@alvh.no-ip.org>
Subject: [PATCH v1] BRIN: be stricter about required support procs
Date: Wed, 12 Feb 2025 11:55:06 +0100
---
src/backend/access/brin/brin_bloom.c | 20 ++++++++++++---
src/backend/access/brin/brin_inclusion.c | 27 +++++++++++++--------
src/backend/access/brin/brin_minmax_multi.c | 23 ++++++++++++++----
3 files changed, 51 insertions(+), 19 deletions(-)
inline patch 1: src/backend/access/brin/brin_bloom.c, src/backend/access/brin/brin_inclusion.c, src/backend/access/brin/brin_minmax_multi.c (+51/-19 lines) download inline-1.patch diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c
index 40e1f303a0a..011562853cf 100644
--- a/src/backend/access/brin/brin_bloom.c
+++ b/src/backend/access/brin/brin_bloom.c
@@ -442,7 +442,7 @@ typedef struct BloomOpaque
} BloomOpaque;
static FmgrInfo *bloom_get_procinfo(BrinDesc *bdesc, uint16 attno,
- uint16 procnum);
+ uint16 procnum, bool missing_ok);
Datum
@@ -574,7 +574,7 @@ brin_bloom_add_value(PG_FUNCTION_ARGS)
* Compute the hash of the new value, using the supplied hash function,
* and then add the hash value to the bloom filter.
*/
- hashFn = bloom_get_procinfo(bdesc, attno, PROCNUM_HASH);
+ hashFn = bloom_get_procinfo(bdesc, attno, PROCNUM_HASH, false);
hashValue = DatumGetUInt32(FunctionCall1Coll(hashFn, colloid, newval));
@@ -634,7 +634,7 @@ brin_bloom_consistent(PG_FUNCTION_ARGS)
* We want to return the current page range if the bloom
* filter seems to contain the value.
*/
- finfo = bloom_get_procinfo(bdesc, attno, PROCNUM_HASH);
+ finfo = bloom_get_procinfo(bdesc, attno, PROCNUM_HASH, false);
hashValue = DatumGetUInt32(FunctionCall1Coll(finfo, colloid, value));
matches &= bloom_contains_value(filter, hashValue);
@@ -703,7 +703,7 @@ brin_bloom_union(PG_FUNCTION_ARGS)
* or null if it does not exist.
*/
static FmgrInfo *
-bloom_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
+bloom_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum, bool missing_ok)
{
BloomOpaque *opaque;
uint16 basenum = procnum - PROCNUM_BASE;
@@ -732,6 +732,18 @@ bloom_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
}
else
{
+ /*
+ * XXX like minmax_multi_get_procinfo, it seems there are no cases
+ * of optional support procs in brin_bloom.c
+ */
+ if (!missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg_internal("invalid opclass definition"),
+ errdetail_internal("opclass %u is missing support function %d",
+ InvalidOid, /* FIXME */
+ procnum)));
+
opaque->extra_proc_missing[basenum] = true;
return NULL;
}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index ef2247347f9..5a6565a709a 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -82,7 +82,7 @@ typedef struct InclusionOpaque
} InclusionOpaque;
static FmgrInfo *inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno,
- uint16 procnum);
+ uint16 procnum, bool missing_ok);
static FmgrInfo *inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno,
Oid subtype, uint16 strategynum);
@@ -179,7 +179,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
* new value for emptiness; if it returns true, we need to set the
* "contains empty" flag in the element (unless already set).
*/
- finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_EMPTY);
+ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_EMPTY, true);
if (finfo != NULL && DatumGetBool(FunctionCall1Coll(finfo, colloid, newval)))
{
if (!DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]))
@@ -195,7 +195,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(true);
/* Check if the new value is already contained. */
- finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_CONTAINS);
+ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_CONTAINS, true);
if (finfo != NULL &&
DatumGetBool(FunctionCall2Coll(finfo, colloid,
column->bv_values[INCLUSION_UNION],
@@ -210,7 +210,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
* it's not going to be used any longer. However, the BRIN framework
* doesn't allow for the value not being present. Improve someday.
*/
- finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE);
+ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE, true);
if (finfo != NULL &&
!DatumGetBool(FunctionCall2Coll(finfo, colloid,
column->bv_values[INCLUSION_UNION],
@@ -221,8 +221,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
}
/* Finally, merge the new value to the existing union. */
- finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
- Assert(finfo != NULL);
+ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE, false);
result = FunctionCall2Coll(finfo, colloid,
column->bv_values[INCLUSION_UNION], newval);
if (!attr->attbyval &&
@@ -506,7 +505,7 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
}
/* Check if A and B are mergeable; if not, mark A unmergeable. */
- finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE);
+ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE, true);
if (finfo != NULL &&
!DatumGetBool(FunctionCall2Coll(finfo, colloid,
col_a->bv_values[INCLUSION_UNION],
@@ -517,8 +516,7 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
}
/* Finally, merge B to A. */
- finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
- Assert(finfo != NULL);
+ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE, false);
result = FunctionCall2Coll(finfo, colloid,
col_a->bv_values[INCLUSION_UNION],
col_b->bv_values[INCLUSION_UNION]);
@@ -542,7 +540,8 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
* or null if it is not exists.
*/
static FmgrInfo *
-inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
+inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum,
+ bool missing_ok)
{
InclusionOpaque *opaque;
uint16 basenum = procnum - PROCNUM_BASE;
@@ -571,6 +570,14 @@ inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
}
else
{
+ if (!missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg_internal("invalid opclass definition"),
+ errdetail_internal("opclass %u is missing support function %d",
+ InvalidOid, /* FIXME */
+ procnum)));
+
opaque->extra_proc_missing[basenum] = true;
return NULL;
}
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 88214720bff..245b72b58a0 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -256,7 +256,7 @@ typedef struct DistanceValue
/* Cache for support and strategy procedures. */
static FmgrInfo *minmax_multi_get_procinfo(BrinDesc *bdesc, uint16 attno,
- uint16 procnum);
+ uint16 procnum, bool missing_ok);
static FmgrInfo *minmax_multi_get_strategy_procinfo(BrinDesc *bdesc,
uint16 attno, Oid subtype,
@@ -1663,7 +1663,7 @@ ensure_free_space_in_buffer(BrinDesc *bdesc, Oid colloid,
AssertCheckExpandedRanges(bdesc, colloid, attno, attr, eranges, neranges);
/* and we'll also need the 'distance' procedure */
- distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE);
+ distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE, false);
/* build array of gap distances and sort them in ascending order */
distances = build_distances(distanceFn, colloid, eranges, neranges);
@@ -1814,7 +1814,8 @@ compactify_ranges(BrinDesc *bdesc, Ranges *ranges, int max_values)
BTLessStrategyNumber);
/* and we'll also need the 'distance' procedure */
- distanceFn = minmax_multi_get_procinfo(bdesc, ranges->attno, PROCNUM_DISTANCE);
+ distanceFn = minmax_multi_get_procinfo(bdesc, ranges->attno, PROCNUM_DISTANCE,
+ false);
/*
* The distanceFn calls (which may internally call e.g. numeric_le) may
@@ -2820,7 +2821,7 @@ brin_minmax_multi_union(PG_FUNCTION_ARGS)
*/
/* build array of gap distances and sort them in ascending order */
- distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE);
+ distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE, false);
distances = build_distances(distanceFn, colloid, eranges, neranges);
/*
@@ -2861,7 +2862,8 @@ brin_minmax_multi_union(PG_FUNCTION_ARGS)
* or null if it does not exist.
*/
static FmgrInfo *
-minmax_multi_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
+minmax_multi_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum,
+ bool missing_ok)
{
MinmaxMultiOpaque *opaque;
uint16 basenum = procnum - PROCNUM_BASE;
@@ -2890,6 +2892,17 @@ minmax_multi_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
}
else
{
+ /*
+ * FIXME it seems that in minmax_multi we don't have any cases
+ * of optional support procs, so perhaps we should remove this.
+ */
+ if (!missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg_internal("invalid opclass definition"),
+ errdetail_internal("opclass %u is missing support function %d",
+ InvalidOid, /* FIXME */
+ procnum)));
opaque->extra_proc_missing[basenum] = true;
return NULL;
}
--
2.39.5
Message-ID: <no-message-id-25390@localhost>
Permalink: ../no-message-id-25390@localhost/
Also on: postgresql.org/message-id/no-message-id-25390@localhost
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: alvherre@alvh.no-ip.org
Subject: Re: [PATCH v1] BRIN: be stricter about required support procs
In-Reply-To: <no-message-id-25390@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox