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

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Cc: James Coleman <jtc331(at)gmail(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-12 21:53:11
Message-ID: 20200312215311.GA8528@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

/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
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
0001-fix-typo.patch text/x-diff 1.9 KB
0002-fix-another-typo.patch text/x-diff 878 bytes
0003-typedefs-additions.patch text/x-diff 746 bytes
0004-pgindent.patch text/x-diff 65.9 KB
0005-no-n-after-left-parens-see-c9d297751959.patch text/x-diff 2.8 KB
0006-use-castNode-instead-of-Assert-IsA-plus-cast.patch text/x-diff 2.8 KB
0007-Test-trivial-condition-before-more-complex-one.patch text/x-diff 1.2 KB
0008-reverse-arguments-.-isn-t-the-other-order-a-bug.patch text/x-diff 2.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Justin Pryzby 2020-03-12 23:11:16 Re: Additional size of hash table is alway zero for hash aggregates
Previous Message Justin Pryzby 2020-03-12 21:01:46 Re: Memory-Bounded Hash Aggregation