Re: [PATCH] Incremental sort (was: PoC: Partial sort) - Mailing list pgsql-hackers

On Thu, Mar 12, 2020 at 5:53 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> 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.

Yes, I think you're correct (re: 0008).

They all look generally good to me, and are included in the attached
patch series.

> 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.

Is there a page on how you're supposed to run pgindent/when stuff like
this does get added/etc.? It's all a big mystery to me right now.

Also, I noticed some of the pgindent changes aren't for changes in
this patch series; I have that as a separate patch, but not attached
because I see that running pgindent locally generates a massive patch,
so I'm assuming we just ignore those for now?

> 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 had seen this before I sent the patch, but then it seemed like it
disappeared, so I didn't come back to it; maybe I just missed it in my
buffer.

I do see it now, and moving the declarations into each relevant block
(rather than trying to share them) seems to fix it. I think that's
correct anyway, since before they were technically being assigned to
more than once which seems wrong for const.

I have this change locally and will include it in my next patch version.

> 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.

I'm not sure how that would work: those functions (for
EXPLAIN_FORMAT_TEXT) all add newlines, and this code is intentionally
trying to avoid too many lines.

I'm open to suggestions though.

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

As would I!

James