Обсуждение: make ExecInsertIndexTuples arguments less bad

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)

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)

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

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)