Обсуждение: pgsql: Move memory management away from writetup() and tuplesort_put*()

Move memory management away from writetup() and tuplesort_put*()

This commit puts some generic work away from sort-variant-specific function.
In particular, tuplesort_put*() now doesn't need to decrease available memory
and switch to sort context before calling puttuple_common().  writetup()
doesn't need to free SortTuple.tuple and increase available memory.

Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/097366c45f5dfe142eb232dc6d348ca0705a63a9

Modified Files
--------------
src/backend/utils/sort/tuplesort.c | 78 ++++++++++++++++----------------------
1 file changed, 33 insertions(+), 45 deletions(-)

On Wed, 27 Jul 2022 at 17:29, Alexander Korotkov
<akorotkov@postgresql.org> wrote:
> src/backend/utils/sort/tuplesort.c | 78 ++++++++++++++++----------------------

I was wondering about the following comment that this commit added:

+/*
+ * Write a stored tuple onto tape.tuple.  Unless the slab allocator is
+ * used, after writing the tuple, pfree() the out-of-line data (not the
+ * SortTuple struct!), and increase state->availMem by the amount of
+ * memory space thereby released.
+ */
static void
writetuple(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)

LogicalTable has no field named 'tuple' so I'm thinking the
"tape.tuple" is a mistake?

If so, maybe the comment should be:

/*
 * Write 'stup' out onto 'tape' and, unless using the slab allocator,
pfree stup's
 * tuple and adjust the memory accounting accordingly.
*/

David

Hi, David!

On Fri, Aug 26, 2022 at 3:53 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Wed, 27 Jul 2022 at 17:29, Alexander Korotkov
> <akorotkov@postgresql.org> wrote:
> > src/backend/utils/sort/tuplesort.c | 78 ++++++++++++++++----------------------
>
> I was wondering about the following comment that this commit added:
>
> +/*
> + * Write a stored tuple onto tape.tuple.  Unless the slab allocator is
> + * used, after writing the tuple, pfree() the out-of-line data (not the
> + * SortTuple struct!), and increase state->availMem by the amount of
> + * memory space thereby released.
> + */
> static void
> writetuple(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
>
> LogicalTable has no field named 'tuple' so I'm thinking the
> "tape.tuple" is a mistake?

Thank you for catching this.  Yes, it definitely should be just
"tape", not "tape.tuple".

> If so, maybe the comment should be:
>
> /*
>  * Write 'stup' out onto 'tape' and, unless using the slab allocator,
> pfree stup's
>  * tuple and adjust the memory accounting accordingly.
> */

I've just pushed a fix for this.  But I didn't change the comment to
refer veritable names, just fixed the typo.

------
Regards,
Alexander Korotkov