Обсуждение: Refactoring: join MakeSingleTupleTableSlot() and MakeTupleTableSlot()
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
Вложения
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]? [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()
От
Aleksander Alekseev
Дата:
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
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)