From: | James Coleman <jtc331(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Shaun Thomas <shaun(dot)thomas(at)2ndquadrant(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] Incremental sort (was: PoC: Partial sort) |
Date: | 2020-03-13 17:06:33 |
Message-ID: | CAAaqYe9rkqSA2OAYQAg=Kdv1v1j6WoB9wLX5MkT_w27z_upKkQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Mar 12, 2020 at 5:53 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)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
Attachment | Content-Type | Size |
---|---|---|
v35-0001-Consider-low-startup-cost-when-adding-partial-pa.patch | application/octet-stream | 3.2 KB |
v35-0004-A-couple-more-places-for-incremental-sort.patch | application/octet-stream | 8.8 KB |
v35-0002-Implement-incremental-sort.patch | application/octet-stream | 149.6 KB |
v35-0003-Consider-incremental-sort-paths-in-additional-pl.patch | application/octet-stream | 13.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2020-03-13 17:09:00 | Re: shared-memory based stats collector |
Previous Message | Tom Lane | 2020-03-13 17:06:29 | Re: range_agg |