From: | James Coleman <jtc331(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(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>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andreas Karlsson <andreas(at)proxel(dot)se> |
Subject: | Re: [PATCH] Incremental sort (was: PoC: Partial sort) |
Date: | 2020-04-07 02:03:10 |
Message-ID: | CAAaqYe-dRK3+Uv_L_A68YCejfFKQzP4ov_azWh0yak6qypTXTA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Apr 6, 2020 at 9:46 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
> > I don't know, I've tried running the tests on a number of machines,
> > similar to those failing. Rapsberry Pi, Fedora 31, ... and it worked
> > everywhere while the failures seem consistent.
>
> On my machine, it reproduces about one time in six with
> force_parallel_mode = regress. It seems possible given your
> results that reducing max_parallel_workers would make it more
> likely, but I've not tried that.
>
> What I'm seeing, after adding some debug printouts, is that sortMethod is
> frequently zero when we reach the EXPLAIN output for a worker. In many of
> the tests this happens even though there is no visible failure, because
> we've got a filter function hiding the output :-(
>
> So I concur with James' conclusion that the existing code is relying on
> sortMethod initializing to zeroes, and that we did the wrong thing by
> trying to give SORT_TYPE_STILL_IN_PROGRESS a nonzero representation.
> I do not like his patch though, particularly not the type pun with NULL.
Sentinel and NULL? I hadn't caught that at all.
> I think the correct fix is to change the enum declaration.
Hmm. I don't actually really like that, because it means the value
here isn't actually semantically correct. That is, the sort type is
not "in progress"; it's "we never started a sort at all". I don't
really love the conflating of those things that the old enum
declaration had (even it'd had a helpful comment). It seems to me that
we should make "we don't have a type" and "we have a type" distinct.
We could add a new enum value SORT_TYPE_UNINITIALIZED or similar though.
James
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-04-07 02:09:09 | Re: [PATCH] Incremental sort (was: PoC: Partial sort) |
Previous Message | Tom Lane | 2020-04-07 01:46:14 | Re: [PATCH] Incremental sort (was: PoC: Partial sort) |