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
Hi,
MakeSingleTupleTableSlot can be defined as a macro, calling
MakeTupleTableSlot().
Cheers
On Fri, Feb 12, 2021 at 5:44 AM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:
Show quoted text
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
Attachments:
v1-0001-Remove-unnecessary-wrapping-of-MakeTupleTableSlot.patchapplication/octet-stream; name=v1-0001-Remove-unnecessary-wrapping-of-MakeTupleTableSlot.patchDownload
From 5c1fe914303a970a812579da41aca9286abe442b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Sat, 13 Feb 2021 09:37:53 +0530
Subject: [PATCH v1] Remove unnecessary wrapping of MakeTupleTableSlot in
MakeSingleTupleTableSlot
Currently, MakeSingleTupleTableSlot function just calls another
function MakeTupleTableSlot to return the final slot without doing
anything on the slot returned by the MakeTupleTableSlot.
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.
This patch just renames MakeTupleTableSlot to MakeSingleTupleTableSlot
and removes existing MakeSingleTupleTableSlot function and also
replaces MakeTupleTableSlot with MakeSingleTupleTableSlot.
---
src/backend/executor/execMain.c | 8 ++++----
src/backend/executor/execTuples.c | 24 +++---------------------
src/include/executor/tuptable.h | 2 --
3 files changed, 7 insertions(+), 27 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c74ce36ffb..36d01dfcd4 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1749,7 +1749,7 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
*/
if (map != NULL)
slot = execute_attr_map_slot(map, slot,
- MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+ MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual));
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
ExecGetUpdatedCols(rootrel, estate));
}
@@ -1834,7 +1834,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
*/
if (map != NULL)
slot = execute_attr_map_slot(map, slot,
- MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+ MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual));
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
ExecGetUpdatedCols(rootrel, estate));
rel = rootrel->ri_RelationDesc;
@@ -1886,7 +1886,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
*/
if (map != NULL)
slot = execute_attr_map_slot(map, slot,
- MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+ MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual));
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
ExecGetUpdatedCols(rootrel, estate));
rel = rootrel->ri_RelationDesc;
@@ -1993,7 +1993,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
*/
if (map != NULL)
slot = execute_attr_map_slot(map, slot,
- MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+ MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual));
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
ExecGetUpdatedCols(rootrel, estate));
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 73c35df9c9..f0aafc2198 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -1070,7 +1070,7 @@ const TupleTableSlotOps TTSOpsBufferHeapTuple = {
*/
/* --------------------------------
- * MakeTupleTableSlot
+ * MakeSingleTupleTableSlot
*
* Basic routine to make an empty TupleTableSlot of given
* TupleTableSlotType. If tupleDesc is specified the slot's descriptor is
@@ -1079,7 +1079,7 @@ const TupleTableSlotOps TTSOpsBufferHeapTuple = {
* --------------------------------
*/
TupleTableSlot *
-MakeTupleTableSlot(TupleDesc tupleDesc,
+MakeSingleTupleTableSlot(TupleDesc tupleDesc,
const TupleTableSlotOps *tts_ops)
{
Size basesz,
@@ -1141,7 +1141,7 @@ TupleTableSlot *
ExecAllocTableSlot(List **tupleTable, TupleDesc desc,
const TupleTableSlotOps *tts_ops)
{
- TupleTableSlot *slot = MakeTupleTableSlot(desc, tts_ops);
+ TupleTableSlot *slot = MakeSingleTupleTableSlot(desc, tts_ops);
*tupleTable = lappend(*tupleTable, slot);
@@ -1195,24 +1195,6 @@ ExecResetTupleTable(List *tupleTable, /* tuple table */
list_free(tupleTable);
}
-/* --------------------------------
- * MakeSingleTupleTableSlot
- *
- * This is a convenience routine for operations that need a standalone
- * TupleTableSlot not gotten from the main executor tuple table. It makes
- * a single slot of given TupleTableSlotType and initializes it to use the
- * given tuple descriptor.
- * --------------------------------
- */
-TupleTableSlot *
-MakeSingleTupleTableSlot(TupleDesc tupdesc,
- const TupleTableSlotOps *tts_ops)
-{
- TupleTableSlot *slot = MakeTupleTableSlot(tupdesc, tts_ops);
-
- return slot;
-}
-
/* --------------------------------
* ExecDropSingleTupleTableSlot
*
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index 679e57fbdd..54ec4514b5 100644
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -293,8 +293,6 @@ typedef struct MinimalTupleTableSlot
((slot) == NULL || TTS_EMPTY(slot))
/* in executor/execTuples.c */
-extern TupleTableSlot *MakeTupleTableSlot(TupleDesc tupleDesc,
- const TupleTableSlotOps *tts_ops);
extern TupleTableSlot *ExecAllocTableSlot(List **tupleTable, TupleDesc desc,
const TupleTableSlotOps *tts_ops);
extern void ExecResetTupleTable(List *tupleTable, bool shouldFree);
--
2.25.1
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