public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/2] Refactor checks for deleted GiST pages. 3+ messages / 3 participants [nested] [flat]
* [PATCH 1/2] Refactor checks for deleted GiST pages. @ 2019-04-04 15:06 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Heikki Linnakangas @ 2019-04-04 15:06 UTC (permalink / raw) The explicit check in gistScanPage() isn't currently really necessary, as a deleted page is always empty, so the loop would fall through without doing anything, anyway. But it's a marginal optimization, and it gives a nice place to attach a comment to explain how it works. --- src/backend/access/gist/gist.c | 40 ++++++++++++------------------- src/backend/access/gist/gistget.c | 14 +++++++++++ 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 470b121e7da..79030e77153 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -709,14 +709,15 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, continue; } - if (stack->blkno != GIST_ROOT_BLKNO && - stack->parent->lsn < GistPageGetNSN(stack->page)) + if ((stack->blkno != GIST_ROOT_BLKNO && + stack->parent->lsn < GistPageGetNSN(stack->page)) || + GistPageIsDeleted(stack->page)) { /* - * Concurrent split detected. There's no guarantee that the - * downlink for this page is consistent with the tuple we're - * inserting anymore, so go back to parent and rechoose the best - * child. + * Concurrent split or page deletion detected. There's no + * guarantee that the downlink for this page is consistent with + * the tuple we're inserting anymore, so go back to parent and + * rechoose the best child. */ UnlockReleaseBuffer(stack->buffer); xlocked = false; @@ -735,9 +736,6 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTInsertStack *item; OffsetNumber downlinkoffnum; - /* currently, internal pages are never deleted */ - Assert(!GistPageIsDeleted(stack->page)); - downlinkoffnum = gistchoose(state.r, stack->page, itup, giststate); iid = PageGetItemId(stack->page, downlinkoffnum); idxtuple = (IndexTuple) PageGetItem(stack->page, iid); @@ -858,12 +856,13 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, * leaf/inner is enough to recognize split for root */ } - else if (GistFollowRight(stack->page) || - stack->parent->lsn < GistPageGetNSN(stack->page)) + else if ((GistFollowRight(stack->page) || + stack->parent->lsn < GistPageGetNSN(stack->page)) && + GistPageIsDeleted(stack->page)) { /* - * The page was split while we momentarily unlocked the - * page. Go back to parent. + * The page was split or deleted while we momentarily + * unlocked the page. Go back to parent. */ UnlockReleaseBuffer(stack->buffer); xlocked = false; @@ -872,18 +871,6 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, } } - /* - * The page might have been deleted after we scanned the parent - * and saw the downlink. - */ - if (GistPageIsDeleted(stack->page)) - { - UnlockReleaseBuffer(stack->buffer); - xlocked = false; - state.stack = stack = stack->parent; - continue; - } - /* now state.stack->(page, buffer and blkno) points to leaf page */ gistinserttuple(&state, stack, giststate, itup, @@ -947,6 +934,9 @@ gistFindPath(Relation r, BlockNumber child, OffsetNumber *downlinkoffnum) break; } + /* currently, internal pages are never deleted */ + Assert(!GistPageIsDeleted(page)); + top->lsn = BufferGetLSNAtomic(buffer); /* diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c index 8108fbb7d8e..77ae2fb3395 100644 --- a/src/backend/access/gist/gistget.c +++ b/src/backend/access/gist/gistget.c @@ -377,6 +377,20 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, double *myDistances, MemoryContextSwitchTo(oldcxt); } + /* + * Check if the page was deleted after we saw the downlink. There's + * nothing of interest on a deleted page. Note that we must do this + * after checking the NSN for concurrent splits! It's possible that + * the page originally contained some tuples that are visible to use, + * but was split so that all the visible tuples were moved to another + * page, and then this page was deleted. + */ + if (GistPageIsDeleted(page)) + { + UnlockReleaseBuffer(buffer); + return; + } + so->nPageData = so->curPageData = 0; scan->xs_hitup = NULL; /* might point into pageDataCxt */ if (so->pageDataCxt) -- 2.20.1 --------------A4A2DD62EBB140914640E8DD Content-Type: text/x-patch; name="0002-Use-full-64-bit-XID-for-checking-if-a-deleted-GiST-p.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0002-Use-full-64-bit-XID-for-checking-if-a-deleted-GiST-p.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add PQsendSyncMessage() to libpq @ 2023-04-29 16:06 Anton Kirilov <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Anton Kirilov @ 2023-04-29 16:06 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers Hello, On 28/04/2023 13:06, Robert Haas wrote: > On Fri, Mar 24, 2023 at 6:39 PM Anton Kirilov <[email protected]> wrote: >> I have attached a patch that introduces PQsendSyncMessage()... > > I wonder whether this is the naming that we want. The two names are > significantly different. Something like PQpipelineSendSync() would be > more similar. The reason is that the function is modeled after PQsendFlushRequest(), since it felt closer to what I was trying to achieve, i.e. appending a protocol message to the output buffer without doing any actual I/O operations. > I also wonder, really even more, whether it would be better to do > something like PQpipelinePutSync(PGconn *conn, bool flush) with > PQpipelineSync(conn) just meaning PQpipelinePutSync(conn, true). Actually I believe that there is another issue with PQpipelineSync() that has to do with ergonomics - according to a comment inside its body ( https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/interfaces/libpq/fe-exec.c;h=a16bbf... ) it could fail silently to send all the buffered data, which seems to be problematic when operating in non-blocking mode. In practice, this means that all calls to PQpipelineSync() must be followed by execution of PQflush() to check whether the application should poll for write readiness. I suppose that that was the reason why I was going for a solution that did not combine changing the connection state with doing I/O operations. In any case I am not particularly attached to any naming or the exact shape of the new API, as long as it achieves the same goal (reducing the number of system calls), but before I make any substantial changes to my patch, I would like to hear your thoughts on the matter. Best wishes, Anton Kirilov ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add PQsendSyncMessage() to libpq @ 2023-05-02 00:42 Michael Paquier <[email protected]> parent: Anton Kirilov <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Michael Paquier @ 2023-05-02 00:42 UTC (permalink / raw) To: Anton Kirilov <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Sat, Apr 29, 2023 at 05:06:03PM +0100, Anton Kirilov wrote: > In any case I am not particularly attached to any naming or the exact shape > of the new API, as long as it achieves the same goal (reducing the number of > system calls), but before I make any substantial changes to my patch, I > would like to hear your thoughts on the matter. Another thing that may matter in terms of extensibility? Would a boolean argument really be the best design? Could it be better to have instead one API with a bits32 and some flags controlling its internals? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-05-02 00:42 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-04-04 15:06 [PATCH 1/2] Refactor checks for deleted GiST pages. Heikki Linnakangas <[email protected]> 2023-04-29 16:06 Re: Add PQsendSyncMessage() to libpq Anton Kirilov <[email protected]> 2023-05-02 00:42 ` Re: Add PQsendSyncMessage() to libpq Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox