On Fri, 13 Sept 2024 at 00:12, Tatsuo Ishii <ishii@postgresql.org> wrote:
> In this patch I refactored show_material_info. I divided it into
> show_material_info and show_storage_info so that the latter can be
> used by other node types including window aggregate node. What do you
> think?
Yes, I think it's a good idea to move that into a helper function. If
you do the other node types, without that helper the could would have
to be repeated quite a few times. Maybe show_storage_info() can be
moved up with the other helper functions, say below
show_sortorder_options() ? It might be a good idea to keep the "if
(!es->analyze || tupstore == NULL)" checks in the calling function
rather than the helper too.
I thought about the location of the test for a while and read the
"This file is concerned with testing EXPLAIN in its own right."
comment at the top of that explain.out. I was trying to decide if
testing output of a specific node type met this or not. I can't pick
out any other tests there which are specific to a node type, so I'm
unsure if this is the location for it or not. However, to put it
anywhere else means having to add a plpgsql function to mask out the
unstable parts of EXPLAIN, so maybe the location is good as it saves
from having to do that. I'm 50/50 on this, so I'm happy to let you
decide. You could also shrink that 100 rows into a smaller number for
the generate_series without losing any coverage.
Aside from that, I think the patch is good. Thanks for working on it.
David
