Обсуждение: Why do we have MakeSingleTupleTableSlot instead of not using MakeTupleTableSlot?

Hi,

I wonder, is there a specific reason that MakeTupleTableSlot is
wrapped up in MakeSingleTupleTableSlot without doing anything than
just returning the slot created by MakeTupleTableSlot? Do we really
need MakeSingleTupleTableSlot? Can't we just use MakeTupleTableSlot
directly? Am I missing something?

I think we can avoid some unnecessary function call costs, for
instance when called 1000 times inside table_slot_create from
copyfrom.c or in some other places where MakeSingleTupleTableSlot is
called in a loop.

If it's okay to remove MakeSingleTupleTableSlot and use
MakeTupleTableSlot instead, we might have to change in a lot of
places. If we don't want to change in those many files, we could
rename MakeTupleTableSlot to MakeSingleTupleTableSlot and change it in
only a few places.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Feb 12, 2021 at 9:37 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> On Fri, Feb 12, 2021 at 5:44 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> Hi,
>>
>> I wonder, is there a specific reason that MakeTupleTableSlot is
>> wrapped up in MakeSingleTupleTableSlot without doing anything than
>> just returning the slot created by MakeTupleTableSlot? Do we really
>> need MakeSingleTupleTableSlot? Can't we just use MakeTupleTableSlot
>> directly? Am I missing something?
>>
>> I think we can avoid some unnecessary function call costs, for
>> instance when called 1000 times inside table_slot_create from
>> copyfrom.c or in some other places where MakeSingleTupleTableSlot is
>> called in a loop.
>>
>> If it's okay to remove MakeSingleTupleTableSlot and use
>> MakeTupleTableSlot instead, we might have to change in a lot of
>> places. If we don't want to change in those many files, we could
>> rename MakeTupleTableSlot to MakeSingleTupleTableSlot and change it in
>> only a few places.
>>
>> Thoughts?
>
> MakeSingleTupleTableSlot can be defined as a macro, calling MakeTupleTableSlot().

Right, we could as well have an inline function. My point was that why
do we need to wrap MakeTupleTableSlot inside MakeSingleTupleTableSlot
which just does nothing.  As I said upthread, how about renaming
MakeTupleTableSlot to
MakeSingleTupleTableSlot which requires  minimal changes?  Patch
attached. Both make check and make check-world passes on it. Please
have a look.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> Right, we could as well have an inline function. My point was that why
> do we need to wrap MakeTupleTableSlot inside MakeSingleTupleTableSlot
> which just does nothing.  As I said upthread, how about renaming
> MakeTupleTableSlot to
> MakeSingleTupleTableSlot which requires  minimal changes?

I'm disinclined to change this just to save one level of function call.
If you dig in the git history (see f92e8a4b5 in particular) you'll note
that the current version of MakeTupleTableSlot originated as code shared
between ExecAllocTableSlot and MakeSingleTupleTableSlot.  The fact that
the latter is currently just equivalent to that shared functionality is
something that happened later and might need to change again.

It is fair to wonder why execTuples.c exports MakeTupleTableSlot at
all, though.  ExecAllocTableSlot is supposed to be used by code that
expects ExecutorEnd to clean up the slot, while MakeSingleTupleTableSlot
is supposed to pair with ExecDropSingleTupleTableSlot.  Direct use of
MakeTupleTableSlot leaves one wondering who is holding the bag for
slot cleanup.  The external callers of it all look to be pretty new
code, so I wonder how carefully that's been thought through.

In short: I'm not okay with doing
s/MakeTupleTableSlot/MakeSingleTupleTableSlot/g in a patch that doesn't
also introduce matching ExecDropSingleTupleTableSlot calls (unless those
exist somewhere already; but where?).  If we did clean that up, maybe
MakeTupleTableSlot could become "static".  But I'd still be inclined to
keep it physically separate, leaving it to the compiler to decide whether
to inline it into the callers.

There's a separate question of whether any of the call sites that lack
cleanup support represent live resource-leak bugs.  I see that they all
use TTSOpsVirtual, so maybe that's a slot type that never holds any
interesting resources (like buffer pins).  If so, maybe the best thing is
to invent a wrapper "MakeVirtualTupleTableSlot" or the like, ensuring such
callers use a TupleTableSlotOps type that doesn't require cleanup.

            regards, tom lane