Re: pgsql: Support parallel btree index builds.
On Tue, Feb 6, 2018 at 8:05 AM, Tom Lane 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
Re: pgsql: Support parallel btree index builds.
Robert Haas writes: > On Tue, Feb 6, 2018 at 10:46 AM, Tom Lane 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
Re: pgsql: Support parallel btree index builds.
On Tue, Feb 6, 2018 at 10:46 AM, Tom Lane wrote: > Robert Haas writes: >> On Sun, Feb 4, 2018 at 1:11 PM, Tom Lane 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
Re: pgsql: Support parallel btree index builds.
Robert Haas writes: > On Sun, Feb 4, 2018 at 1:11 PM, Tom Lane 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
Re: pgsql: Support parallel btree index builds.
On Sun, Feb 4, 2018 at 1:11 PM, Tom Lane 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
Re: pgsql: Support parallel btree index builds.
On Sun, Feb 4, 2018 at 10:11 AM, Tom Lane 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
Re: pgsql: Support parallel btree index builds.
Andres Freund writes: > On 2018-02-02 18:37:11 +, 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
Re: pgsql: Support parallel btree index builds.
On Sun, Feb 4, 2018 at 9:42 AM, Andres Freund wrote: > Wheee! Congrats Peter, Rushash, and everyone else involved! Thanks! -- Peter Geoghegan
Re: pgsql: Support parallel btree index builds.
Hi, On 2018-02-02 18:37:11 +, Robert Haas wrote: > Support parallel btree index builds. Wheee! Congrats Peter, Rushash, and everyone else involved! - Andres