On Fri, 13 Sept 2024 at 00:12, Tatsuo Ishii <is...@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
