Thread: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Aleksander Alekseev
Date:
Hi hackers,

During the discussion [1] it was discovered that we have two
procedures in execTuples.c that do the same thing:

* MakeSingleTupleTableSlot()
* MakeTupleTableSlot()

In fact, MakeSingleTupleTableSlot() is simply a wrapper for
MakeTupleTableSlot().

I propose keeping only one of these procedures to simplify navigating
through the code and debugging, and maybe saving a CPU cycle or two. A
search for MakeTupleTableSlot produced 8 matches across 2 files, while
MakeSingleTupleTableSlot is used 41 times across 26 files. Thus the
proposed patch removes MakeTupleTableSlot and keeps
MakeSingleTupleTableSlot to keep the patch less invasive and simplify
backporting of the other patches. Hopefully, this will not complicate
the life of the extension developers too much.

The patch was tested on MacOS against `master` branch b1ce6c28.

[1]: https://www.postgresql.org/message-id/flat/CAJ7c6TP0AowkUgNL6zcAK-s5HYsVHVBRWfu69FRubPpfwZGM9A%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Attachment

Re: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Michael Paquier
Date:
On Fri, Oct 22, 2021 at 04:39:37PM +0300, Aleksander Alekseev wrote:
> I propose keeping only one of these procedures to simplify navigating
> through the code and debugging, and maybe saving a CPU cycle or two. A
> search for MakeTupleTableSlot produced 8 matches across 2 files, while
> MakeSingleTupleTableSlot is used 41 times across 26 files. Thus the
> proposed patch removes MakeTupleTableSlot and keeps
> MakeSingleTupleTableSlot to keep the patch less invasive and simplify
> backporting of the other patches. Hopefully, this will not complicate
> the life of the extension developers too much.

To make the life of extension developers easier, we could as well have
a compatibility macro so as anybody using MakeTupleTableSlot() won't
be annoyed by this change.  However, looking around, this does not
look like a popular API so I'd be fine with your change as proposed.

Other opinions?
--
Michael

Attachment

Re: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Alvaro Herrera
Date:
On 2021-Oct-22, Aleksander Alekseev wrote:

> Hi hackers,
> 
> During the discussion [1] it was discovered that we have two
> procedures in execTuples.c that do the same thing:
> 
> * MakeSingleTupleTableSlot()
> * MakeTupleTableSlot()
> 
> In fact, MakeSingleTupleTableSlot() is simply a wrapper for
> MakeTupleTableSlot().

Did you see the arguments at [1]?

[1] https://www.postgresql.org/message-id/1632520.1613195514%40sss.pgh.pa.us

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/



Re: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Aleksander Alekseev
Date:
Hi Alvaro,

> Did you see the arguments at [1]?
>
> [1] https://www.postgresql.org/message-id/1632520.1613195514%40sss.pgh.pa.us

No, I missed it. Thanks for sharing.

> 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.
> [...]
> 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.
> [...]

OK, I will need some time to figure out the actual difference between
these two functions and to submit an updated version of the patch.

-- 
Best regards,
Aleksander Alekseev



Re: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Alvaro Herrera
Date:
On 2021-Oct-26, Aleksander Alekseev wrote:

> > 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.

Another point that could be made is that perhaps
MakeSingleTupleTableSlot should always construct a slot using virtual
tuples rather than passing TTSOps as a parameter?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)



Re: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Robert Haas
Date:
On Tue, Oct 26, 2021 at 7:54 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Another point that could be made is that perhaps
> MakeSingleTupleTableSlot should always construct a slot using virtual
> tuples rather than passing TTSOps as a parameter?

I haven't really looked at this issue deeply but that seems like it
might be a bit confusing. Then "single" would end up being an alias
for "virtual" which I don't suppose is what anyone is expecting.

-- 
Robert Haas
EDB: http://www.enterprisedb.com



Re: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

From
Alvaro Herrera
Date:
On 2021-Oct-26, Robert Haas wrote:

> On Tue, Oct 26, 2021 at 7:54 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Another point that could be made is that perhaps
> > MakeSingleTupleTableSlot should always construct a slot using virtual
> > tuples rather than passing TTSOps as a parameter?
> 
> I haven't really looked at this issue deeply but that seems like it
> might be a bit confusing. Then "single" would end up being an alias
> for "virtual" which I don't suppose is what anyone is expecting.

Yeah -- another point against that idea is that most of the callers are
indeed not using virtual tuples, so it doesn't really work.  I was just
thinking that if something wants to process transient tuples they may
just be virtual and not be forced to make them heap tuples, but on
looking again, that's not how the abstraction works.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)