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

On Sun, Mar 22, 2020 at 6:02 PM Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>
> On 3/21/20 1:56 AM, Tomas Vondra wrote:
> > I've looked at v38 but it seems it's a bit broken by some recent explain
> > changes (mostly missing type in declarations). Attached is v39 fixing
> > those issues, and including a bunch of fixes based on a review - most of
> > the changes is in comments, so I've instead kept them in separate "fix"
> > patches after each part.
> >
> > In general I'm mostly happy with the current shape of the patch, and
> > unless there are some objections I'd like to get some of it committed
> > sometime next week.
>
> Hi,
>
> I haven't had any time to look at the patch yet but when compiling it
> and running the tests GCC gave me a warning. The tests passed btw.
>
> gcc (Debian 8.3.0-6) 8.3.0
>
> explain.c: In function ‘show_incremental_sort_group_info’:
> explain.c:2772:39: warning: passing argument 2 of ‘lappend’ discards
> ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> methodNames = lappend(methodNames, sortMethodName);
> ^~~~~~~~~~~~~~
> In file included from ../../../src/include/access/xact.h:20,
> from explain.c:16:
> ../../../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);
> ^~~~~~~
> explain.c:2772:39: warning: passing 'const char *' to parameter of type
> 'void *' discards qualifiers
> [-Wincompatible-pointer-types-discards-qualifiers]
> methodNames = lappend(methodNames, sortMethodName);
> ^~~~~~~~~~~~~~
> ../../../src/include/nodes/pg_list.h:509:40: note: passing argument to
> parameter 'datum' here
> extern List *lappend(List *list, void *datum);

So if we naively get rid of the const on the variable declaration in
question, then we get this warning instead:

explain.c: In function ‘show_incremental_sort_group_info’:
explain.c:2770:27: warning: initialization discards ‘const’ qualifier
from pointer target type [-Wdiscarded-qualifiers]
char *sortMethodName = tuplesort_method_name(methodCell->int_value);

So on the face of it we have a bit of a no-win situation. The function
tuple_sort_method_name returns a const, but lappend wants a non-const.
I'm not sure what the project style preference is here: we could cast
the result as (char *) to drop the const qualifier, but that's frowned
upon some places. Alternatively we could make a new non-const copy of
string. Which is preferable in the postgres project style?

James