pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedmake ExecInsertIndexTuples arguments less bad
5+ messages / 3 participants
[nested] [flat]
* make ExecInsertIndexTuples arguments less bad
@ 2026-02-11 19:07 Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 5+ messages in thread
From: Álvaro Herrera @ 2026-02-11 19:07 UTC (permalink / raw)
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Hello,
The arguments to ExecInsertIndexTuples() are rather unhelpful to read;
patching them is messy and hard to follow. How about we reuse the
pattern we used in commit f831d4accda0 to make them less bad?
I think the code is much nicer to read this way.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it." (New York Times, about Microsoft PowerPoint)
Attachments:
[text/x-diff] 0001-Split-out-ExecInsertIndexTuples-arguments.patch (12.2K, ../../202602111846.xpvuccb3inbx@alvherre.pgsql/2-0001-Split-out-ExecInsertIndexTuples-arguments.patch)
download | inline diff:
From 7732ab00600b5b58b27f2e70b9c7618b690d627c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Wed, 11 Feb 2026 17:35:34 +0100
Subject: [PATCH] Split out ExecInsertIndexTuples() arguments
The way we call this function is somewhat messy, and further patches
make it messier. We can simplify the way it looks by defining a struct
that carries some of its arguments.
This is modelled after the change to ArchiveEntry() in commit
f831d4accda0.
---
src/include/executor/executor.h | 16 ++++++--
src/backend/executor/execIndexing.c | 53 +++++++++++++-------------
src/backend/executor/nodeModifyTable.c | 19 +++++----
src/backend/executor/execReplication.c | 18 +++++----
src/backend/commands/copyfrom.c | 10 ++---
src/tools/pgindent/typedefs.list | 1 +
6 files changed, 61 insertions(+), 56 deletions(-)
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 55a7d930d26..e02c88b422d 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -739,12 +739,20 @@ extern Bitmapset *ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate);
*/
extern void ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative);
extern void ExecCloseIndices(ResultRelInfo *resultRelInfo);
+
+/* Options for ExecInsertIndexTuples */
+typedef struct EIITOptions
+{
+ bool is_update; /* is it an update? */
+ bool no_dupe_error; /* no throw in case of duplicates */
+ bool only_summarizing; /* only affect summarizing indexes */
+ List *arbiter_indexes; /* indexes to check for dupes; NIL=all */
+ bool *spec_conflict; /* output: was a conflict detected? */
+} EIITOptions;
+#define EIIT_OPTS(...) &(EIITOptions){__VA_ARGS__}
extern List *ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate,
- bool update,
- bool noDupErr,
- bool *specConflict, List *arbiterIndexes,
- bool onlySummarizing);
+ EIITOptions *options);
extern bool ExecCheckIndexConstraints(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
EState *estate, ItemPointer conflictTid,
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 6ae0f959592..1f0c4c3f61a 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -276,18 +276,18 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
* into all the relations indexing the result relation
* when a heap tuple is inserted into the result relation.
*
- * When 'update' is true and 'onlySummarizing' is false,
+ * When options->is_update is true and options->only_summarizing is false,
* executor is performing an UPDATE that could not use an
* optimization like heapam's HOT (in more general terms a
* call to table_tuple_update() took place and set
* 'update_indexes' to TU_All). Receiving this hint makes
* us consider if we should pass down the 'indexUnchanged'
* hint in turn. That's something that we figure out for
- * each index_insert() call iff 'update' is true.
- * (When 'update' is false we already know not to pass the
+ * each index_insert() call iff options->is_update is true.
+ * (When options->is_update is false we already know not to pass the
* hint to any index.)
*
- * If onlySummarizing is set, an equivalent optimization to
+ * If options->only_summarizing is set, an equivalent optimization to
* HOT has been applied and any updated columns are indexed
* only by summarizing indexes (or in more general terms a
* call to table_tuple_update() took place and set
@@ -298,23 +298,21 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
* Unique and exclusion constraints are enforced at the same
* time. This returns a list of index OIDs for any unique or
* exclusion constraints that are deferred and that had
- * potential (unconfirmed) conflicts. (if noDupErr == true,
+ * potential (unconfirmed) conflicts. (If options->no_dupe_error is true,
* the same is done for non-deferred constraints, but report
- * if conflict was speculative or deferred conflict to caller)
+ * if conflict was speculative or deferred conflict to caller
+ * by setting options->spec_conflict).
*
- * If 'arbiterIndexes' is nonempty, noDupErr applies only to
- * those indexes. NIL means noDupErr applies to all indexes.
+ * If options->arbiter_indexes is nonempty, options->no_dupe_error
+ * applies only to those indexes. NIL means no_dupe_error applies to
+ * all indexes.
* ----------------------------------------------------------------
*/
List *
ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
EState *estate,
- bool update,
- bool noDupErr,
- bool *specConflict,
- List *arbiterIndexes,
- bool onlySummarizing)
+ EIITOptions *options)
{
ItemPointer tupleid = &slot->tts_tid;
List *result = NIL;
@@ -374,7 +372,7 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
* Skip processing of non-summarizing indexes if we only update
* summarizing indexes
*/
- if (onlySummarizing && !indexInfo->ii_Summarizing)
+ if (options->only_summarizing && !indexInfo->ii_Summarizing)
continue;
/* Check for partial index */
@@ -408,10 +406,10 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
values,
isnull);
- /* Check whether to apply noDupErr to this index */
- applyNoDupErr = noDupErr &&
- (arbiterIndexes == NIL ||
- list_member_oid(arbiterIndexes,
+ /* Check whether to apply no_dupe_error to this index */
+ applyNoDupErr = options->no_dupe_error &&
+ (options->arbiter_indexes == NIL ||
+ list_member_oid(options->arbiter_indexes,
indexRelation->rd_index->indexrelid));
/*
@@ -441,10 +439,11 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
* index. If we're being called as part of an UPDATE statement,
* consider if the 'indexUnchanged' = true hint should be passed.
*/
- indexUnchanged = update && index_unchanged_by_update(resultRelInfo,
- estate,
- indexInfo,
- indexRelation);
+ indexUnchanged = options->is_update &&
+ index_unchanged_by_update(resultRelInfo,
+ estate,
+ indexInfo,
+ indexRelation);
satisfiesConstraint =
index_insert(indexRelation, /* index relation */
@@ -462,9 +461,9 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
* always insert first and then check. If the constraint is deferred,
* we check now anyway, but don't throw error on violation or wait for
* a conclusive outcome from a concurrent insertion; instead we'll
- * queue a recheck event. Similarly, noDupErr callers (speculative
- * inserters) will recheck later, and wait for a conclusive outcome
- * then.
+ * queue a recheck event. Similarly, no_dupe_error callers
+ * (speculative inserters) will recheck later, and wait for a
+ * conclusive outcome then.
*
* An index for an exclusion constraint can't also be UNIQUE (not an
* essential property, we just don't allow it in the grammar), so no
@@ -510,8 +509,8 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
* speculative conflict, since that always requires a restart.
*/
result = lappend_oid(result, RelationGetRelid(indexRelation));
- if (indexRelation->rd_index->indimmediate && specConflict)
- *specConflict = true;
+ if (indexRelation->rd_index->indimmediate && options->spec_conflict)
+ *options->spec_conflict = true;
}
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index f5e9d369940..b0be0e8fe98 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1197,10 +1197,10 @@ ExecInsert(ModifyTableContext *context,
/* insert index entries for tuple */
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false, true,
- &specConflict,
- arbiterIndexes,
- false);
+ slot, estate,
+ EIIT_OPTS(.no_dupe_error = true,
+ .spec_conflict = &specConflict,
+ .arbiter_indexes = arbiterIndexes));
/* adjust the tuple's state accordingly */
table_tuple_complete_speculative(resultRelationDesc, slot,
@@ -1238,9 +1238,8 @@ ExecInsert(ModifyTableContext *context,
/* insert index entries for tuple */
if (resultRelInfo->ri_NumIndices > 0)
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false,
- false, NULL, NIL,
- false);
+ slot, estate,
+ EIIT_OPTS());
}
}
@@ -2329,9 +2328,9 @@ ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt,
if (resultRelInfo->ri_NumIndices > 0 && (updateCxt->updateIndexes != TU_None))
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
slot, context->estate,
- true, false,
- NULL, NIL,
- (updateCxt->updateIndexes == TU_Summarizing));
+ EIIT_OPTS(.is_update = true,
+ .only_summarizing =
+ updateCxt->updateIndexes == TU_Summarizing));
/* AFTER ROW UPDATE Triggers */
ExecARUpdateTriggers(context->estate, resultRelInfo,
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 743b1ee2b28..039cca53705 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -847,10 +847,10 @@ ExecSimpleRelationInsert(ResultRelInfo *resultRelInfo,
if (resultRelInfo->ri_NumIndices > 0)
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false,
- conflictindexes ? true : false,
- &conflict,
- conflictindexes, false);
+ slot, estate,
+ EIIT_OPTS(.no_dupe_error = conflictindexes != NIL,
+ .spec_conflict = &conflict,
+ .arbiter_indexes = conflictindexes));
/*
* Checks the conflict indexes to fetch the conflicting local row and
@@ -944,10 +944,12 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
if (resultRelInfo->ri_NumIndices > 0 && (update_indexes != TU_None))
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, true,
- conflictindexes ? true : false,
- &conflict, conflictindexes,
- (update_indexes == TU_Summarizing));
+ slot, estate,
+ EIIT_OPTS(.is_update = true,
+ .no_dupe_error = conflictindexes != NIL,
+ .spec_conflict = &conflict,
+ .arbiter_indexes = conflictindexes,
+ .only_summarizing = (update_indexes == TU_Summarizing)));
/*
* Refer to the comments above the call to CheckAndReportConflict() in
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 25ee20b23db..c0597266a82 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -572,8 +572,8 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
cstate->cur_lineno = buffer->linenos[i];
recheckIndexes =
ExecInsertIndexTuples(resultRelInfo,
- buffer->slots[i], estate, false,
- false, NULL, NIL, false);
+ buffer->slots[i], estate,
+ EIIT_OPTS());
ExecARInsertTriggers(estate, resultRelInfo,
slots[i], recheckIndexes,
cstate->transition_capture);
@@ -1431,11 +1431,7 @@ CopyFrom(CopyFromState cstate)
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
myslot,
estate,
- false,
- false,
- NULL,
- NIL,
- false);
+ EIIT_OPTS());
}
/* AFTER ROW INSERT Triggers */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 39c76691c86..bd1f6e2fc44 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -695,6 +695,7 @@ DynamicZoneAbbrev
ECDerivesEntry
ECDerivesKey
EDGE
+EIITOptions
ENGINE
EOM_flatten_into_method
EOM_get_flat_size_method
--
2.47.3
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make ExecInsertIndexTuples arguments less bad
@ 2026-02-11 19:29 Fabrízio de Royes Mello <fabriziomello@gmail.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 5+ messages in thread
From: Fabrízio de Royes Mello @ 2026-02-11 19:29 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Feb 11, 2026 at 4:07 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> Hello,
>
> The arguments to ExecInsertIndexTuples() are rather unhelpful to read;
> patching them is messy and hard to follow. How about we reuse the
> pattern we used in commit f831d4accda0 to make them less bad?
> I think the code is much nicer to read this way.
>
>
Much better. LGTM!
--
Fabrízio de Royes Mello
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make ExecInsertIndexTuples arguments less bad
@ 2026-02-16 18:19 Álvaro Herrera <alvherre@kurilemu.de>
parent: Fabrízio de Royes Mello <fabriziomello@gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Álvaro Herrera @ 2026-02-16 18:19 UTC (permalink / raw)
To: Fabrízio de Royes Mello <fabriziomello@gmail.com>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Hello Fabrízio, hackers,
On 2026-Feb-11, Fabrízio de Royes Mello wrote:
> On Wed, Feb 11, 2026 at 4:07 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> > The arguments to ExecInsertIndexTuples() are rather unhelpful to read;
> > patching them is messy and hard to follow. How about we reuse the
> > pattern we used in commit f831d4accda0 to make them less bad?
> > I think the code is much nicer to read this way.
>
> Much better. LGTM!
Thanks for looking! However, I had second thoughts about this
formulation. Mainly, the fact that one of the output arguments is part
of the macro is kinda icky. So I decided that a simpler, less
innovative option is to just use a bits32 argument to carry the input
booleans, and let the output boolean be a separate argument (which I
also moved to appear last in the argument list, as we normally do).
This also means the list of indexes continues to be its own argument.
But, as I said, this is less innovative, which I think is mostly good.
And it definitely reads better than currently.
There might be places in executor.h to reuse the f831d4accda0 thingy,
but this is probably not it.
Thanks,
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
Attachments:
[text/x-diff] 0001-Use-bitmask-for-ExecInsertIndexTuples-options.patch (10.1K, ../../202602161805.jwp2f4s5svmp@alvherre.pgsql/2-0001-Use-bitmask-for-ExecInsertIndexTuples-options.patch)
download | inline diff:
From f98ee25bd47f1e03ce421032198819f3d94103ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 16 Feb 2026 18:49:53 +0100
Subject: [PATCH] Use bitmask for ExecInsertIndexTuples options
... instead of passing a bunch of separate booleans.
---
src/backend/commands/copyfrom.c | 11 ++++-----
src/backend/executor/execIndexing.c | 33 +++++++++++++-------------
src/backend/executor/execReplication.c | 24 ++++++++++++-------
src/backend/executor/nodeModifyTable.c | 20 +++++++++-------
src/include/executor/executor.h | 11 +++++----
5 files changed, 54 insertions(+), 45 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 25ee20b23db..ecb78b8adb9 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -572,8 +572,8 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
cstate->cur_lineno = buffer->linenos[i];
recheckIndexes =
ExecInsertIndexTuples(resultRelInfo,
- buffer->slots[i], estate, false,
- false, NULL, NIL, false);
+ buffer->slots[i], estate, NIL, 0,
+ NULL);
ExecARInsertTriggers(estate, resultRelInfo,
slots[i], recheckIndexes,
cstate->transition_capture);
@@ -1431,11 +1431,8 @@ CopyFrom(CopyFromState cstate)
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
myslot,
estate,
- false,
- false,
- NULL,
- NIL,
- false);
+ NIL, 0,
+ NULL);
}
/* AFTER ROW INSERT Triggers */
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index f0ba7eac87d..5fb79159930 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -276,18 +276,18 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
* into all the relations indexing the result relation
* when a heap tuple is inserted into the result relation.
*
- * When 'update' is true and 'onlySummarizing' is false,
+ * When EIIT_IS_UPDATE is set and EIIT_ONLY_SUMMARIZING isn't,
* executor is performing an UPDATE that could not use an
* optimization like heapam's HOT (in more general terms a
* call to table_tuple_update() took place and set
* 'update_indexes' to TU_All). Receiving this hint makes
* us consider if we should pass down the 'indexUnchanged'
* hint in turn. That's something that we figure out for
- * each index_insert() call iff 'update' is true.
- * (When 'update' is false we already know not to pass the
+ * each index_insert() call iff EIIT_IS_UPDATE is set.
+ * (When that flag is not set we already know not to pass the
* hint to any index.)
*
- * If onlySummarizing is set, an equivalent optimization to
+ * If EIIT_ONLY_SUMMARIZING is set, an equivalent optimization to
* HOT has been applied and any updated columns are indexed
* only by summarizing indexes (or in more general terms a
* call to table_tuple_update() took place and set
@@ -298,23 +298,21 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
* Unique and exclusion constraints are enforced at the same
* time. This returns a list of index OIDs for any unique or
* exclusion constraints that are deferred and that had
- * potential (unconfirmed) conflicts. (if noDupErr == true,
+ * potential (unconfirmed) conflicts. (if EIIT_NO_DUPE_ERROR,
* the same is done for non-deferred constraints, but report
* if conflict was speculative or deferred conflict to caller)
*
- * If 'arbiterIndexes' is nonempty, noDupErr applies only to
- * those indexes. NIL means noDupErr applies to all indexes.
+ * If 'arbiterIndexes' is nonempty, EIIT_NO_DUPE_ERROR applies only to
+ * those indexes. NIL means EIIT_NO_DUPE_ERROR applies to all indexes.
* ----------------------------------------------------------------
*/
List *
ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
EState *estate,
- bool update,
- bool noDupErr,
- bool *specConflict,
List *arbiterIndexes,
- bool onlySummarizing)
+ bits32 flags,
+ bool *specConflict)
{
ItemPointer tupleid = &slot->tts_tid;
List *result = NIL;
@@ -374,7 +372,7 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
* Skip processing of non-summarizing indexes if we only update
* summarizing indexes
*/
- if (onlySummarizing && !indexInfo->ii_Summarizing)
+ if ((flags & EIIT_ONLY_SUMMARIZING) && !indexInfo->ii_Summarizing)
continue;
/* Check for partial index */
@@ -409,7 +407,7 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
isnull);
/* Check whether to apply noDupErr to this index */
- applyNoDupErr = noDupErr &&
+ applyNoDupErr = (flags & EIIT_NO_DUPE_ERROR) &&
(arbiterIndexes == NIL ||
list_member_oid(arbiterIndexes,
indexRelation->rd_index->indexrelid));
@@ -441,10 +439,11 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
* index. If we're being called as part of an UPDATE statement,
* consider if the 'indexUnchanged' = true hint should be passed.
*/
- indexUnchanged = update && index_unchanged_by_update(resultRelInfo,
- estate,
- indexInfo,
- indexRelation);
+ indexUnchanged = ((flags & EIIT_IS_UPDATE) &&
+ index_unchanged_by_update(resultRelInfo,
+ estate,
+ indexInfo,
+ indexRelation));
satisfiesConstraint =
index_insert(indexRelation, /* index relation */
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 743b1ee2b28..cf22775871d 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -846,11 +846,14 @@ ExecSimpleRelationInsert(ResultRelInfo *resultRelInfo,
conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
if (resultRelInfo->ri_NumIndices > 0)
+ {
+ bits32 flags = conflictindexes != NIL ? EIIT_NO_DUPE_ERROR : 0;
+
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false,
- conflictindexes ? true : false,
- &conflict,
- conflictindexes, false);
+ slot, estate,
+ conflictindexes, flags,
+ &conflict);
+ }
/*
* Checks the conflict indexes to fetch the conflicting local row and
@@ -943,11 +946,16 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
if (resultRelInfo->ri_NumIndices > 0 && (update_indexes != TU_None))
+ {
+ bits32 flags = EIIT_IS_UPDATE;
+
+ flags |= conflictindexes != NIL ? EIIT_NO_DUPE_ERROR : 0;
+ flags |= update_indexes == TU_Summarizing ? EIIT_ONLY_SUMMARIZING : 0;
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, true,
- conflictindexes ? true : false,
- &conflict, conflictindexes,
- (update_indexes == TU_Summarizing));
+ slot, estate,
+ conflictindexes, flags,
+ &conflict);
+ }
/*
* Refer to the comments above the call to CheckAndReportConflict() in
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 6802fc13e95..5dbf866a1b9 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1226,10 +1226,10 @@ ExecInsert(ModifyTableContext *context,
/* insert index entries for tuple */
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false, true,
- &specConflict,
+ slot, estate,
arbiterIndexes,
- false);
+ EIIT_NO_DUPE_ERROR,
+ &specConflict);
/* adjust the tuple's state accordingly */
table_tuple_complete_speculative(resultRelationDesc, slot,
@@ -1267,9 +1267,8 @@ ExecInsert(ModifyTableContext *context,
/* insert index entries for tuple */
if (resultRelInfo->ri_NumIndices > 0)
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false,
- false, NULL, NIL,
- false);
+ slot, estate, NIL, 0,
+ NULL);
}
}
@@ -2356,11 +2355,14 @@ ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt,
/* insert index entries for tuple if necessary */
if (resultRelInfo->ri_NumIndices > 0 && (updateCxt->updateIndexes != TU_None))
+ {
+ bits32 flags = EIIT_IS_UPDATE;
+
+ flags |= updateCxt->updateIndexes == TU_Summarizing ? EIIT_ONLY_SUMMARIZING : 0;
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
slot, context->estate,
- true, false,
- NULL, NIL,
- (updateCxt->updateIndexes == TU_Summarizing));
+ NIL, flags, NULL);
+ }
/* AFTER ROW UPDATE Triggers */
ExecARUpdateTriggers(context->estate, resultRelInfo,
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 55a7d930d26..e788c5c7aee 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -739,12 +739,15 @@ extern Bitmapset *ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate);
*/
extern void ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative);
extern void ExecCloseIndices(ResultRelInfo *resultRelInfo);
+
+/* flags for ExecInsertIndexTuples */
+#define EIIT_IS_UPDATE (1<<0)
+#define EIIT_NO_DUPE_ERROR (1<<1)
+#define EIIT_ONLY_SUMMARIZING (1<<2)
extern List *ExecInsertIndexTuples(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate,
- bool update,
- bool noDupErr,
- bool *specConflict, List *arbiterIndexes,
- bool onlySummarizing);
+ List *arbiterIndexes, bits32 options,
+ bool *specConflict);
extern bool ExecCheckIndexConstraints(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
EState *estate, ItemPointer conflictTid,
--
2.47.3
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make ExecInsertIndexTuples arguments less bad
@ 2026-02-16 19:11 Andres Freund <andres@anarazel.de>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 5+ messages in thread
From: Andres Freund @ 2026-02-16 19:11 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Fabrízio de Royes Mello <fabriziomello@gmail.com>; Pg Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2026-02-16 19:19:33 +0100, Álvaro Herrera wrote:
> On 2026-Feb-11, Fabrízio de Royes Mello wrote:
> > On Wed, Feb 11, 2026 at 4:07 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > > The arguments to ExecInsertIndexTuples() are rather unhelpful to read;
> > > patching them is messy and hard to follow. How about we reuse the
> > > pattern we used in commit f831d4accda0 to make them less bad?
> > > I think the code is much nicer to read this way.
> >
> > Much better. LGTM!
>
> Thanks for looking! However, I had second thoughts about this
> formulation. Mainly, the fact that one of the output arguments is part
> of the macro is kinda icky.
Yea, that's not great.
> So I decided that a simpler, less innovative option is to just use a bits32
> argument to carry the input booleans, and let the output boolean be a
> separate argument (which I also moved to appear last in the argument list,
> as we normally do). This also means the list of indexes continues to be its
> own argument. But, as I said, this is less innovative, which I think is
> mostly good. And it definitely reads better than currently.
I think it does look better than before.
Personally I'd move the flags to before the slot and the estate before slot
(because it seems like options should come before the data and the most
frequently changing arguments should be later on), but that's an extremely
minor detail.
I'm mildly surprised about using bits32, we seem to be more widely just using
uint32 or such. I find a lot of the typedefs in c.h much more noise than
useful. But also, whatever.
> There might be places in executor.h to reuse the f831d4accda0 thingy,
> but this is probably not it.
FWIW, when passing <= 6 values, passing the arguments by reference in a struct
(rather than passing the struct by value), is likely to lead to less efficient
code.
> @@ -943,11 +946,16 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
> conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
>
> if (resultRelInfo->ri_NumIndices > 0 && (update_indexes != TU_None))
> + {
> + bits32 flags = EIIT_IS_UPDATE;
> +
> + flags |= conflictindexes != NIL ? EIIT_NO_DUPE_ERROR : 0;
> + flags |= update_indexes == TU_Summarizing ? EIIT_ONLY_SUMMARIZING : 0;
I'd just make these ifs, this is somewhat hard to read.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make ExecInsertIndexTuples arguments less bad
@ 2026-02-17 18:00 Álvaro Herrera <alvherre@kurilemu.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 5+ messages in thread
From: Álvaro Herrera @ 2026-02-17 18:00 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Fabrízio de Royes Mello <fabriziomello@gmail.com>; Pg Hackers <pgsql-hackers@lists.postgresql.org>
On 2026-Feb-16, Andres Freund wrote:
> Personally I'd move the flags to before the slot and the estate before slot
> (because it seems like options should come before the data and the most
> frequently changing arguments should be later on), but that's an extremely
> minor detail.
Good point, I pushed like that.
> I'm mildly surprised about using bits32, we seem to be more widely just using
> uint32 or such. I find a lot of the typedefs in c.h much more noise than
> useful. But also, whatever.
Yeah, I dunno, maybe we can retire those, but to me they say more
explicitly that the variable is not a number but rather a set of bits.
It doesn't make any actual difference, of course ... I've tried to prod
others to use bits32 instead of uint32 and have had little (read: none)
uptake, hah.
> > There might be places in executor.h to reuse the f831d4accda0 thingy,
> > but this is probably not it.
>
> FWIW, when passing <= 6 values, passing the arguments by reference in a struct
> (rather than passing the struct by value), is likely to lead to less efficient
> code.
Noted.
> > @@ -943,11 +946,16 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
> > conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
> >
> > if (resultRelInfo->ri_NumIndices > 0 && (update_indexes != TU_None))
> > + {
> > + bits32 flags = EIIT_IS_UPDATE;
> > +
> > + flags |= conflictindexes != NIL ? EIIT_NO_DUPE_ERROR : 0;
> > + flags |= update_indexes == TU_Summarizing ? EIIT_ONLY_SUMMARIZING : 0;
>
> I'd just make these ifs, this is somewhat hard to read.
Changed that way.
Thanks for reviewing!
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los cuentos de hadas no dan al niño su primera idea sobre los monstruos.
Lo que le dan es su primera idea de la posible derrota del monstruo."
(G. K. Chesterton)
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-02-17 18:00 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-11 19:07 make ExecInsertIndexTuples arguments less bad Álvaro Herrera <alvherre@kurilemu.de>
2026-02-11 19:29 ` Fabrízio de Royes Mello <fabriziomello@gmail.com>
2026-02-16 18:19 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-02-16 19:11 ` Andres Freund <andres@anarazel.de>
2026-02-17 18:00 ` Álvaro Herrera <alvherre@kurilemu.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox