Skip to content

Commit 17ce344

Browse files
alvherrealvherre
committed
BRIN: be more strict about required support procs
With improperly defined operator classes, it's possible to get a Postgres crash because we'd try to invoke a procedure that doesn't exist. This is because the code is being a bit too trusting that the opclass is correctly defined. Add some ereport(ERROR)s for cases where mandatory support procedures are not defined, transforming the crashes into errors. The particular case that was reported is an incomplete opclass in PostGIS. Backpatch all the way down to 13. Reported-by: Tobias Wendorff <tobias.wendorff@tu-dortmund.de> Diagnosed-by: David Rowley <dgrowleyml@gmail.com> Reviewed-by: Tomas Vondra <tomas@vondra.me> Discussion: https://postgr.es/m/fb6d9a35-6c8e-4869-af80-0a4944a793a4@tu-dortmund.de
1 parent d35d32d commit 17ce344

File tree

3 files changed

+28
-41
lines changed

3 files changed

+28
-41
lines changed

‎src/backend/access/brin/brin_bloom.c‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,6 @@ typedef struct BloomOpaque
438438
* consistency. We may need additional procs in the future.
439439
*/
440440
FmgrInfo extra_procinfos[BLOOM_MAX_PROCNUMS];
441-
bool extra_proc_missing[BLOOM_MAX_PROCNUMS];
442441
} BloomOpaque;
443442

444443
static FmgrInfo *bloom_get_procinfo(BrinDesc *bdesc, uint16 attno,
@@ -714,27 +713,19 @@ bloom_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
714713
*/
715714
opaque = (BloomOpaque *) bdesc->bd_info[attno - 1]->oi_opaque;
716715

717-
/*
718-
* If we already searched for this proc and didn't find it, don't bother
719-
* searching again.
720-
*/
721-
if (opaque->extra_proc_missing[basenum])
722-
return NULL;
723-
724716
if (opaque->extra_procinfos[basenum].fn_oid == InvalidOid)
725717
{
726718
if (RegProcedureIsValid(index_getprocid(bdesc->bd_index, attno,
727719
procnum)))
728-
{
729720
fmgr_info_copy(&opaque->extra_procinfos[basenum],
730721
index_getprocinfo(bdesc->bd_index, attno, procnum),
731722
bdesc->bd_context);
732-
}
733723
else
734-
{
735-
opaque->extra_proc_missing[basenum] = true;
736-
return NULL;
737-
}
724+
ereport(ERROR,
725+
errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
726+
errmsg_internal("invalid opclass definition"),
727+
errdetail_internal("The operator class is missing support function %d for column %d.",
728+
procnum, attno));
738729
}
739730

740731
return &opaque->extra_procinfos[basenum];

‎src/backend/access/brin/brin_inclusion.c‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ typedef struct InclusionOpaque
8282
} InclusionOpaque;
8383

8484
static FmgrInfo *inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno,
85-
uint16 procnum);
85+
uint16 procnum, bool missing_ok);
8686
static FmgrInfo *inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno,
8787
Oid subtype, uint16 strategynum);
8888

@@ -179,7 +179,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
179179
* new value for emptiness; if it returns true, we need to set the
180180
* "contains empty" flag in the element (unless already set).
181181
*/
182-
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_EMPTY);
182+
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_EMPTY, true);
183183
if (finfo != NULL && DatumGetBool(FunctionCall1Coll(finfo, colloid, newval)))
184184
{
185185
if (!DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]))
@@ -195,7 +195,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
195195
PG_RETURN_BOOL(true);
196196

