Horton
Menu

Re: brininsert optimization opportunity

Richard Guo <guofenglinux@gmail.com>

View thread
From: Richard Guo <guofenglinux@gmail.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>, Matthias van de Meent <boekewurm+postgres@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, pgsql-hackers <pgsql-hackers@postgresql.org>, Ashwin Agrawal <ashwinstar@gmail.com>
Date: 2023-11-27T05:28:22Z
Lists: pgsql-hackers 
On Sun, Nov 26, 2023 at 4:06 AM Tomas Vondra <tomas.vondra@enterprisedb.com>
wrote:

> I've done a bit more cleanup on the last version of the patch (renamed
> the fields to start with bis_ as agreed, rephrased the comments / docs /
> commit message a bit) and pushed.


It seems that we have an oversight in this commit.  If there is no tuple
that has been inserted, we wouldn't have an available insert state in
the clean up phase.  So the Assert in brininsertcleanup() is not always
right.  For example:

regression=# update brin_summarize set value = brin_summarize.value;
server closed the connection unexpectedly

So I wonder if we should check 'bistate' and do the clean up only if
there is an available one, something like below.

--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -359,7 +359,9 @@ brininsertcleanup(IndexInfo *indexInfo)
 {
    BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;

-   Assert(bistate);
+   /* We don't have an available insert state, nothing to do */
+   if (!bistate)
+       return;

Thanks
Richard

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add missing index_insert_cleanup calls

  2. Fix a couple typos in BRIN code

  3. Check if ii_AmCache is NULL in aminsertcleanup

  4. Use fipshash in brin_multi test

  5. Reuse BrinDesc and BrinRevmap in brininsert

  6. Consider fillfactor when estimating relation size

  7. Postpone some stuff out of ExecInitModifyTable.