public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v8 1/4] Add READ_REPLICATION_SLOT command
13+ messages / 6 participants
[nested] [flat]
* [PATCH v8 1/4] Add READ_REPLICATION_SLOT command
@ 2021-09-02 07:25 Michael Paquier <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Michael Paquier @ 2021-09-02 07:25 UTC (permalink / raw)
---
doc/src/sgml/protocol.sgml | 47 +++++++++++
src/backend/replication/repl_gram.y | 16 +++-
src/backend/replication/repl_scanner.l | 1 +
src/backend/replication/walsender.c | 89 +++++++++++++++++++++
src/include/nodes/nodes.h | 1 +
src/include/nodes/replnodes.h | 10 +++
src/test/recovery/t/001_stream_rep.pl | 47 ++++++++++-
src/test/recovery/t/006_logical_decoding.pl | 9 ++-
src/tools/pgindent/typedefs.list | 1 +
9 files changed, 218 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index b95cc88599..51a15cc3da 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2067,6 +2067,53 @@ The commands accepted in replication mode are:
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>READ_REPLICATION_SLOT</literal> <replaceable class="parameter">slot_name</replaceable>
+ <indexterm><primary>READ_REPLICATION_SLOT</primary></indexterm>
+ </term>
+ <listitem>
+ <para>
+ Read the information of a replication slot. Returns a tuple with
+ <literal>NULL</literal> values if the replication slot does not
+ exist. This command is currently only supported for <literal>physical</literal> slots.
+ </para>
+ <para>
+ In response to this command, the server will return a one-row result set,
+ containing the following fields:
+ <variablelist>
+ <varlistentry>
+ <term><literal>slot_type</literal> (<type>text</type>)</term>
+ <listitem>
+ <para>
+ The replication slot's type, either <literal>physical</literal> or
+ <literal>NULL</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>restart_lsn</literal> (<type>text</type>)</term>
+ <listitem>
+ <para>
+ The replication slot's <literal>restart_lsn</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>restart_tli</literal> (<type>int4</type>)</term>
+ <listitem>
+ <para>
+ The timeline ID for the <literal>restart_lsn</literal> position,
+ when following the current timeline history.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>START_REPLICATION</literal> [ <literal>SLOT</literal> <replaceable class="parameter">slot_name</replaceable> ] [ <literal>PHYSICAL</literal> ] <replaceable class="parameter">XXX/XXX</replaceable> [ <literal>TIMELINE</literal> <replaceable class="parameter">tli</replaceable> ]
<indexterm><primary>START_REPLICATION</primary></indexterm>
diff --git a/src/backend/replication/repl_gram.y b/src/backend/replication/repl_gram.y
index 126380e2df..913a99da5a 100644
--- a/src/backend/replication/repl_gram.y
+++ b/src/backend/replication/repl_gram.y
@@ -64,6 +64,7 @@ static SQLCmd *make_sqlcmd(void);
/* Keyword tokens. */
%token K_BASE_BACKUP
%token K_IDENTIFY_SYSTEM
+%token K_READ_REPLICATION_SLOT
%token K_SHOW
%token K_START_REPLICATION
%token K_CREATE_REPLICATION_SLOT
@@ -94,7 +95,7 @@ static SQLCmd *make_sqlcmd(void);
%type <node> command
%type <node> base_backup start_replication start_logical_replication
create_replication_slot drop_replication_slot identify_system
- timeline_history show sql_cmd
+ read_replication_slot timeline_history show sql_cmd
%type <list> base_backup_legacy_opt_list generic_option_list
%type <defelt> base_backup_legacy_opt generic_option
%type <uintval> opt_timeline
@@ -120,6 +121,7 @@ opt_semicolon: ';'
command:
identify_system
+ | read_replication_slot
| base_backup
| start_replication
| start_logical_replication
@@ -140,6 +142,18 @@ identify_system:
}
;
+/*
+ * READ_REPLICATION_SLOT %s
+ */
+read_replication_slot:
+ K_READ_REPLICATION_SLOT var_name
+ {
+ ReadReplicationSlotCmd *n = makeNode(ReadReplicationSlotCmd);
+ n->slotname = $2;
+ $$ = (Node *) n;
+ }
+ ;
+
/*
* SHOW setting
*/
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index c038a636c3..1b599c255e 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -85,6 +85,7 @@ identifier {ident_start}{ident_cont}*
BASE_BACKUP { return K_BASE_BACKUP; }
FAST { return K_FAST; }
IDENTIFY_SYSTEM { return K_IDENTIFY_SYSTEM; }
+READ_REPLICATION_SLOT { return K_READ_REPLICATION_SLOT; }
SHOW { return K_SHOW; }
LABEL { return K_LABEL; }
NOWAIT { return K_NOWAIT; }
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index b811a5c0ef..5d68a7e66a 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -232,6 +232,7 @@ static void XLogSendLogical(void);
static void WalSndDone(WalSndSendDataCallback send_data);
static XLogRecPtr GetStandbyFlushRecPtr(void);
static void IdentifySystem(void);
+static void ReadReplicationSlot(ReadReplicationSlotCmd *cmd);
static void CreateReplicationSlot(CreateReplicationSlotCmd *cmd);
static void DropReplicationSlot(DropReplicationSlotCmd *cmd);
static void StartReplication(StartReplicationCmd *cmd);
@@ -457,6 +458,87 @@ IdentifySystem(void)
end_tup_output(tstate);
}
+/* Handle READ_REPLICATION_SLOT command */
+static void
+ReadReplicationSlot(ReadReplicationSlotCmd *cmd)
+{
+#define READ_REPLICATION_SLOT_COLS 3
+ ReplicationSlot *slot;
+ DestReceiver *dest;
+ TupOutputState *tstate;
+ TupleDesc tupdesc;
+ Datum values[READ_REPLICATION_SLOT_COLS];
+ bool nulls[READ_REPLICATION_SLOT_COLS];
+
+ tupdesc = CreateTemplateTupleDesc(READ_REPLICATION_SLOT_COLS);
+ TupleDescInitBuiltinEntry(tupdesc, (AttrNumber) 1, "slot_type",
+ TEXTOID, -1, 0);
+ TupleDescInitBuiltinEntry(tupdesc, (AttrNumber) 2, "restart_lsn",
+ TEXTOID, -1, 0);
+ TupleDescInitBuiltinEntry(tupdesc, (AttrNumber) 3, "restart_tli",
+ INT4OID, -1, 0);
+
+ MemSet(nulls, true, READ_REPLICATION_SLOT_COLS * sizeof(bool));
+
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+ slot = SearchNamedReplicationSlot(cmd->slotname, false);
+ if (slot == NULL || !slot->in_use)
+ {
+ LWLockRelease(ReplicationSlotControlLock);
+ }
+ else
+ {
+ List *timeline_history = NIL;
+ ReplicationSlot slot_contents;
+ int i = 0;
+ char xloc[MAXFNAMELEN];
+ TimeLineID slots_position_timeline;
+
+ /* Copy slot contents while holding spinlock */
+ SpinLockAcquire(&slot->mutex);
+ slot_contents = *slot;
+ SpinLockRelease(&slot->mutex);
+ LWLockRelease(ReplicationSlotControlLock);
+
+ if (OidIsValid(slot_contents.data.database))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("READ_REPLICATION_SLOT is only supported for physical slots"));
+ values[i] = CStringGetTextDatum("physical");
+ nulls[i] = false;
+ i++;
+ if (!XLogRecPtrIsInvalid(slot_contents.data.restart_lsn))
+ {
+ snprintf(xloc, sizeof(xloc), "%X/%X",
+ LSN_FORMAT_ARGS(slot_contents.data.restart_lsn));
+ values[i] = CStringGetTextDatum(xloc);
+ nulls[i] = false;
+ }
+ i++;
+
+ /*
+ * Now get the timeline this wal was produced on, to get to the
+ * current timeline
+ */
+ if (!XLogRecPtrIsInvalid(slot_contents.data.restart_lsn))
+ {
+ timeline_history = readTimeLineHistory(ThisTimeLineID);
+ slots_position_timeline = tliOfPointInHistory(slot_contents.data.restart_lsn,
+ timeline_history);
+ values[i] = Int32GetDatum(slots_position_timeline);
+ nulls[i] = false;
+ }
+ i++;
+
+ Assert(i == READ_REPLICATION_SLOT_COLS);
+ }
+
+ dest = CreateDestReceiver(DestRemoteSimple);
+ tstate = begin_tup_output_tupdesc(dest, tupdesc, &TTSOpsVirtual);
+ do_tup_output(tstate, values, nulls);
+ end_tup_output(tstate);
+}
+
/*
* Handle TIMELINE_HISTORY command.
@@ -1622,6 +1704,13 @@ exec_replication_command(const char *cmd_string)
EndReplicationCommand(cmdtag);
break;
+ case T_ReadReplicationSlotCmd:
+ cmdtag = "READ_REPLICATION_SLOT";
+ set_ps_display(cmdtag);
+ ReadReplicationSlot((ReadReplicationSlotCmd *) cmd_node);
+ EndReplicationCommand(cmdtag);
+ break;
+
case T_BaseBackupCmd:
cmdtag = "BASE_BACKUP";
set_ps_display(cmdtag);
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index e0057daa06..6201940637 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -493,6 +493,7 @@ typedef enum NodeTag
* TAGS FOR REPLICATION GRAMMAR PARSE NODES (replnodes.h)
*/
T_IdentifySystemCmd,
+ T_ReadReplicationSlotCmd,
T_BaseBackupCmd,
T_CreateReplicationSlotCmd,
T_DropReplicationSlotCmd,
diff --git a/src/include/nodes/replnodes.h b/src/include/nodes/replnodes.h
index faa3a251f2..46384ea074 100644
--- a/src/include/nodes/replnodes.h
+++ b/src/include/nodes/replnodes.h
@@ -33,6 +33,16 @@ typedef struct IdentifySystemCmd
NodeTag type;
} IdentifySystemCmd;
+/* ----------------------
+ * READ_REPLICATION_SLOT command
+ * ----------------------
+ */
+typedef struct ReadReplicationSlotCmd
+{
+ NodeTag type;
+ char *slotname;
+} ReadReplicationSlotCmd;
+
/* ----------------------
* BASE_BACKUP command
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 9916a36012..2317f55149 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 49;
+use Test::More tests => 55;
# Initialize primary node
my $node_primary = PostgresNode->new('primary');
@@ -254,6 +254,51 @@ ok( $ret == 0,
"SHOW with superuser-settable parameter, replication role and logical replication"
);
+note "testing READ_REPLICATION_SLOT command";
+
+my $slotname = 'test_read_replication_slot_physical';
+
+($ret, $stdout, $stderr) = $node_primary->psql(
+ 'postgres',
+ 'READ_REPLICATION_SLOT non_existent_slot;',
+ extra_params => [ '-d', $connstr_rep ]);
+ok( $ret == 0,
+ "READ_REPLICATION_SLOT does not produce an error with non existent slot");
+ok($stdout eq '||',
+ "READ_REPLICATION_SLOT returns NULL values if slot does not exist");
+
+($ret, $stdout, $stderr) = $node_primary->psql(
+ 'postgres',
+ "CREATE_REPLICATION_SLOT $slotname PHYSICAL RESERVE_WAL;",
+ extra_params => [ '-d', $connstr_rep ],
+ 0,
+ 'physical slot created on primary');
+
+($ret, $stdout, $stderr) = $node_primary->psql(
+ 'postgres',
+ "READ_REPLICATION_SLOT $slotname;",
+ extra_params => [ '-d', $connstr_rep ]);
+ok($ret == 0,
+ "READ_REPLICATION_SLOT does not produce an error with existing slot");
+like($stdout, qr/^physical\|[^|]*\|1$/,
+ "READ_REPLICATION_SLOT returns tuple corresponding to the slot");
+
+$node_primary->psql(
+ 'postgres',
+ "DROP_REPLICATION_SLOT $slotname;",
+ extra_params => [ '-d', $connstr_rep ],
+ 0,
+ 'physical slot dropped on primary');
+
+($ret, $stdout, $stderr) = $node_primary->psql(
+ 'postgres',
+ "READ_REPLICATION_SLOT $slotname;",
+ extra_params => [ '-d', $connstr_rep ]);
+ok($ret == 0,
+ "READ_REPLICATION_SLOT does not produce an error with dropped slot");
+ok($stdout eq '||',
+ "READ_REPLICATION_SLOT returns NULL values if slot has been dropped");
+
note "switching to physical replication slot";
# Switch to using a physical replication slot. We can do this without a new
diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index cc116062c2..1e08afca74 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -10,7 +10,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 14;
+use Test::More tests => 15;
use Config;
# Initialize primary node
@@ -39,6 +39,13 @@ ok( $stderr =~
m/replication slot "test_slot" was not created in this database/,
"Logical decoding correctly fails to start");
+($result, $stdout, $stderr) = $node_primary->psql(
+ 'template1',
+ qq[READ_REPLICATION_SLOT test_slot;],
+ replication => 'database');
+like($stderr, qr/READ_REPLICATION_SLOT is only supported for physical slots/,
+ 'Logical replication slot is not supported');
+
# Check case of walsender not using a database connection. Logical
# decoding should not be allowed.
($result, $stdout, $stderr) = $node_primary->psql(
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index cb5b5ec74c..a8e4a2afd6 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2128,6 +2128,7 @@ ReadBufferMode
ReadBytePtrType
ReadExtraTocPtrType
ReadFunc
+ReadReplicationSlotCmd
ReassignOwnedStmt
RecheckForeignScan_function
RecordCacheEntry
--
2.33.0
--nextPart2557710.X9hSmTKtgW
Content-Disposition: attachment;
filename="v8-0002-Use-READ_REPLICATION_SLOT-command-in-pg_receivewa.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="UTF-8";
name="v8-0002-Use-READ_REPLICATION_SLOT-command-in-pg_receivewa.patch"
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-07-05 18:57 Soumyadeep Chakraborty <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-07-05 18:57 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
On Wed, Jul 5, 2023 at 3:16 AM Tomas Vondra
<[email protected]> wrote:
> > I'll try this out and introduce a couple of new index AM callbacks. I
> > think it's best to do it before releasing the locks - otherwise it
> > might be weird
> > to manipulate buffers of an index relation, without having some sort of lock on
> > it. I'll think about it some more.
> >
>
> I don't understand why would this need more than just a callback to
> release the cache.
We wouldn't. I thought that it would be slightly cleaner and slightly more
performant if we moved the (if !state) branches out of the XXXinsert()
functions.
But I guess, let's minimize the changes here. One cleanup callback is enough.
> > PS: It should be possible to make GIN and GiST use the new index AM APIs
> > as well.
> >
>
> Why should GIN/GiST use the new API? I think it's perfectly sensible to
> only require the "cleanup callback" when just pfree() is not enough.
Yeah no need.
Attached v3 of the patch w/ a single index AM callback.
Regards,
Soumyadeep (VMware)
Attachments:
[text/x-patch] v3-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch (13.7K, ../../CAE-ML+9JVD3vt7JZtmNy8hOFQKtP2jxJtf27=kE=ViGyv36V3w@mail.gmail.com/2-v3-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch)
download | inline diff:
From 563b905da5c2602113044689e37f8385cbfbde64 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty <[email protected]>
Date: Wed, 5 Jul 2023 10:59:11 -0700
Subject: [PATCH v3 1/1] Reuse revmap and brin desc in brininsert
brininsert() used to have code that performed per-tuple initialization
of the revmap. That had some overhead.
To mitigate, we introduce a new AM callback to clean up state at the end
of all inserts, and perform the revmap cleanup there.
---
contrib/bloom/blutils.c | 1 +
doc/src/sgml/indexam.sgml | 14 +++++-
src/backend/access/brin/brin.c | 74 +++++++++++++++++++++++-----
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/index/indexam.c | 15 ++++++
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/executor/execIndexing.c | 5 ++
src/include/access/amapi.h | 3 ++
src/include/access/brin_internal.h | 1 +
src/include/access/genam.h | 2 +
13 files changed, 108 insertions(+), 12 deletions(-)
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index d935ed8fbdf..9720f69de09 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -131,6 +131,7 @@ blhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = blbuild;
amroutine->ambuildempty = blbuildempty;
amroutine->aminsert = blinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = blbulkdelete;
amroutine->amvacuumcleanup = blvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index e813e2b620a..17f7d6597f1 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -139,6 +139,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
@@ -355,11 +356,22 @@ aminsert (Relation indexRelation,
within an SQL statement, it can allocate space
in <literal>indexInfo->ii_Context</literal> and store a pointer to the
data in <literal>indexInfo->ii_AmCache</literal> (which will be NULL
- initially).
+ initially). If the data cached contains structures that can be simply pfree'd,
+ they will implicitly be pfree'd. But, if it contains more complex data, such
+ as Buffers or TupleDescs, additional cleanup is necessary. This additional
+ cleanup can be performed in <literal>indexinsertcleanup</literal>.
</para>
<para>
<programlisting>
+bool
+aminsertcleanup (IndexInfo *indexInfo);
+</programlisting>
+ Clean up state that was maintained across successive inserts in
+ <literal>indexInfo->ii_AmCache</literal>. This is useful if the data
+ contained is complex - like Buffers or TupleDescs which need additional
+ cleanup, unlike simple structures that will be implicitly pfree'd.
+<programlisting>
IndexBulkDeleteResult *
ambulkdelete (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3c6a956eaa3..2da59f2ab03 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -58,6 +58,17 @@ typedef struct BrinBuildState
BrinMemTuple *bs_dtuple;
} BrinBuildState;
+/*
+ * We use a BrinInsertState to capture running state spanning multiple
+ * brininsert invocations, within the same command.
+ */
+typedef struct BrinInsertState
+{
+ BrinRevmap *bs_rmAccess;
+ BrinDesc *bs_desc;
+ BlockNumber bs_pages_per_range;
+} BrinInsertState;
+
/*
* Struct used as "opaque" during index scans
*/
@@ -72,6 +83,7 @@ typedef struct BrinOpaque
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
+static BrinInsertState *initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo);
static void terminate_brin_buildstate(BrinBuildState *state);
static void brinsummarize(Relation index, Relation heapRel, BlockNumber pageRange,
bool include_partial, double *numSummarized, double *numExisting);
@@ -117,6 +129,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = brinbuild;
amroutine->ambuildempty = brinbuildempty;
amroutine->aminsert = brininsert;
+ amroutine->aminsertcleanup = brininsertcleanup;
amroutine->ambulkdelete = brinbulkdelete;
amroutine->amvacuumcleanup = brinvacuumcleanup;
amroutine->amcanreturn = NULL;
@@ -140,6 +153,28 @@ brinhandler(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(amroutine);
}
+/*
+ * Initialize a BrinInsertState to maintain state to be used across multiple
+ * tuple inserts, within the same command.
+ */
+static BrinInsertState *
+initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(indexInfo->ii_Context);
+ bistate = palloc0(sizeof(BrinInsertState));
+ bistate->bs_desc = brin_build_desc(idxRel);
+ bistate->bs_rmAccess = brinRevmapInitialize(idxRel,
+ &bistate->bs_pages_per_range,
+ NULL);
+ indexInfo->ii_AmCache = bistate;
+ MemoryContextSwitchTo(oldcxt);
+
+ return bistate;
+}
+
/*
* A tuple in the heap is being inserted. To keep a brin index up to date,
* we need to obtain the relevant index tuple and compare its stored values
@@ -162,14 +197,22 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
BlockNumber pagesPerRange;
BlockNumber origHeapBlk;
BlockNumber heapBlk;
- BrinDesc *bdesc = (BrinDesc *) indexInfo->ii_AmCache;
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
BrinRevmap *revmap;
+ BrinDesc *bdesc;
Buffer buf = InvalidBuffer;
MemoryContext tupcxt = NULL;
MemoryContext oldcxt = CurrentMemoryContext;
bool autosummarize = BrinGetAutoSummarize(idxRel);
- revmap = brinRevmapInitialize(idxRel, &pagesPerRange, NULL);
+ if (!bistate)
+ {
+ /* First time through in this statement? */
+ bistate = initialize_brin_insertstate(idxRel, indexInfo);
+ }
+ revmap = bistate->bs_rmAccess;
+ bdesc = bistate->bs_desc;
+ pagesPerRange = bistate->bs_pages_per_range;
/*
* origHeapBlk is the block number where the insertion occurred. heapBlk
@@ -228,14 +271,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
if (!brtup)
break;
- /* First time through in this statement? */
- if (bdesc == NULL)
- {
- MemoryContextSwitchTo(indexInfo->ii_Context);
- bdesc = brin_build_desc(idxRel);
- indexInfo->ii_AmCache = (void *) bdesc;
- MemoryContextSwitchTo(oldcxt);
- }
/* First time through in this brininsert call? */
if (tupcxt == NULL)
{
@@ -306,7 +341,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
break;
}
- brinRevmapTerminate(revmap);
if (BufferIsValid(buf))
ReleaseBuffer(buf);
MemoryContextSwitchTo(oldcxt);
@@ -316,6 +350,24 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
return false;
}
+/*
+ * Callback to clean up the BrinInsertState once all tuple inserts are done.
+ */
+void
+brininsertcleanup(IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
+
+ Assert(bistate);
+ /*
+ * Clean up the revmap. Note that the brinDesc has already been cleaned up
+ * as part of its own memory context.
+ */
+ brinRevmapTerminate(bistate->bs_rmAccess);
+ bistate->bs_rmAccess = NULL;
+ bistate->bs_desc = NULL;
+}
+
/*
* Initialize state for a BRIN index scan.
*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 437f24753c0..56223305442 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -64,6 +64,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = ginbuild;
amroutine->ambuildempty = ginbuildempty;
amroutine->aminsert = gininsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = ginbulkdelete;
amroutine->amvacuumcleanup = ginvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 516465f8b7d..c42746130cc 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -86,6 +86,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = gistbuild;
amroutine->ambuildempty = gistbuildempty;
amroutine->aminsert = gistinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = gistbulkdelete;
amroutine->amvacuumcleanup = gistvacuumcleanup;
amroutine->amcanreturn = gistcanreturn;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index fc5d97f606e..cba15cde0d7 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -83,6 +83,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = hashbuild;
amroutine->ambuildempty = hashbuildempty;
amroutine->aminsert = hashinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = hashbulkdelete;
amroutine->amvacuumcleanup = hashvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b25b03f7abc..55b9c18369a 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -196,6 +196,21 @@ index_insert(Relation indexRelation,
indexInfo);
}
+/* -------------------------
+ * index_insert_cleanup - clean up after all index inserts are done.
+ * -------------------------
+ */
+void
+index_insert_cleanup(Relation indexRelation,
+ IndexInfo *indexInfo)
+{
+ RELATION_CHECKS;
+ Assert(indexInfo);
+
+ if (indexRelation->rd_indam->aminsertcleanup)
+ indexRelation->rd_indam->aminsertcleanup(indexInfo);
+}
+
/*
* index_beginscan - start a scan of an index with amgettuple
*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4553aaee531..2753566437f 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -122,6 +122,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = btbuild;
amroutine->ambuildempty = btbuildempty;
amroutine->aminsert = btinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = btbulkdelete;
amroutine->amvacuumcleanup = btvacuumcleanup;
amroutine->amcanreturn = btcanreturn;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 190e4f76a9e..b8efcf5cfb2 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -70,6 +70,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->ambuild = spgbuild;
amroutine->ambuildempty = spgbuildempty;
amroutine->aminsert = spginsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = spgbulkdelete;
amroutine->amvacuumcleanup = spgvacuumcleanup;
amroutine->amcanreturn = spgcanreturn;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 1d82b64b897..63699fdd93d 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -233,15 +233,20 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
int i;
int numIndices;
RelationPtr indexDescs;
+ IndexInfo **indexInfos;
numIndices = resultRelInfo->ri_NumIndices;
indexDescs = resultRelInfo->ri_IndexRelationDescs;
+ indexInfos = resultRelInfo->ri_IndexRelationInfo;
for (i = 0; i < numIndices; i++)
{
if (indexDescs[i] == NULL)
continue; /* shouldn't happen? */
+ /* Give the index a chance to do some post-insert cleanup */
+ index_insert_cleanup(indexDescs[i], indexInfos[i]);
+
/* Drop lock acquired by ExecOpenIndices */
index_close(indexDescs[i], RowExclusiveLock);
}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 4476ff7fba1..d29721eaf1e 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -113,6 +113,8 @@ typedef bool (*aminsert_function) (Relation indexRelation,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
+
/* bulk delete */
typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
@@ -261,6 +263,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 97ddc925b27..ed231e208eb 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -96,6 +96,7 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void brininsertcleanup(struct IndexInfo *indexInfo);
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index a3087956654..2cf9dc5dca9 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -148,6 +148,8 @@ extern bool index_insert(Relation indexRelation,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void index_insert_cleanup(Relation indexRelation,
+ struct IndexInfo *indexInfo);
extern IndexScanDesc index_beginscan(Relation heapRelation,
Relation indexRelation,
--
2.34.1
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-07-29 16:28 Soumyadeep Chakraborty <[email protected]>
parent: Soumyadeep Chakraborty <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-07-29 16:28 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
Attached v4 of the patch, rebased against latest HEAD.
Regards,
Soumyadeep (VMware)
Attachments:
[text/x-patch] v4-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch (13.7K, ../../CAE-ML+_jerNg64mSD5VYy=bOxcRQSr-QfESHvCVFPEwMM_jufQ@mail.gmail.com/2-v4-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch)
download | inline diff:
From b5fb12fbb8b0c1b2963a05a2877b5063bbc75f58 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty <[email protected]>
Date: Sat, 29 Jul 2023 09:17:49 -0700
Subject: [PATCH v4 1/1] Reuse revmap and brin desc in brininsert
brininsert() used to have code that performed per-tuple initialization
of the revmap. That had some overhead.
To mitigate, we introduce a new AM callback to clean up state at the end
of all inserts, and perform the revmap cleanup there.
---
contrib/bloom/blutils.c | 1 +
doc/src/sgml/indexam.sgml | 14 +++++-
src/backend/access/brin/brin.c | 74 +++++++++++++++++++++++-----
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/index/indexam.c | 15 ++++++
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/executor/execIndexing.c | 5 ++
src/include/access/amapi.h | 3 ++
src/include/access/brin_internal.h | 1 +
src/include/access/genam.h | 2 +
13 files changed, 108 insertions(+), 12 deletions(-)
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index d935ed8fbdf..9720f69de09 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -131,6 +131,7 @@ blhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = blbuild;
amroutine->ambuildempty = blbuildempty;
amroutine->aminsert = blinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = blbulkdelete;
amroutine->amvacuumcleanup = blvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index e813e2b620a..17f7d6597f1 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -139,6 +139,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
@@ -355,11 +356,22 @@ aminsert (Relation indexRelation,
within an SQL statement, it can allocate space
in <literal>indexInfo->ii_Context</literal> and store a pointer to the
data in <literal>indexInfo->ii_AmCache</literal> (which will be NULL
- initially).
+ initially). If the data cached contains structures that can be simply pfree'd,
+ they will implicitly be pfree'd. But, if it contains more complex data, such
+ as Buffers or TupleDescs, additional cleanup is necessary. This additional
+ cleanup can be performed in <literal>indexinsertcleanup</literal>.
</para>
<para>
<programlisting>
+bool
+aminsertcleanup (IndexInfo *indexInfo);
+</programlisting>
+ Clean up state that was maintained across successive inserts in
+ <literal>indexInfo->ii_AmCache</literal>. This is useful if the data
+ contained is complex - like Buffers or TupleDescs which need additional
+ cleanup, unlike simple structures that will be implicitly pfree'd.
+<programlisting>
IndexBulkDeleteResult *
ambulkdelete (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3c6a956eaa3..2da59f2ab03 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -58,6 +58,17 @@ typedef struct BrinBuildState
BrinMemTuple *bs_dtuple;
} BrinBuildState;
+/*
+ * We use a BrinInsertState to capture running state spanning multiple
+ * brininsert invocations, within the same command.
+ */
+typedef struct BrinInsertState
+{
+ BrinRevmap *bs_rmAccess;
+ BrinDesc *bs_desc;
+ BlockNumber bs_pages_per_range;
+} BrinInsertState;
+
/*
* Struct used as "opaque" during index scans
*/
@@ -72,6 +83,7 @@ typedef struct BrinOpaque
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
+static BrinInsertState *initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo);
static void terminate_brin_buildstate(BrinBuildState *state);
static void brinsummarize(Relation index, Relation heapRel, BlockNumber pageRange,
bool include_partial, double *numSummarized, double *numExisting);
@@ -117,6 +129,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = brinbuild;
amroutine->ambuildempty = brinbuildempty;
amroutine->aminsert = brininsert;
+ amroutine->aminsertcleanup = brininsertcleanup;
amroutine->ambulkdelete = brinbulkdelete;
amroutine->amvacuumcleanup = brinvacuumcleanup;
amroutine->amcanreturn = NULL;
@@ -140,6 +153,28 @@ brinhandler(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(amroutine);
}
+/*
+ * Initialize a BrinInsertState to maintain state to be used across multiple
+ * tuple inserts, within the same command.
+ */
+static BrinInsertState *
+initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(indexInfo->ii_Context);
+ bistate = palloc0(sizeof(BrinInsertState));
+ bistate->bs_desc = brin_build_desc(idxRel);
+ bistate->bs_rmAccess = brinRevmapInitialize(idxRel,
+ &bistate->bs_pages_per_range,
+ NULL);
+ indexInfo->ii_AmCache = bistate;
+ MemoryContextSwitchTo(oldcxt);
+
+ return bistate;
+}
+
/*
* A tuple in the heap is being inserted. To keep a brin index up to date,
* we need to obtain the relevant index tuple and compare its stored values
@@ -162,14 +197,22 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
BlockNumber pagesPerRange;
BlockNumber origHeapBlk;
BlockNumber heapBlk;
- BrinDesc *bdesc = (BrinDesc *) indexInfo->ii_AmCache;
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
BrinRevmap *revmap;
+ BrinDesc *bdesc;
Buffer buf = InvalidBuffer;
MemoryContext tupcxt = NULL;
MemoryContext oldcxt = CurrentMemoryContext;
bool autosummarize = BrinGetAutoSummarize(idxRel);
- revmap = brinRevmapInitialize(idxRel, &pagesPerRange, NULL);
+ if (!bistate)
+ {
+ /* First time through in this statement? */
+ bistate = initialize_brin_insertstate(idxRel, indexInfo);
+ }
+ revmap = bistate->bs_rmAccess;
+ bdesc = bistate->bs_desc;
+ pagesPerRange = bistate->bs_pages_per_range;
/*
* origHeapBlk is the block number where the insertion occurred. heapBlk
@@ -228,14 +271,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
if (!brtup)
break;
- /* First time through in this statement? */
- if (bdesc == NULL)
- {
- MemoryContextSwitchTo(indexInfo->ii_Context);
- bdesc = brin_build_desc(idxRel);
- indexInfo->ii_AmCache = (void *) bdesc;
- MemoryContextSwitchTo(oldcxt);
- }
/* First time through in this brininsert call? */
if (tupcxt == NULL)
{
@@ -306,7 +341,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
break;
}
- brinRevmapTerminate(revmap);
if (BufferIsValid(buf))
ReleaseBuffer(buf);
MemoryContextSwitchTo(oldcxt);
@@ -316,6 +350,24 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
return false;
}
+/*
+ * Callback to clean up the BrinInsertState once all tuple inserts are done.
+ */
+void
+brininsertcleanup(IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
+
+ Assert(bistate);
+ /*
+ * Clean up the revmap. Note that the brinDesc has already been cleaned up
+ * as part of its own memory context.
+ */
+ brinRevmapTerminate(bistate->bs_rmAccess);
+ bistate->bs_rmAccess = NULL;
+ bistate->bs_desc = NULL;
+}
+
/*
* Initialize state for a BRIN index scan.
*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 437f24753c0..56223305442 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -64,6 +64,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = ginbuild;
amroutine->ambuildempty = ginbuildempty;
amroutine->aminsert = gininsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = ginbulkdelete;
amroutine->amvacuumcleanup = ginvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 516465f8b7d..c42746130cc 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -86,6 +86,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = gistbuild;
amroutine->ambuildempty = gistbuildempty;
amroutine->aminsert = gistinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = gistbulkdelete;
amroutine->amvacuumcleanup = gistvacuumcleanup;
amroutine->amcanreturn = gistcanreturn;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index fc5d97f606e..cba15cde0d7 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -83,6 +83,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = hashbuild;
amroutine->ambuildempty = hashbuildempty;
amroutine->aminsert = hashinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = hashbulkdelete;
amroutine->amvacuumcleanup = hashvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b25b03f7abc..55b9c18369a 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -196,6 +196,21 @@ index_insert(Relation indexRelation,
indexInfo);
}
+/* -------------------------
+ * index_insert_cleanup - clean up after all index inserts are done.
+ * -------------------------
+ */
+void
+index_insert_cleanup(Relation indexRelation,
+ IndexInfo *indexInfo)
+{
+ RELATION_CHECKS;
+ Assert(indexInfo);
+
+ if (indexRelation->rd_indam->aminsertcleanup)
+ indexRelation->rd_indam->aminsertcleanup(indexInfo);
+}
+
/*
* index_beginscan - start a scan of an index with amgettuple
*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4553aaee531..2753566437f 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -122,6 +122,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = btbuild;
amroutine->ambuildempty = btbuildempty;
amroutine->aminsert = btinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = btbulkdelete;
amroutine->amvacuumcleanup = btvacuumcleanup;
amroutine->amcanreturn = btcanreturn;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 190e4f76a9e..b8efcf5cfb2 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -70,6 +70,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->ambuild = spgbuild;
amroutine->ambuildempty = spgbuildempty;
amroutine->aminsert = spginsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = spgbulkdelete;
amroutine->amvacuumcleanup = spgvacuumcleanup;
amroutine->amcanreturn = spgcanreturn;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 1d82b64b897..63699fdd93d 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -233,15 +233,20 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
int i;
int numIndices;
RelationPtr indexDescs;
+ IndexInfo **indexInfos;
numIndices = resultRelInfo->ri_NumIndices;
indexDescs = resultRelInfo->ri_IndexRelationDescs;
+ indexInfos = resultRelInfo->ri_IndexRelationInfo;
for (i = 0; i < numIndices; i++)
{
if (indexDescs[i] == NULL)
continue; /* shouldn't happen? */
+ /* Give the index a chance to do some post-insert cleanup */
+ index_insert_cleanup(indexDescs[i], indexInfos[i]);
+
/* Drop lock acquired by ExecOpenIndices */
index_close(indexDescs[i], RowExclusiveLock);
}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 4476ff7fba1..d29721eaf1e 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -113,6 +113,8 @@ typedef bool (*aminsert_function) (Relation indexRelation,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
+
/* bulk delete */
typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
@@ -261,6 +263,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 97ddc925b27..ed231e208eb 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -96,6 +96,7 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void brininsertcleanup(struct IndexInfo *indexInfo);
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index a3087956654..2cf9dc5dca9 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -148,6 +148,8 @@ extern bool index_insert(Relation indexRelation,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void index_insert_cleanup(Relation indexRelation,
+ struct IndexInfo *indexInfo);
extern IndexScanDesc index_beginscan(Relation heapRelation,
Relation indexRelation,
--
2.34.1
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-08-05 07:53 Soumyadeep Chakraborty <[email protected]>
parent: Soumyadeep Chakraborty <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-08-05 07:53 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
Created an entry for the Sep CF: https://commitfest.postgresql.org/44/4484/
Regards,
Soumyadeep (VMware)
On Sat, Jul 29, 2023 at 9:28 AM Soumyadeep Chakraborty
<[email protected]> wrote:
>
> Attached v4 of the patch, rebased against latest HEAD.
>
> Regards,
> Soumyadeep (VMware)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-09-04 23:43 Soumyadeep Chakraborty <[email protected]>
parent: Soumyadeep Chakraborty <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-09-04 23:43 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
Rebased against latest HEAD.
Regards,
Soumyadeep (VMware)
Attachments:
[text/x-patch] v5-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch (13.7K, ../../CAE-ML+_9Adp8rRfZ9FP4ODtQZLZcMaBvae5ACO9H+-FZdk_v9w@mail.gmail.com/2-v5-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch)
download | inline diff:
From 94a8acd3125aa4a613c238e435ed78dba9f40625 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty <[email protected]>
Date: Sat, 29 Jul 2023 09:17:49 -0700
Subject: [PATCH v5 1/1] Reuse revmap and brin desc in brininsert
brininsert() used to have code that performed per-tuple initialization
of the revmap. That had some overhead.
To mitigate, we introduce a new AM callback to clean up state at the end
of all inserts, and perform the revmap cleanup there.
---
contrib/bloom/blutils.c | 1 +
doc/src/sgml/indexam.sgml | 14 +++++-
src/backend/access/brin/brin.c | 74 +++++++++++++++++++++++-----
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/index/indexam.c | 15 ++++++
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/executor/execIndexing.c | 5 ++
src/include/access/amapi.h | 3 ++
src/include/access/brin_internal.h | 1 +
src/include/access/genam.h | 2 +
13 files changed, 108 insertions(+), 12 deletions(-)
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index f23fbb1d9e0..4830cb3fee6 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -131,6 +131,7 @@ blhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = blbuild;
amroutine->ambuildempty = blbuildempty;
amroutine->aminsert = blinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = blbulkdelete;
amroutine->amvacuumcleanup = blvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index e813e2b620a..17f7d6597f1 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -139,6 +139,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
@@ -355,11 +356,22 @@ aminsert (Relation indexRelation,
within an SQL statement, it can allocate space
in <literal>indexInfo->ii_Context</literal> and store a pointer to the
data in <literal>indexInfo->ii_AmCache</literal> (which will be NULL
- initially).
+ initially). If the data cached contains structures that can be simply pfree'd,
+ they will implicitly be pfree'd. But, if it contains more complex data, such
+ as Buffers or TupleDescs, additional cleanup is necessary. This additional
+ cleanup can be performed in <literal>indexinsertcleanup</literal>.
</para>
<para>
<programlisting>
+bool
+aminsertcleanup (IndexInfo *indexInfo);
+</programlisting>
+ Clean up state that was maintained across successive inserts in
+ <literal>indexInfo->ii_AmCache</literal>. This is useful if the data
+ contained is complex - like Buffers or TupleDescs which need additional
+ cleanup, unlike simple structures that will be implicitly pfree'd.
+<programlisting>
IndexBulkDeleteResult *
ambulkdelete (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index d4fec654bb6..76927beb0ec 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -58,6 +58,17 @@ typedef struct BrinBuildState
BrinMemTuple *bs_dtuple;
} BrinBuildState;
+/*
+ * We use a BrinInsertState to capture running state spanning multiple
+ * brininsert invocations, within the same command.
+ */
+typedef struct BrinInsertState
+{
+ BrinRevmap *bs_rmAccess;
+ BrinDesc *bs_desc;
+ BlockNumber bs_pages_per_range;
+} BrinInsertState;
+
/*
* Struct used as "opaque" during index scans
*/
@@ -72,6 +83,7 @@ typedef struct BrinOpaque
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
+static BrinInsertState *initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo);
static void terminate_brin_buildstate(BrinBuildState *state);
static void brinsummarize(Relation index, Relation heapRel, BlockNumber pageRange,
bool include_partial, double *numSummarized, double *numExisting);
@@ -117,6 +129,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = brinbuild;
amroutine->ambuildempty = brinbuildempty;
amroutine->aminsert = brininsert;
+ amroutine->aminsertcleanup = brininsertcleanup;
amroutine->ambulkdelete = brinbulkdelete;
amroutine->amvacuumcleanup = brinvacuumcleanup;
amroutine->amcanreturn = NULL;
@@ -140,6 +153,28 @@ brinhandler(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(amroutine);
}
+/*
+ * Initialize a BrinInsertState to maintain state to be used across multiple
+ * tuple inserts, within the same command.
+ */
+static BrinInsertState *
+initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(indexInfo->ii_Context);
+ bistate = palloc0(sizeof(BrinInsertState));
+ bistate->bs_desc = brin_build_desc(idxRel);
+ bistate->bs_rmAccess = brinRevmapInitialize(idxRel,
+ &bistate->bs_pages_per_range,
+ NULL);
+ indexInfo->ii_AmCache = bistate;
+ MemoryContextSwitchTo(oldcxt);
+
+ return bistate;
+}
+
/*
* A tuple in the heap is being inserted. To keep a brin index up to date,
* we need to obtain the relevant index tuple and compare its stored values
@@ -162,14 +197,22 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
BlockNumber pagesPerRange;
BlockNumber origHeapBlk;
BlockNumber heapBlk;
- BrinDesc *bdesc = (BrinDesc *) indexInfo->ii_AmCache;
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
BrinRevmap *revmap;
+ BrinDesc *bdesc;
Buffer buf = InvalidBuffer;
MemoryContext tupcxt = NULL;
MemoryContext oldcxt = CurrentMemoryContext;
bool autosummarize = BrinGetAutoSummarize(idxRel);
- revmap = brinRevmapInitialize(idxRel, &pagesPerRange, NULL);
+ if (!bistate)
+ {
+ /* First time through in this statement? */
+ bistate = initialize_brin_insertstate(idxRel, indexInfo);
+ }
+ revmap = bistate->bs_rmAccess;
+ bdesc = bistate->bs_desc;
+ pagesPerRange = bistate->bs_pages_per_range;
/*
* origHeapBlk is the block number where the insertion occurred. heapBlk
@@ -228,14 +271,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
if (!brtup)
break;
- /* First time through in this statement? */
- if (bdesc == NULL)
- {
- MemoryContextSwitchTo(indexInfo->ii_Context);
- bdesc = brin_build_desc(idxRel);
- indexInfo->ii_AmCache = (void *) bdesc;
- MemoryContextSwitchTo(oldcxt);
- }
/* First time through in this brininsert call? */
if (tupcxt == NULL)
{
@@ -306,7 +341,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
break;
}
- brinRevmapTerminate(revmap);
if (BufferIsValid(buf))
ReleaseBuffer(buf);
MemoryContextSwitchTo(oldcxt);
@@ -316,6 +350,24 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
return false;
}
+/*
+ * Callback to clean up the BrinInsertState once all tuple inserts are done.
+ */
+void
+brininsertcleanup(IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
+
+ Assert(bistate);
+ /*
+ * Clean up the revmap. Note that the brinDesc has already been cleaned up
+ * as part of its own memory context.
+ */
+ brinRevmapTerminate(bistate->bs_rmAccess);
+ bistate->bs_rmAccess = NULL;
+ bistate->bs_desc = NULL;
+}
+
/*
* Initialize state for a BRIN index scan.
*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 7a4cd93f301..a875c5d3d7a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -64,6 +64,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = ginbuild;
amroutine->ambuildempty = ginbuildempty;
amroutine->aminsert = gininsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = ginbulkdelete;
amroutine->amvacuumcleanup = ginvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index b1f19f6a8e6..bfdd8fef118 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -86,6 +86,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = gistbuild;
amroutine->ambuildempty = gistbuildempty;
amroutine->aminsert = gistinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = gistbulkdelete;
amroutine->amvacuumcleanup = gistvacuumcleanup;
amroutine->amcanreturn = gistcanreturn;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index fc5d97f606e..cba15cde0d7 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -83,6 +83,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = hashbuild;
amroutine->ambuildempty = hashbuildempty;
amroutine->aminsert = hashinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = hashbulkdelete;
amroutine->amvacuumcleanup = hashvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b25b03f7abc..55b9c18369a 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -196,6 +196,21 @@ index_insert(Relation indexRelation,
indexInfo);
}
+/* -------------------------
+ * index_insert_cleanup - clean up after all index inserts are done.
+ * -------------------------
+ */
+void
+index_insert_cleanup(Relation indexRelation,
+ IndexInfo *indexInfo)
+{
+ RELATION_CHECKS;
+ Assert(indexInfo);
+
+ if (indexRelation->rd_indam->aminsertcleanup)
+ indexRelation->rd_indam->aminsertcleanup(indexInfo);
+}
+
/*
* index_beginscan - start a scan of an index with amgettuple
*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 62bc9917f13..bb60f97270c 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -122,6 +122,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = btbuild;
amroutine->ambuildempty = btbuildempty;
amroutine->aminsert = btinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = btbulkdelete;
amroutine->amvacuumcleanup = btvacuumcleanup;
amroutine->amcanreturn = btcanreturn;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 8f32e46fb83..5bd5dcac9db 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -70,6 +70,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->ambuild = spgbuild;
amroutine->ambuildempty = spgbuildempty;
amroutine->aminsert = spginsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = spgbulkdelete;
amroutine->amvacuumcleanup = spgvacuumcleanup;
amroutine->amcanreturn = spgcanreturn;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 1d82b64b897..63699fdd93d 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -233,15 +233,20 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
int i;
int numIndices;
RelationPtr indexDescs;
+ IndexInfo **indexInfos;
numIndices = resultRelInfo->ri_NumIndices;
indexDescs = resultRelInfo->ri_IndexRelationDescs;
+ indexInfos = resultRelInfo->ri_IndexRelationInfo;
for (i = 0; i < numIndices; i++)
{
if (indexDescs[i] == NULL)
continue; /* shouldn't happen? */
+ /* Give the index a chance to do some post-insert cleanup */
+ index_insert_cleanup(indexDescs[i], indexInfos[i]);
+
/* Drop lock acquired by ExecOpenIndices */
index_close(indexDescs[i], RowExclusiveLock);
}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 4476ff7fba1..d29721eaf1e 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -113,6 +113,8 @@ typedef bool (*aminsert_function) (Relation indexRelation,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
+
/* bulk delete */
typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
@@ -261,6 +263,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 97ddc925b27..ed231e208eb 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -96,6 +96,7 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void brininsertcleanup(struct IndexInfo *indexInfo);
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index a3087956654..2cf9dc5dca9 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -148,6 +148,8 @@ extern bool index_insert(Relation indexRelation,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void index_insert_cleanup(Relation indexRelation,
+ struct IndexInfo *indexInfo);
extern IndexScanDesc index_beginscan(Relation heapRelation,
Relation indexRelation,
--
2.34.1
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-03 18:35 Tomas Vondra <[email protected]>
parent: Soumyadeep Chakraborty <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tomas Vondra @ 2023-11-03 18:35 UTC (permalink / raw)
To: Soumyadeep Chakraborty <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
Hi,
I took a look at this patch today. I had to rebase the patch, due to
some minor bitrot related to 9f0602539d (but nothing major). I also did
a couple tiny cosmetic tweaks, but other than that the patch seems OK.
See the attached v6.
I did some simple performance tests too, similar to those in the initial
message:
CREATE UNLOGGED TABLE heap (i int);
CREATE INDEX ON heap USING brin(i) WITH (pages_per_range=1);
--
TRUNCATE heap;
INSERT INTO heap SELECT 1 FROM generate_series(1, 20000000);
And the results look like this (5 runs each):
master: 16448.338 16066.473 16039.166 16067.420 16080.066
patched: 13260.065 13229.800 13254.454 13265.479 13273.693
So that's a nice improvement, even though enabling WAL will make the
relative speedup somewhat smaller.
The one thing I'm not entirely sure about is adding new stuff to the
IndexAmRoutine. I don't think we want to end up with too many callbacks
that all AMs have to initialize etc. I can't think of a different/better
way to do this, though.
Barring objections, I'll try to push this early next week, after another
round of cleanup.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] v6-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch (13.7K, ../../[email protected]/2-v6-0001-Reuse-revmap-and-brin-desc-in-brininsert.patch)
download | inline diff:
From b1c47527132f0e8c39ff221b6b5adcc9678ba6ce Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Fri, 3 Nov 2023 19:17:21 +0100
Subject: [PATCH v6] Reuse revmap and brin desc in brininsert
brininsert() used to have code that performed per-tuple initialization
of the revmap. That had some overhead.
To mitigate, we introduce a new AM callback to clean up state at the end
of all inserts, and perform the revmap cleanup there.
---
contrib/bloom/blutils.c | 1 +
doc/src/sgml/indexam.sgml | 14 +++++-
src/backend/access/brin/brin.c | 74 +++++++++++++++++++++++-----
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/index/indexam.c | 15 ++++++
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/executor/execIndexing.c | 5 ++
src/include/access/amapi.h | 4 ++
src/include/access/brin_internal.h | 1 +
src/include/access/genam.h | 2 +
13 files changed, 109 insertions(+), 12 deletions(-)
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index f23fbb1d9e0..4830cb3fee6 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -131,6 +131,7 @@ blhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = blbuild;
amroutine->ambuildempty = blbuildempty;
amroutine->aminsert = blinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = blbulkdelete;
amroutine->amvacuumcleanup = blvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 30eda37afa8..a36d2eaebe7 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -139,6 +139,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
@@ -359,11 +360,22 @@ aminsert (Relation indexRelation,
within an SQL statement, it can allocate space
in <literal>indexInfo->ii_Context</literal> and store a pointer to the
data in <literal>indexInfo->ii_AmCache</literal> (which will be NULL
- initially).
+ initially). If the data cached contains structures that can be simply pfree'd,
+ they will implicitly be pfree'd. But, if it contains more complex data, such
+ as Buffers or TupleDescs, additional cleanup is necessary. This additional
+ cleanup can be performed in <literal>indexinsertcleanup</literal>.
</para>
<para>
<programlisting>
+bool
+aminsertcleanup (IndexInfo *indexInfo);
+</programlisting>
+ Clean up state that was maintained across successive inserts in
+ <literal>indexInfo->ii_AmCache</literal>. This is useful if the data
+ contained is complex - like Buffers or TupleDescs which need additional
+ cleanup, unlike simple structures that will be implicitly pfree'd.
+<programlisting>
IndexBulkDeleteResult *
ambulkdelete (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 25338a90e29..fd99b6eb11e 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -58,6 +58,17 @@ typedef struct BrinBuildState
BrinMemTuple *bs_dtuple;
} BrinBuildState;
+/*
+ * We use a BrinInsertState to capture running state spanning multiple
+ * brininsert invocations, within the same command.
+ */
+typedef struct BrinInsertState
+{
+ BrinRevmap *bs_rmAccess;
+ BrinDesc *bs_desc;
+ BlockNumber bs_pages_per_range;
+} BrinInsertState;
+
/*
* Struct used as "opaque" during index scans
*/
@@ -72,6 +83,7 @@ typedef struct BrinOpaque
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
+static BrinInsertState *initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo);
static void terminate_brin_buildstate(BrinBuildState *state);
static void brinsummarize(Relation index, Relation heapRel, BlockNumber pageRange,
bool include_partial, double *numSummarized, double *numExisting);
@@ -117,6 +129,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = brinbuild;
amroutine->ambuildempty = brinbuildempty;
amroutine->aminsert = brininsert;
+ amroutine->aminsertcleanup = brininsertcleanup;
amroutine->ambulkdelete = brinbulkdelete;
amroutine->amvacuumcleanup = brinvacuumcleanup;
amroutine->amcanreturn = NULL;
@@ -140,6 +153,27 @@ brinhandler(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(amroutine);
}
+/*
+ * Initialize a BrinInsertState to maintain state to be used across multiple
+ * tuple inserts, within the same command.
+ */
+static BrinInsertState *
+initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(indexInfo->ii_Context);
+ bistate = palloc0(sizeof(BrinInsertState));
+ bistate->bs_desc = brin_build_desc(idxRel);
+ bistate->bs_rmAccess = brinRevmapInitialize(idxRel,
+ &bistate->bs_pages_per_range);
+ indexInfo->ii_AmCache = bistate;
+ MemoryContextSwitchTo(oldcxt);
+
+ return bistate;
+}
+
/*
* A tuple in the heap is being inserted. To keep a brin index up to date,
* we need to obtain the relevant index tuple and compare its stored values
@@ -162,14 +196,23 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
BlockNumber pagesPerRange;
BlockNumber origHeapBlk;
BlockNumber heapBlk;
- BrinDesc *bdesc = (BrinDesc *) indexInfo->ii_AmCache;
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
BrinRevmap *revmap;
+ BrinDesc *bdesc;
Buffer buf = InvalidBuffer;
MemoryContext tupcxt = NULL;
MemoryContext oldcxt = CurrentMemoryContext;
bool autosummarize = BrinGetAutoSummarize(idxRel);
- revmap = brinRevmapInitialize(idxRel, &pagesPerRange);
+ if (!bistate)
+ {
+ /* First time through in this statement? */
+ bistate = initialize_brin_insertstate(idxRel, indexInfo);
+ }
+
+ revmap = bistate->bs_rmAccess;
+ bdesc = bistate->bs_desc;
+ pagesPerRange = bistate->bs_pages_per_range;
/*
* origHeapBlk is the block number where the insertion occurred. heapBlk
@@ -228,14 +271,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
if (!brtup)
break;
- /* First time through in this statement? */
- if (bdesc == NULL)
- {
- MemoryContextSwitchTo(indexInfo->ii_Context);
- bdesc = brin_build_desc(idxRel);
- indexInfo->ii_AmCache = (void *) bdesc;
- MemoryContextSwitchTo(oldcxt);
- }
/* First time through in this brininsert call? */
if (tupcxt == NULL)
{
@@ -306,7 +341,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
break;
}
- brinRevmapTerminate(revmap);
if (BufferIsValid(buf))
ReleaseBuffer(buf);
MemoryContextSwitchTo(oldcxt);
@@ -316,6 +350,24 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
return false;
}
+/*
+ * Callback to clean up the BrinInsertState once all tuple inserts are done.
+ */
+void
+brininsertcleanup(IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
+
+ Assert(bistate);
+ /*
+ * Clean up the revmap. Note that the brinDesc has already been cleaned up
+ * as part of its own memory context.
+ */
+ brinRevmapTerminate(bistate->bs_rmAccess);
+ bistate->bs_rmAccess = NULL;
+ bistate->bs_desc = NULL;
+}
+
/*
* Initialize state for a BRIN index scan.
*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 7a4cd93f301..a875c5d3d7a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -64,6 +64,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = ginbuild;
amroutine->ambuildempty = ginbuildempty;
amroutine->aminsert = gininsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = ginbulkdelete;
amroutine->amvacuumcleanup = ginvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 8ef5fa03290..9a1bf8f66cb 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -86,6 +86,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = gistbuild;
amroutine->ambuildempty = gistbuildempty;
amroutine->aminsert = gistinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = gistbulkdelete;
amroutine->amvacuumcleanup = gistvacuumcleanup;
amroutine->amcanreturn = gistcanreturn;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 7a025f33cfe..6443ff21bda 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -83,6 +83,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = hashbuild;
amroutine->ambuildempty = hashbuildempty;
amroutine->aminsert = hashinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = hashbulkdelete;
amroutine->amvacuumcleanup = hashvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b25b03f7abc..597b763d1f6 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -196,6 +196,21 @@ index_insert(Relation indexRelation,
indexInfo);
}
+/* -------------------------
+ * index_insert_cleanup - clean up after all index inserts are done
+ * -------------------------
+ */
+void
+index_insert_cleanup(Relation indexRelation,
+ IndexInfo *indexInfo)
+{
+ RELATION_CHECKS;
+ Assert(indexInfo);
+
+ if (indexRelation->rd_indam->aminsertcleanup)
+ indexRelation->rd_indam->aminsertcleanup(indexInfo);
+}
+
/*
* index_beginscan - start a scan of an index with amgettuple
*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index a88b36a589a..0930f9b37e3 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -122,6 +122,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = btbuild;
amroutine->ambuildempty = btbuildempty;
amroutine->aminsert = btinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = btbulkdelete;
amroutine->amvacuumcleanup = btvacuumcleanup;
amroutine->amcanreturn = btcanreturn;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index c112e1e5dd4..30c00876a56 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -70,6 +70,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->ambuild = spgbuild;
amroutine->ambuildempty = spgbuildempty;
amroutine->aminsert = spginsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = spgbulkdelete;
amroutine->amvacuumcleanup = spgvacuumcleanup;
amroutine->amcanreturn = spgcanreturn;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 384b39839a0..2fa2118f3c2 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -233,15 +233,20 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
int i;
int numIndices;
RelationPtr indexDescs;
+ IndexInfo **indexInfos;
numIndices = resultRelInfo->ri_NumIndices;
indexDescs = resultRelInfo->ri_IndexRelationDescs;
+ indexInfos = resultRelInfo->ri_IndexRelationInfo;
for (i = 0; i < numIndices; i++)
{
if (indexDescs[i] == NULL)
continue; /* shouldn't happen? */
+ /* Give the index a chance to do some post-insert cleanup */
+ index_insert_cleanup(indexDescs[i], indexInfos[i]);
+
/* Drop lock acquired by ExecOpenIndices */
index_close(indexDescs[i], RowExclusiveLock);
}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 995725502a6..244459587fc 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -113,6 +113,9 @@ typedef bool (*aminsert_function) (Relation indexRelation,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+/* cleanup after insert */
+typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
+
/* bulk delete */
typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
@@ -261,6 +264,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 97ddc925b27..ed231e208eb 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -96,6 +96,7 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void brininsertcleanup(struct IndexInfo *indexInfo);
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index f31dec6ee0f..80dc8d54066 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -148,6 +148,8 @@ extern bool index_insert(Relation indexRelation,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void index_insert_cleanup(Relation indexRelation,
+ struct IndexInfo *indexInfo);
extern IndexScanDesc index_beginscan(Relation heapRelation,
Relation indexRelation,
--
2.41.0
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-03 19:16 Matthias van de Meent <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Matthias van de Meent @ 2023-11-03 19:16 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Soumyadeep Chakraborty <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
On Fri, 3 Nov 2023 at 19:37, Tomas Vondra <[email protected]> wrote:
>
> Hi,
>
> I took a look at this patch today. I had to rebase the patch, due to
> some minor bitrot related to 9f0602539d (but nothing major). I also did
> a couple tiny cosmetic tweaks, but other than that the patch seems OK.
> See the attached v6.
> [...]
> Barring objections, I'll try to push this early next week, after another
> round of cleanup.
No hard objections: The principle looks fine.
I do think we should choose a better namespace than bs_* for the
fields of BrinInsertState, as BrinBuildState already uses the bs_*
namespace for its fields in the same file, but that's only cosmetic.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-04 18:58 Soumyadeep Chakraborty <[email protected]>
parent: Matthias van de Meent <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-11-04 18:58 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
On Fri, 3 Nov 2023 at 19:37, Tomas Vondra <[email protected]> wrote:
> The one thing I'm not entirely sure about is adding new stuff to the
> IndexAmRoutine. I don't think we want to end up with too many callbacks
> that all AMs have to initialize etc. I can't think of a different/better
> way to do this, though.
Yes there is not really an alternative. Also, aminsertcleanup() is very similar
to amvacuumcleanup(), so it is not awkward. Why should vacuum be an
exclusive VIP? :)
And there are other indexam callbacks that not every AM implements. So this
addition is not unprecedented in that sense.
> Barring objections, I'll try to push this early next week, after another
> round of cleanup.
Many thanks for resurrecting this patch!
On Fri, Nov 3, 2023 at 12:16PM Matthias van de Meent
<[email protected]> wrote:
>
> I do think we should choose a better namespace than bs_* for the
> fields of BrinInsertState, as BrinBuildState already uses the bs_*
> namespace for its fields in the same file, but that's only cosmetic.
>
bis_* then.
Regards,
Soumyadeep (VMware)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-25 20:06 Tomas Vondra <[email protected]>
parent: Soumyadeep Chakraborty <[email protected]>
0 siblings, 2 replies; 13+ messages in thread
From: Tomas Vondra @ 2023-11-25 20:06 UTC (permalink / raw)
To: Soumyadeep Chakraborty <[email protected]>; Matthias van de Meent <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
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.
Thanks for the patch!
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-25 22:33 Ashwin Agrawal <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 1 reply; 13+ messages in thread
From: Ashwin Agrawal @ 2023-11-25 22:33 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Matthias van de Meent <[email protected]>; Soumyadeep Chakraborty <[email protected]>; pgsql-hackers
On Sat, Nov 25, 2023 at 12:06 PM Tomas Vondra <[email protected]>
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.
Thanks a lot Tomas for helping to drive the patch to completion iteratively
and realizing the benefits.
- Ashwin
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-25 23:23 Soumyadeep Chakraborty <[email protected]>
parent: Ashwin Agrawal <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-11-25 23:23 UTC (permalink / raw)
To: Ashwin Agrawal <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Alvaro Herrera <[email protected]>; Matthias van de Meent <[email protected]>; pgsql-hackers
Thanks a lot for reviewing and pushing! :)
Regards,
Soumyadeep (VMware)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-27 05:28 Richard Guo <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 1 reply; 13+ messages in thread
From: Richard Guo @ 2023-11-27 05:28 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Soumyadeep Chakraborty <[email protected]>; Matthias van de Meent <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
On Sun, Nov 26, 2023 at 4:06 AM Tomas Vondra <[email protected]>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: brininsert optimization opportunity
@ 2023-11-27 05:52 Soumyadeep Chakraborty <[email protected]>
parent: Richard Guo <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Soumyadeep Chakraborty @ 2023-11-27 05:52 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Matthias van de Meent <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers; Ashwin Agrawal <[email protected]>
On Sun, Nov 26, 2023 at 9:28 PM Richard Guo <[email protected]> wrote:
>
>
> On Sun, Nov 26, 2023 at 4:06 AM Tomas Vondra <[email protected]> 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
>
I wasn't able to repro the issue on 86b64bafc19c4c60136a4038d2a8d1e6eecc59f2.
with UPDATE/INSERT:
postgres=# drop table a;
DROP TABLE
postgres=# create table a(i int);
CREATE TABLE
postgres=# create index on a using brin(i);
CREATE INDEX
postgres=# insert into a select 1 where 1!=1;
INSERT 0 0
postgres=# update a set i = 2 where i = 0;
UPDATE 0
postgres=# update a set i = a.i;
UPDATE 0
This could be because since c5b7ba4e67aeb5d6f824b74f94114d99ed6e42b7,
we have moved ExecOpenIndices()
from ExecInitModifyTable() to ExecInsert(). Since we never open the
indices if nothing is
inserted, we would never attempt to close them with ExecCloseIndices()
while the ii_AmCache
is NULL (which is what causes this assertion failure).
However, it is possible to repro the issue with:
# create empty file
touch /tmp/a.csv
postgres=# create table a(i int);
CREATE TABLE
postgres=# create index on a using brin(i);
CREATE INDEX
postgres=# copy a from '/tmp/a.csv';
TRAP: failed Assert("bistate"), File: "brin.c", Line: 362, PID: 995511
> So I wonder if we should check 'bistate' and do the clean up only if
there is an available one, something like below.
Yes, this is the right way to go. Thanks for reporting!
Regards,
Soumyadeep (VMware)
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2023-11-27 05:52 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 07:25 [PATCH v8 1/4] Add READ_REPLICATION_SLOT command Michael Paquier <[email protected]>
2023-07-05 18:57 Re: brininsert optimization opportunity Soumyadeep Chakraborty <[email protected]>
2023-07-29 16:28 ` Re: brininsert optimization opportunity Soumyadeep Chakraborty <[email protected]>
2023-08-05 07:53 ` Re: brininsert optimization opportunity Soumyadeep Chakraborty <[email protected]>
2023-09-04 23:43 ` Re: brininsert optimization opportunity Soumyadeep Chakraborty <[email protected]>
2023-11-03 18:35 ` Re: brininsert optimization opportunity Tomas Vondra <[email protected]>
2023-11-03 19:16 ` Re: brininsert optimization opportunity Matthias van de Meent <[email protected]>
2023-11-04 18:58 ` Re: brininsert optimization opportunity Soumyadeep Chakraborty <[email protected]>
2023-11-25 20:06 ` Re: brininsert optimization opportunity Tomas Vondra <[email protected]>
2023-11-25 22:33 ` Re: brininsert optimization opportunity Ashwin Agrawal <[email protected]>
2023-11-25 23:23 ` Re: brininsert optimization opportunity Soumyadeep Chakraborty <[email protected]>
2023-11-27 05:28 ` Re: brininsert optimization opportunity Richard Guo <[email protected]>
2023-11-27 05:52 ` Re: brininsert optimization opportunity Soumyadeep Chakraborty <[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