Re: [PATCH] Incremental sort (was: PoC: Partial sort)

I gave this a very quick look; I don't claim to understand it or
anything, but I thought these trivial cleanups worthwhile. The only
non-cosmetic thing is changing order of arguments to the SOn_printf()
calls in 0008; I think they are contrary to what the comment says.

I don't propose to commit 0003 of course, since it's not our policy;
that's just to allow running pgindent sanely, which gives you 0004
(though my local pgindent has an unrelated fix). And after that you
notice the issue that 0005 fixes.

I did notice that show_incremental_sort_group_info() seems to be doing
things in a hard way, or something. I got there because it throws this
warning:

/pgsql/source/master/src/backend/commands/explain.c: In function 'show_incremental_sort_group_info':
/pgsql/source/master/src/backend/commands/explain.c:2766:39: warning: passing argument 2 of 'lappend' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
methodNames = lappend(methodNames, sortMethodName);
^~~~~~~~~~~~~~
In file included from /pgsql/source/master/src/include/access/xact.h:20,
from /pgsql/source/master/src/backend/commands/explain.c:16:
/pgsql/source/master/src/include/nodes/pg_list.h:509:14: note: expected 'void *' but argument is of type 'const char *'
extern List *lappend(List *list, void *datum);
^~~~~~~
/pgsql/source/master/src/backend/commands/explain.c:2766:39: warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
methodNames = lappend(methodNames, sortMethodName);
^~~~~~~~~~~~~~
/pgsql/source/master/src/include/nodes/pg_list.h:509:40: note: passing argument to parameter 'datum' here
extern List *lappend(List *list, void *datum);
^
1 warning generated.

(Eh, it's funny that GCC reports two warnings about the same line, and
then says there's one warning.)

I suppose you could silence this by adding pstrdup(), and then use
list_free_deep (you have to put the sortMethodName declaration in the
inner scope for that, but seems fine). Or maybe there's a clever way
around it.

But I hesitate to send a patch for that because I think the whole
function is written by handling text and the other outputs completely
separately -- but looking for example show_modifytable_info() it seems
you can do ExplainOpenGroup, ExplainPropertyText, ExplainPropertyList
etc in all explain output modes, and those routines will care about
emitting the data in the correct format, without having the
show_incremental_sort_group_info function duplicate everything.

HTH. I would really like to get this patch done for pg13.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services