Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 21a4e4a

Browse files
committed
Fix BRIN free space computations
A bug in the original free space computation made it possible to return a page which wasn't actually able to fit the item. Since the insertion code isn't prepared to deal with PageAddItem failing, a PANIC resulted ("failed to add BRIN tuple [to new page]"). Add a macro to encapsulate the correct computation, and use it in brin_getinsertbuffer's callers before calling that routine, to raise an early error. I became aware of the possiblity of a problem in this area while working on ccc4c07. There's no archived discussion about it, but it's easy to reproduce a problem in the unpatched code with something like CREATE TABLE t (a text); CREATE INDEX ti ON t USING brin (a) WITH (pages_per_range=1); for length in `seq 8000 8196` do psql -f - <<EOF TRUNCATE TABLE t; INSERT INTO t VALUES ('z'), (repeat('a', $length)); EOF done Backpatch to 9.5, where BRIN was introduced.
1 parent 531d21b commit 21a4e4a

File tree

1 file changed

+43
-20
lines changed

1 file changed

+43
-20
lines changed

src/backend/access/brin/brin_pageops.c

Lines changed: 43 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,16 @@
2323
#include "utils/rel.h"
2424

2525

26+
/*
27+
* Maximum size of an entry in a BRIN_PAGETYPE_REGULAR page. We can tolerate
28+
* a single item per page, unlike other index AMs.
29+
*/
30+
#define BrinMaxItemSize \
31+
MAXALIGN_DOWN(BLCKSZ - \
32+
(MAXALIGN(SizeOfPageHeaderData + \
33+
sizeof(ItemIdData)) + \
34+
MAXALIGN(sizeof(BrinSpecialSpace))))
35+
2636
static Buffer brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
2737
bool *extended);
2838
static Size br_page_get_freespace(Page page);
@@ -58,6 +68,18 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
5868

5969
Assert(newsz == MAXALIGN(newsz));
6070

71+
/* If the item is oversized, don't bother. */
72+
if (newsz > BrinMaxItemSize)
73+
{
74+
ereport(ERROR,
75+
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
76+
errmsg("index row size %lu exceeds maximum %lu for index \"%s\"",
77+
(unsigned long) newsz,
78+
(unsigned long) BrinMaxItemSize,
79+
RelationGetRelationName(idxrel))));
80+
return false; /* keep compiler quiet */
81+
}
82+
6183
/* make sure the revmap is long enough to contain the entry we need */
6284
brinRevmapExtend(revmap, heapBlk);
6385

@@ -332,6 +354,18 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
332354

333355
Assert(itemsz == MAXALIGN(itemsz));
334356

357+
/* If the item is oversized, don't even bother. */
358+
if (itemsz > BrinMaxItemSize)
359+
{
360+
ereport(ERROR,
361+
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
362+
errmsg("index row size %lu exceeds maximum %lu for index \"%s\"",
363+
(unsigned long) itemsz,
364+
(unsigned long) BrinMaxItemSize,
365+
RelationGetRelationName(idxrel))));
366+
return InvalidOffsetNumber; /* keep compiler quiet */
367+
}
368+
335369
/* Make sure the revmap is long enough to contain the entry we need */
336370
brinRevmapExtend(revmap, heapBlk);
337371

@@ -360,9 +394,9 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
360394
*/
361395
if (!BufferIsValid(*buffer))
362396
{
363-
*buffer = brin_getinsertbuffer(idxrel, InvalidBuffer, itemsz, &extended);
364-
Assert(BufferIsValid(*buffer));
365-
Assert(extended || br_page_get_freespace(BufferGetPage(*buffer)) >= itemsz);
397+
do
398+
*buffer = brin_getinsertbuffer(idxrel, InvalidBuffer, itemsz, &extended);
399+
while (!BufferIsValid(*buffer));
366400
}
367401
else
368402
extended = false;
@@ -610,8 +644,9 @@ brin_page_cleanup(Relation idxrel, Buffer buf)
610644

611645
/*
612646
* Return a pinned and exclusively locked buffer which can be used to insert an
613-
* index item of size itemsz. If oldbuf is a valid buffer, it is also locked
614-
* (in an order determined to avoid deadlocks.)
647+
* index item of size itemsz (caller must ensure not to request sizes
648+
* impossible to fulfill). If oldbuf is a valid buffer, it is also locked (in
649+
* an order determined to avoid deadlocks.)
615650
*
616651
* If we find that the old page is no longer a regular index page (because
617652
* of a revmap extension), the old buffer is unlocked and we return
@@ -636,20 +671,8 @@ brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
636671
Page page;
637672
int freespace;
638673

639-
/*
640-
* If the item is oversized, don't bother. We have another, more precise
641-
* check below.
642-
*/
643-
if (itemsz > BLCKSZ - sizeof(BrinSpecialSpace))
644-
{
645-
ereport(ERROR,
646-
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
647-
errmsg("index row size %lu exceeds maximum %lu for index \"%s\"",
648-
(unsigned long) itemsz,
649-
(unsigned long) BLCKSZ - sizeof(BrinSpecialSpace),
650-
RelationGetRelationName(irel))));
651-
return InvalidBuffer; /* keep compiler quiet */
652-
}
674+
/* callers must have checked */
675+
Assert(itemsz <= BrinMaxItemSize);
653676

654677
*extended = false;
655678

@@ -759,7 +782,7 @@ brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
759782
* page that has since been repurposed for the revmap.)
760783
*/
761784
freespace = *extended ?
762-
BLCKSZ - sizeof(BrinSpecialSpace) : br_page_get_freespace(page);
785+
BrinMaxItemSize : br_page_get_freespace(page);
763786
if (freespace >= itemsz)
764787
{
765788
RelationSetTargetBlock(irel, BufferGetBlockNumber(buf));

0 commit comments

Comments
 (0)