Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit747cef5

Browse files
committed
Fix another bug in parent page splitting during GiST index build.
Yet another bug in the ilk of commitsa7ee7c8 and741b884. In741b884, we took care to clear the memorized location of thedownlink when we split the parent page, because splitting the parentpage can move the downlink. But we missed that even *updating* a tupleon the parent can move it, because updating a tuple on a gist page isimplemented as a delete+insert, so the updated tuple gets moved to theend of the page.This commit fixes the bug in two different ways (belt and suspenders):1. Clear the downlink when we update a tuple on the parent page, even if it's not split. This the same approach as in commitsa7ee7c8 and741b884. I also noticed that gistFindCorrectParent did not clear the 'downlinkoffnum' when it stepped to the right sibling. Fix that too, as it seems like a clear bug even though I haven't been able to find a test case to hit that.2. Change gistFindCorrectParent so that it treats 'downlinkoffnum' merely as a hint. It now always first checks if the downlink is still at that location, and if not, it scans the page like before. That's more robust if there are still more cases where we fail to clear 'downlinkoffnum' that we haven't yet uncovered. With this, it's no longer necessary to meticulously clear 'downlinkoffnum', so this makes the previous fixes unnecessary, but I didn't revert them because it still seems nice to clear it when we know that the downlink has moved.Also add the test case using the same test data that Alexanderposted. I tried to reduce it to a smaller test, and I also tried toreproduce this with different test data, but I was not able to, solet's just include what we have.Backpatch to v12, like the previous fixes.Reported-by: Alexander LakhinDiscussion:https://www.postgresql.org/message-id/18129-caca016eaf0c3702@postgresql.org
1 parent0c4d7a1 commit747cef5

File tree

3 files changed

+226
-80
lines changed

3 files changed

+226
-80
lines changed

‎contrib/intarray/expected/_int.out

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -857,4 +857,95 @@ SELECT count(*) from test__int WHERE a @@ '!20 & !21';
857857
6343
858858
(1 row)
859859

860+
DROP INDEX text_idx;
861+
-- Repeat the same queries with an extended data set. The data set is the
862+
-- same that we used before, except that each element in the array is
863+
-- repeated three times, offset by 1000 and 2000. For example, {1, 5}
864+
-- becomes {1, 1001, 2001, 5, 1005, 2005}.
865+
--
866+
-- That has proven to be unreasonably effective at exercising codepaths in
867+
-- core GiST code related to splitting parent pages, which is not covered by
868+
-- other tests. This is a bit out-of-place as the point is to test core GiST
869+
-- code rather than this extension, but there is no suitable GiST opclass in
870+
-- core that would reach the same codepaths.
871+
CREATE TABLE more__int AS SELECT
872+
-- Leave alone NULLs, empty arrays and the one row that we use to test
873+
-- equality
874+
CASE WHEN a IS NULL OR a = '{}' OR a = '{73,23,20}' THEN a ELSE
875+
(select array_agg(u) || array_agg(u + 1000) || array_agg(u + 2000) from (select unnest(a) u) x)
876+
END AS a, a as b
877+
FROM test__int;
878+
CREATE INDEX ON more__int using gist (a gist__int_ops(numranges = 252));
879+
SELECT count(*) from more__int WHERE a && '{23,50}';
880+
count
881+
-------
882+
403
883+
(1 row)
884+
885+
SELECT count(*) from more__int WHERE a @@ '23|50';
886+
count
887+
-------
888+
403
889+
(1 row)
890+
891+
SELECT count(*) from more__int WHERE a @> '{23,50}';
892+
count
893+
-------
894+
12
895+
(1 row)
896+
897+
SELECT count(*) from more__int WHERE a @@ '23&50';
898+
count
899+
-------
900+
12
901+
(1 row)
902+
903+
SELECT count(*) from more__int WHERE a @> '{20,23}';
904+
count
905+
-------
906+
12
907+
(1 row)
908+
909+
SELECT count(*) from more__int WHERE a <@ '{73,23,20}';
910+
count
911+
-------
912+
10
913+
(1 row)
914+
915+
SELECT count(*) from more__int WHERE a = '{73,23,20}';
916+
count
917+
-------
918+
1
919+
(1 row)
920+
921+
SELECT count(*) from more__int WHERE a @@ '50&68';
922+
count
923+
-------
924+
9
925+
(1 row)
926+
927+
SELECT count(*) from more__int WHERE a @> '{20,23}' or a @> '{50,68}';
928+
count
929+
-------
930+
21
931+
(1 row)
932+
933+
SELECT count(*) from more__int WHERE a @@ '(20&23)|(50&68)';
934+
count
935+
-------
936+
21
937+
(1 row)
938+
939+
SELECT count(*) from more__int WHERE a @@ '20 | !21';
940+
count
941+
-------
942+
6566
943+
(1 row)
944+
945+
SELECT count(*) from more__int WHERE a @@ '!20 & !21';
946+
count
947+
-------
948+
6343
949+
(1 row)
950+
860951
RESET enable_seqscan;

