Re: Use generation memory context for tuplestore.c

On Fri, 31 May 2024 at 05:26, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sat, 4 May 2024 at 03:51, Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> >
> > On Fri, 3 May 2024 at 15:55, David Rowley <dgrowleyml@gmail.com> wrote:
> > > master @ 8f0a97dff
> > > Storage: Memory  Maximum Storage: 16577kB
> > >
> > > patched:
> > > Storage: Memory  Maximum Storage: 8577kB
> >
> > Those are some impressive numbers.
>
> This patch needed to be rebased, so updated patches are attached.

Here's a review for the V2 patch:

I noticed this change to buffile.c shows up in both v1 and v2 of the
patchset, but I can't trace the reasoning why it changed with this
patch (rather than a separate bugfix):

> +++ b/src/backend/storage/file/buffile.c
> @@ -867,7 +867,7 @@ BufFileSize(BufFile *file)
> {
>     int64        lastFileSize;
>
> -    Assert(file->fileset != NULL);
> +    Assert(file->files != NULL);

> +++ b/src/backend/commands/explain.c
> [...]
> +show_material_info(MaterialState *mstate, ExplainState *es)
> +    spaceUsedKB = (tuplestore_space_used(tupstore) + 1023) / 1024;

I think this should use the BYTES_TO_KILOBYTES macro for consistency.

Lastly, I think this would benefit from a test in
regress/sql/explain.sql, as the test changes that were included
removed the only occurrance of a Materialize node from the regression
tests' EXPLAIN outputs.



Kind regards,

Matthias van de Meent