197197
/* Check if the new value is already contained. */
198-
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_CONTAINS);
198+
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_CONTAINS, true);
199199
if (finfo != NULL &&
200200
DatumGetBool(FunctionCall2Coll(finfo, colloid,
201201
column->bv_values[INCLUSION_UNION],
@@ -210,7 +210,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
210210
* it's not going to be used any longer. However, the BRIN framework
211211
* doesn't allow for the value not being present. Improve someday.
212212
*/
213-
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE);
213+
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE, true);
214214
if (finfo != NULL &&
215215
!DatumGetBool(FunctionCall2Coll(finfo, colloid,
216216
column->bv_values[INCLUSION_UNION],
@@ -221,8 +221,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
221221
}
222222

223223
/* Finally, merge the new value to the existing union. */
224-
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
225-
Assert(finfo != NULL);
224+
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE, false);
226225
result = FunctionCall2Coll(finfo, colloid,
227226
column->bv_values[INCLUSION_UNION], newval);
228227
if (!attr->attbyval &&
@@ -506,7 +505,7 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
506505
}
507506

508507
/* Check if A and B are mergeable; if not, mark A unmergeable. */
509-
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE);
508+
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE, true);
510509
if (finfo != NULL &&
511510
!DatumGetBool(FunctionCall2Coll(finfo, colloid,
512511
col_a->bv_values[INCLUSION_UNION],
@@ -517,8 +516,7 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
517516
}
518517

519518
/* Finally, merge B to A. */
520-
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
521-
Assert(finfo != NULL);
519+
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE, false);
522520
result = FunctionCall2Coll(finfo, colloid,
523521
col_a->bv_values[INCLUSION_UNION],
524522
col_b->bv_values[INCLUSION_UNION]);
@@ -539,10 +537,12 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
539537
* Cache and return inclusion opclass support procedure
540538
*
541539
* Return the procedure corresponding to the given function support number
542-
* or null if it is not exists.
540+
* or null if it is not exists. If missing_ok is true and the procedure
541+
* isn't set up for this opclass, return NULL instead of raising an error.
543542
*/
544543
static FmgrInfo *
545-
inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
544+
inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum,
545+
bool missing_ok)
546546
{
547547
InclusionOpaque *opaque;
548548
uint16 basenum = procnum - PROCNUM_BASE;
@@ -564,13 +564,18 @@ inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
564564
{
565565
if (RegProcedureIsValid(index_getprocid(bdesc->bd_index, attno,
566566
procnum)))
567-
{
568567
fmgr_info_copy(&opaque->extra_procinfos[basenum],
569568
index_getprocinfo(bdesc->bd_index, attno, procnum),
570569
bdesc->bd_context);
571-
}
572570
else
573571
{
572+
if (!missing_ok)
573+
ereport(ERROR,
574+
errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
575+
errmsg_internal("invalid opclass definition"),
576+
errdetail_internal("The operator class is missing support function %d for column %d.",
577+
procnum, attno));
578+
574579
opaque->extra_proc_missing[basenum] = true;
575580
return NULL;
576581
}

‎src/backend/access/brin/brin_minmax_multi.c‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,6 @@
111111
typedef struct MinmaxMultiOpaque
112112
{
113113
FmgrInfo extra_procinfos[MINMAX_MAX_PROCNUMS];
114-
bool extra_proc_missing[MINMAX_MAX_PROCNUMS];
115114
Oid cached_subtype;
116115
FmgrInfo strategy_procinfos[BTMaxStrategyNumber];
117116
} MinmaxMultiOpaque;
@@ -2872,27 +2871,19 @@ minmax_multi_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
28722871
*/
28732872
opaque = (MinmaxMultiOpaque *) bdesc->bd_info[attno - 1]->oi_opaque;
28742873

2875-
/*
2876-
* If we already searched for this proc and didn't find it, don't bother
2877-
* searching again.
2878-
*/
2879-
if (opaque->extra_proc_missing[basenum])
2880-
return NULL;
2881-
28822874
if (opaque->extra_procinfos[basenum].fn_oid == InvalidOid)
28832875
{
28842876
if (RegProcedureIsValid(index_getprocid(bdesc->bd_index, attno,
28852877
procnum)))
2886-
{
28872878
fmgr_info_copy(&opaque->extra_procinfos[basenum],
28882879
index_getprocinfo(bdesc->bd_index, attno, procnum),
28892880
bdesc->bd_context);
2890-
}
28912881
else
2892-
{
2893-
opaque->extra_proc_missing[basenum] = true;
2894-
return NULL;
2895-
}
2882+
ereport(ERROR,
2883+
errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2884+
errmsg_internal("invalid opclass definition"),
2885+
errdetail_internal("The operator class is missing support function %d for column %d.",
2886+
procnum, attno));
28962887
}
28972888

28982889
return &opaque->extra_procinfos[basenum];

0 commit comments

Comments
 (0)