‎contrib/intarray/sql/_int.sql

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,4 +180,39 @@ SELECT count(*) from test__int WHERE a @@ '(20&23)|(50&68)';
180180
SELECTcount(*)from test__intWHERE a @@'20 | !21';
181181
SELECTcount(*)from test__intWHERE a @@'!20 & !21';
182182

183+
DROPINDEX text_idx;
184+
185+
-- Repeat the same queries with an extended data set. The data set is the
186+
-- same that we used before, except that each element in the array is
187+
-- repeated three times, offset by 1000 and 2000. For example, {1, 5}
188+
-- becomes {1, 1001, 2001, 5, 1005, 2005}.
189+
--
190+
-- That has proven to be unreasonably effective at exercising codepaths in
191+
-- core GiST code related to splitting parent pages, which is not covered by
192+
-- other tests. This is a bit out-of-place as the point is to test core GiST
193+
-- code rather than this extension, but there is no suitable GiST opclass in
194+
-- core that would reach the same codepaths.
195+
CREATETABLEmore__intASSELECT
196+
-- Leave alone NULLs, empty arrays and the one row that we use to test
197+
-- equality
198+
CASE WHEN a ISNULLOR a='{}'OR a='{73,23,20}' THEN a ELSE
199+
(select array_agg(u)|| array_agg(u+1000)|| array_agg(u+2000)from (select unnest(a) u) x)
200+
ENDAS a, aas b
201+
FROM test__int;
202+
CREATEINDEXON more__int using gist (a gist__int_ops(numranges=252));
203+
204+
SELECTcount(*)from more__intWHERE a &&'{23,50}';
205+
SELECTcount(*)from more__intWHERE a @@'23|50';
206+
SELECTcount(*)from more__intWHERE a @>'{23,50}';
207+
SELECTcount(*)from more__intWHERE a @@'23&50';
208+
SELECTcount(*)from more__intWHERE a @>'{20,23}';
209+
SELECTcount(*)from more__intWHERE a<@'{73,23,20}';
210+
SELECTcount(*)from more__intWHERE a='{73,23,20}';
211+
SELECTcount(*)from more__intWHERE a @@'50&68';
212+
SELECTcount(*)from more__intWHERE a @>'{20,23}'or a @>'{50,68}';
213+
SELECTcount(*)from more__intWHERE a @@'(20&23)|(50&68)';
214+
SELECTcount(*)from more__intWHERE a @@'20 | !21';
215+
SELECTcount(*)from more__intWHERE a @@'!20 & !21';
216+
217+
183218
RESET enable_seqscan;

‎src/backend/access/gist/gist.c

