Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()

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

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]/messages/by-id/1632520.1613195514@sss.pgh.pa.us?

[1]: /messages/by-id/1632520.1613195514@sss.pgh.pa.us

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

Hi Alvaro,

Did you see the arguments at [1]?

[1] /messages/by-id/1632520.1613195514@sss.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

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)

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

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)