pgsql: Support parallel btree index builds.

Hi,

On 2018-02-02 18:37:11 +0000, Robert Haas wrote:

Support parallel btree index builds.

Wheee! Congrats Peter, Rushash, and everyone else involved!

- Andres

On Sun, Feb 4, 2018 at 9:42 AM, Andres Freund <andres@anarazel.de> wrote:

Wheee! Congrats Peter, Rushash, and everyone else involved!

Thanks!

--
Peter Geoghegan

Andres Freund <andres@anarazel.de> writes:

On 2018-02-02 18:37:11 +0000, Robert Haas wrote:

Support parallel btree index builds.

Wheee! Congrats Peter, Rushash, and everyone else involved!

I'll be happier about it when the valgrind buildfarm animals are
happy.

regards, tom lane

On Sun, Feb 4, 2018 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll be happier about it when the valgrind buildfarm animals are
happy.

I don't know if you noticed, but I did post a patch for that on Friday.

--
Peter Geoghegan

On Sun, Feb 4, 2018 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll be happier about it when the valgrind buildfarm animals are
happy.

Me too, but it's not clear what the right fix is. One thing that
would help is if you put in an appearance on the thread where this is
being discussed and cast a vote. (Ditto to Andres.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Feb 4, 2018 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll be happier about it when the valgrind buildfarm animals are
happy.

Me too, but it's not clear what the right fix is. One thing that
would help is if you put in an appearance on the thread where this is
being discussed and cast a vote. (Ditto to Andres.)

If you mean do I like fixing this by adding a valgrind suppression,
no I do not. Valgrind suppressions are last-resort band-aids IMO,
to be applied only when it's clearly understood what behavior we're
masking and why it's more reasonable to mask it than make it better
defined. I, at least, don't have that understanding from looking
at the thread. For one thing, Peter has not explained why this issue
appears now with parallel index build when it did not before; it's
not like logtape.c isn't old enough to vote.

Even granting that a suppression is the way to fix it, the proposed
suppression seems pretty darn broad, and hence likely to mask things
we'd wish it hadn't.

regards, tom lane

On Tue, Feb 6, 2018 at 10:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Feb 4, 2018 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll be happier about it when the valgrind buildfarm animals are
happy.

Me too, but it's not clear what the right fix is. One thing that
would help is if you put in an appearance on the thread where this is
being discussed and cast a vote. (Ditto to Andres.)

If you mean do I like fixing this by adding a valgrind suppression,
no I do not. Valgrind suppressions are last-resort band-aids IMO,
to be applied only when it's clearly understood what behavior we're
masking and why it's more reasonable to mask it than make it better
defined. I, at least, don't have that understanding from looking
at the thread. For one thing, Peter has not explained why this issue
appears now with parallel index build when it did not before; it's
not like logtape.c isn't old enough to vote.

Yeah, he has actually. In other cases, the buffer is guaranteed to
have been filled at least once (and thus, from valgrind's point of
view, is initialized) because if that weren't going to happen then we
would have not have switched to a tape-sort in the first place. You
can't set work_mem smaller than 8kB. But in the parallel case each
worker must always produce a tape, so it can happen if a worker is
unlucky enough to get only a very small slice of the data (because the
other participants gobble it all up before that process really gets
going).

Even granting that a suppression is the way to fix it, the proposed
suppression seems pretty darn broad, and hence likely to mask things
we'd wish it hadn't.

Well, he talked about that at some length too. I don't know how
you're not seeing it on the thread. But what I really need here is
some input on an option you do like, not just a list of things you
don't like.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Feb 6, 2018 at 10:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

... I, at least, don't have that understanding from looking
at the thread. For one thing, Peter has not explained why this issue
appears now with parallel index build when it did not before; it's
not like logtape.c isn't old enough to vote.

Yeah, he has actually. In other cases, the buffer is guaranteed to
have been filled at least once (and thus, from valgrind's point of
view, is initialized) because if that weren't going to happen then we
would have not have switched to a tape-sort in the first place. You
can't set work_mem smaller than 8kB. But in the parallel case each
worker must always produce a tape, so it can happen if a worker is
unlucky enough to get only a very small slice of the data (because the
other participants gobble it all up before that process really gets
going).

Ah, I see. So this is really a problem that's been latent all along,
but was never exposed in any previous use-case for logtape.c.

But what I really need here is
some input on an option you do like, not just a list of things you
don't like.

I like the option of doing VALGRIND_MAKE_MEM_DEFINED on the tail
portion of the buffer before writing it. That seems pretty tightly
tied to the behavior we're decreeing valid, whereas the suppression
is not.

regards, tom lane

On Tue, Feb 6, 2018 at 8:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I like the option of doing VALGRIND_MAKE_MEM_DEFINED on the tail
portion of the buffer before writing it. That seems pretty tightly
tied to the behavior we're decreeing valid, whereas the suppression
is not.

I think that the suppression is actually slightly better scoped than
that, since, for example, that won't just affect writes of
uninitialized bytes from the buffer. But I'll do it that way.

--
Peter Geoghegan

On 2018-Feb-02, Robert Haas wrote:

Support parallel btree index builds.

While looking at a complaint related to progress report of parallel
index builds[1]/messages/by-id/CAEze2Wgm-NnZe3rOnwjYTVriS8xsVhzzVBCGj34h06cDNuaTig@mail.gmail.com, I noticed this comment

+   /*
+    * Execute this worker's part of the sort.
+    *
+    * Unlike leader and serial cases, we cannot avoid calling
+    * tuplesort_performsort() for spool2 if it ends up containing no dead
+    * tuples (this is disallowed for workers by tuplesort).
+    */
+   tuplesort_performsort(btspool->sortstate);
+   if (btspool2)
+       tuplesort_performsort(btspool2->sortstate);

I've been trying to understand why this says "Unlike leader and serial
cases, ...". I understand the "serial" part -- it refers to
_bt_leafbuild. So I'm to understand that that one works differently;
see below. But why does it say "the leader case"? As far as I can see,
the leader executes exactly the same code, so what is the comment
talking about?

Now, if you do look at _bt_leafbuild(), it can be seen that nothing is
done differently there either; we're not actually skipping any calls to
tuplesort_performsort(). Any differentiation between serial/leader/
worker cases seems to be done inside that routine. So the comment is
not very useful there either.

I am wondering if these comments are leftovers from early development
versions of this patch. Maybe we could remove them -- or rewrite them
to indicate not that we avoid calling tuplesort_performsort(), but
instead to say that that function behaves differently.

[1]: /messages/by-id/CAEze2Wgm-NnZe3rOnwjYTVriS8xsVhzzVBCGj34h06cDNuaTig@mail.gmail.com

--
�lvaro Herrera 39�49'30"S 73�17'W
"Puedes vivir s�lo una vez, pero si lo haces bien, una vez es suficiente"

On Mon, Jun 7, 2021 at 4:11 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Now, if you do look at _bt_leafbuild(), it can be seen that nothing is
done differently there either; we're not actually skipping any calls to
tuplesort_performsort(). Any differentiation between serial/leader/
worker cases seems to be done inside that routine. So the comment is
not very useful there either.

I am wondering if these comments are leftovers from early development
versions of this patch. Maybe we could remove them -- or rewrite them
to indicate not that we avoid calling tuplesort_performsort(), but
instead to say that that function behaves differently.

It's talking about something described in the tuplesort.h contract. It
applies to a tuplesort state, not a process -- the leader always has two
tuplesort states (the leader tuplesort state, plus its own worker
tuplesort state).

The leader tuplesort is very much like a serial tuplesort. In
particular, as step 8 in tuplesort.h points out, the leader doesn't
have to call tuplesort_performsort() for the leader tuplesort state if
it already knows that there is no input to sort.

This matters less than it might in a world where we had a user of
parallel tuplesort that doesn't always simply make the leader
participate as a worker. There is a build-time testing option in
nbtsort.c that does this for parallel CREATE INDEX, actually -- see
DISABLE_LEADER_PARTICIPATION.

You kind of have a point about this being something that made more
sense in revisions of the patch from before commit, though. There was
a question about the cost model and the role of the leader that was
ultimately resolved by inventing the current simple behavior. So
feel free to change the wording now.

--
Peter Geoghegan