Lines changed: 100 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,95 +1017,114 @@ gistFindPath(Relation r, BlockNumber child, OffsetNumber *downlinkoffnum)
10171017
* remain so at exit, but it might not be the same page anymore.
10181018
*/
10191019
staticvoid
1020-
gistFindCorrectParent(Relationr,GISTInsertStack*child)
1020+
gistFindCorrectParent(Relationr,GISTInsertStack*child,boolis_build)
10211021
{
10221022
GISTInsertStack*parent=child->parent;
1023+
ItemIdiid;
1024+
IndexTupleidxtuple;
1025+
OffsetNumbermaxoff;
1026+
GISTInsertStack*ptr;
10231027

10241028
gistcheckpage(r,parent->buffer);
10251029
parent->page= (Page)BufferGetPage(parent->buffer);
1030+
maxoff=PageGetMaxOffsetNumber(parent->page);
10261031

1027-
/* here we don't need to distinguish between split and page update */
1028-
if (child->downlinkoffnum==InvalidOffsetNumber||
1029-
parent->lsn!=PageGetLSN(parent->page))
1032+
/* Check if the downlink is still where it was before */
1033+
if (child->downlinkoffnum!=InvalidOffsetNumber&&child->downlinkoffnum <=maxoff)
10301034
{
1031-
/* parent is changed, look child in right links until found */
1032-
OffsetNumberi,
1033-
maxoff;
1034-
ItemIdiid;
1035-
IndexTupleidxtuple;
1036-
GISTInsertStack*ptr;
1035+
iid=PageGetItemId(parent->page,child->downlinkoffnum);
1036+
idxtuple= (IndexTuple)PageGetItem(parent->page,iid);
1037+
if (ItemPointerGetBlockNumber(&(idxtuple->t_tid))==child->blkno)
1038+
return;/* still there */
1039+
}
10371040

1038-
while (true)
1039-
{
1040-
maxoff=PageGetMaxOffsetNumber(parent->page);
1041-
for (i=FirstOffsetNumber;i <=maxoff;i=OffsetNumberNext(i))
1042-
{
1043-
iid=PageGetItemId(parent->page,i);
1044-
idxtuple= (IndexTuple)PageGetItem(parent->page,iid);
1045-
if (ItemPointerGetBlockNumber(&(idxtuple->t_tid))==child->blkno)
1046-
{
1047-
/* yes!!, found */
1048-
child->downlinkoffnum=i;
1049-
return;
1050-
}
1051-
}
1041+
/*
1042+
* The page has changed since we looked. During normal operation, every
1043+
* update of a page changes its LSN, so the LSN we memorized should have
1044+
* changed too. During index build, however, we don't WAL-log the changes
1045+
* until we have built the index, so the LSN doesn't change. There is no
1046+
* concurrent activity during index build, but we might have changed the
1047+
* parent ourselves.
1048+
*/
1049+
Assert(parent->lsn!=PageGetLSN(parent->page)||is_build);
1050+
1051+
/*
1052+
* Scan the page to re-find the downlink. If the page was split, it might
1053+
* have moved to a different page, so follow the right links until we find
1054+
* it.
1055+
*/
1056+
while (true)
1057+
{
1058+
OffsetNumberi;
10521059

1053-
parent->blkno=GistPageGetOpaque(parent->page)->rightlink;
1054-
UnlockReleaseBuffer(parent->buffer);
1055-
if (parent->blkno==InvalidBlockNumber)
1060+
maxoff=PageGetMaxOffsetNumber(parent->page);
1061+
for (i=FirstOffsetNumber;i <=maxoff;i=OffsetNumberNext(i))
1062+
{
1063+
iid=PageGetItemId(parent->page,i);
1064+
idxtuple= (IndexTuple)PageGetItem(parent->page,iid);
1065+
if (ItemPointerGetBlockNumber(&(idxtuple->t_tid))==child->blkno)
10561066
{
1057-
/*
1058-
* End of chain and still didn't find parent. It's a very-very
1059-
* rare situation when root splitted.
1060-
*/
1061-
break;
1067+
/* yes!!, found */
1068+
child->downlinkoffnum=i;
1069+
return;
10621070
}
1063-
parent->buffer=ReadBuffer(r,parent->blkno);
1064-
LockBuffer(parent->buffer,GIST_EXCLUSIVE);
1065-
gistcheckpage(r,parent->buffer);
1066-
parent->page= (Page)BufferGetPage(parent->buffer);
10671071
}
10681072

1069-
/*
1070-
* awful!!, we need search tree to find parent ... , but before we
1071-
* should release all old parent
1072-
*/
1073-
1074-
ptr=child->parent->parent;/* child->parent already released
1075-
* above */
1076-
while (ptr)
1073+
parent->blkno=GistPageGetOpaque(parent->page)->rightlink;
1074+
parent->downlinkoffnum=InvalidOffsetNumber;
1075+
UnlockReleaseBuffer(parent->buffer);
1076+
if (parent->blkno==InvalidBlockNumber)
10771077
{
1078-
ReleaseBuffer(ptr->buffer);
1079-
ptr=ptr->parent;
1078+
/*
1079+
* End of chain and still didn't find parent. It's a very-very
1080+
* rare situation when root splitted.
1081+
*/
1082+
break;
10801083
}
1084+
parent->buffer=ReadBuffer(r,parent->blkno);
1085+
LockBuffer(parent->buffer,GIST_EXCLUSIVE);
1086+
gistcheckpage(r,parent->buffer);
1087+
parent->page= (Page)BufferGetPage(parent->buffer);
1088+
}
10811089

1082-
/* ok, find new path */
1083-
ptr=parent=gistFindPath(r,child->blkno,&child->downlinkoffnum);
1090+
/*
1091+
* awful!!, we need search tree to find parent ... , but before we should
1092+
* release all old parent
1093+
*/
10841094

1085-
/* read all buffers as expected by caller */
1086-
/* note we don't lock them or gistcheckpage them here! */
1087-
while (ptr)
1088-
{
1089-
ptr->buffer=ReadBuffer(r,ptr->blkno);
1090-
ptr->page= (Page)BufferGetPage(ptr->buffer);
1091-
ptr=ptr->parent;
1092-
}
1095+
ptr=child->parent->parent;/* child->parent already released above */
1096+
while (ptr)
1097+
{
1098+
ReleaseBuffer(ptr->buffer);
1099+
ptr=ptr->parent;
1100+
}
10931101

1094-
/*installnewchain of parents to stack */
1095-
child->parent=parent;
1102+
/*ok, findnewpath */
1103+
ptr=parent=gistFindPath(r,child->blkno,&child->downlinkoffnum);
10961104

1097-
/* make recursive call to normal processing */
1098-
LockBuffer(child->parent->buffer,GIST_EXCLUSIVE);
1099-
gistFindCorrectParent(r,child);
1105+
/* read all buffers as expected by caller */
1106+
/* note we don't lock them or gistcheckpage them here! */
1107+
while (ptr)
1108+
{
1109+
ptr->buffer=ReadBuffer(r,ptr->blkno);
1110+
ptr->page= (Page)BufferGetPage(ptr->buffer);
1111+
ptr=ptr->parent;
11001112
}
1113+
1114+
/* install new chain of parents to stack */
1115+
child->parent=parent;
1116+
1117+
/* make recursive call to normal processing */
1118+
LockBuffer(child->parent->buffer,GIST_EXCLUSIVE);
1119+
gistFindCorrectParent(r,child,is_build);
11011120
}
11021121

11031122
/*
11041123
* Form a downlink pointer for the page in 'buf'.
11051124
*/
11061125
staticIndexTuple
11071126
gistformdownlink(Relationrel,Bufferbuf,GISTSTATE*giststate,
1108-
GISTInsertStack*stack)
1127+
GISTInsertStack*stack,boolis_build)
11091128
{
11101129
Pagepage=BufferGetPage(buf);
11111130
OffsetNumbermaxoff;
@@ -1146,7 +1165,7 @@ gistformdownlink(Relation rel, Buffer buf, GISTSTATE *giststate,
11461165
ItemIdiid;
11471166

11481167
LockBuffer(stack->parent->buffer,GIST_EXCLUSIVE);
1149-
gistFindCorrectParent(rel,stack);
1168+
gistFindCorrectParent(rel,stack,is_build);
11501169
iid=PageGetItemId(stack->parent->page,stack->downlinkoffnum);
11511170
downlink= (IndexTuple)PageGetItem(stack->parent->page,iid);
11521171
downlink=CopyIndexTuple(downlink);
@@ -1192,7 +1211,7 @@ gistfixsplit(GISTInsertState *state, GISTSTATE *giststate)
11921211
page=BufferGetPage(buf);
11931212

11941213
/* Form the new downlink tuples to insert to parent */
1195-
downlink=gistformdownlink(state->r,buf,giststate,stack);
1214+
downlink=gistformdownlink(state->r,buf,giststate,stack,state->is_build);
11961215

11971216
si->buf=buf;
11981217
si->downlink=downlink;
@@ -1346,7 +1365,7 @@ gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
13461365
right= (GISTPageSplitInfo*)list_nth(splitinfo,pos);
13471366
left= (GISTPageSplitInfo*)list_nth(splitinfo,pos-1);
13481367

1349-
gistFindCorrectParent(state->r,stack);
1368+
gistFindCorrectParent(state->r,stack,state->is_build);
13501369
if (gistinserttuples(state,stack->parent,giststate,
13511370
&right->downlink,1,
13521371
InvalidOffsetNumber,
@@ -1371,21 +1390,22 @@ gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
13711390
*/
13721391
tuples[0]=left->downlink;
13731392
tuples[1]=right->downlink;
1374-
gistFindCorrectParent(state->r,stack);
1375-
if (gistinserttuples(state,stack->parent,giststate,
1376-
tuples,2,
1377-
stack->downlinkoffnum,
1378-
left->buf,right->buf,
1379-
true,/* Unlock parent */
1380-
unlockbuf/* Unlock stack->buffer if caller wants
1381-
* that */
1382-
))
1383-
{
1384-
/*
1385-
* If the parent page was split, the downlink might have moved.
1386-
*/
1387-
stack->downlinkoffnum=InvalidOffsetNumber;
1388-
}
1393+
gistFindCorrectParent(state->r,stack,state->is_build);
1394+
(void)gistinserttuples(state,stack->parent,giststate,
1395+
tuples,2,
1396+
stack->downlinkoffnum,
1397+
left->buf,right->buf,
1398+
true,/* Unlock parent */
1399+
unlockbuf/* Unlock stack->buffer if caller
1400+
* wants that */
1401+
);
1402+
1403+
/*
1404+
* The downlink might have moved when we updated it. Even if the page
1405+
* wasn't split, because gistinserttuples() implements updating the old
1406+
* tuple by removing and re-inserting it!
1407+
*/
1408+
stack->downlinkoffnum=InvalidOffsetNumber;
13891409

13901410
Assert(left->buf==stack->buffer);
13911411

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp