Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Wed, Jul 13, 2016 at 12:24 PM, Tom Lane wrote: >> I am happy with the adjustment. Please commit the adjusted patch. > > Done with minor adjustments. Thanks. I'm pleased that we found a way forward that addressed every concern. -- Peter Geoghegan -- Sent via

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

Peter Geoghegan writes: > On Wed, Jul 13, 2016 at 11:44 AM, Tom Lane wrote: >> If you're good with that adjustment, I'm happy to commit this. > I am happy with the adjustment. Please commit the adjusted patch. Done with minor adjustments.

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Wed, Jul 13, 2016 at 11:44 AM, Tom Lane wrote: > What I don't much like is that it enlarges cluster.out with 200K of > random-looking, hard-to-manually-verify data. May I suggest that > we replace the SELECTs with > > select * from > (select hundred, lag(hundred) over ()

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

Peter Geoghegan writes: > Attached patch adds a CLUSTER external sort test case to the > regression tests, as discussed. I took a quick look at this patch. > This makes a hardly noticeable difference on the run time of the > CLUSTER tests, at least for me. Consider the

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Thu, Jul 7, 2016 at 12:34 AM, Noah Misch wrote: >> How do you feel about adding testing to tuplesort.c not limited to >> hitting this bug (when Valgrind memcheck is used)? > > Sounds great, but again, not in the patch fixing this bug. Attached patch adds a CLUSTER external

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Thu, Jul 7, 2016 at 10:51 AM, Robert Haas wrote: > Thanks for testing. I've committed this patch, breaking off one > unrelated bit of into a separate commit. Thank you. To be clear, I still intend to follow up with a CLUSTER external sort test case, as outlined to

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Thu, Jul 7, 2016 at 3:34 AM, Noah Misch wrote: >> Are you satisfied that I have adequately described steps to reproduce? > > I can confirm that (after 62 minutes) your test procedure reached SIGSEGV > today and then completed successfully with your patch. Thanks for

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Thu, Jul 7, 2016 at 12:34 AM, Noah Misch wrote: >> How do you feel about adding testing to tuplesort.c not limited to >> hitting this bug (when Valgrind memcheck is used)? > > Sounds great, but again, not in the patch fixing this bug. I'll work on a minimal CLUSTER testcase

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

This PostgreSQL 9.6 open item is past due for a status update. On Sat, Jul 02, 2016 at 08:47:20PM -0700, Peter Geoghegan wrote: > On Sat, Jul 2, 2016 at 3:17 PM, Robert Haas wrote: > > In the interest of clarity, I was not intending to say that there > > should be a

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Sat, Jul 2, 2016 at 3:17 PM, Robert Haas wrote: > In the interest of clarity, I was not intending to say that there > should be a regression test in the patch. I was intending to say that > there should be a test case with the bug report. I'm not opposed to > adding a

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 11:37 PM, Noah Misch wrote: > I don't know, either. You read both Robert and me indicating that this bug > fix will be better with a test, and nobody has said that a test-free fix is > optimal. Here's your chance to obliterate that no-tests precedent.

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

Peter Geoghegan writes: > On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch wrote: >> PostgreSQL is open to moving features from zero test coverage to nonzero test >> coverage. The last several releases have each done so. Does that >> sufficiently clarify the

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On 7/2/16 12:40 AM, Peter Geoghegan wrote: We should be more ambitious about adding test coverage to tuplesort.c. IMHO, s/ to tuplesort.c//, at least for the buildfarm. TAP tests don't run by default, so maybe that's the place to start "going crazy" with adding more testing. Though I do

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 10:01 PM, Noah Misch wrote: >> I'm happy that I can do at least that much, but I see no reason to not >> go significantly further. > > Don't risk bundling tests for other sorting scenarios. A minimal test for the > bug in question helps to qualify your

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 01, 2016 at 09:12:42PM -0700, Peter Geoghegan wrote: > On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch wrote: > > PostgreSQL is open to moving features from zero test coverage to nonzero > > test > > coverage. The last several releases have each done so. Does that > >

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch wrote: > Yes, please produce a patch version bearing a regression test that exercises > the bug. I feel it counts even if the you need Valgrind Memcheck to see that > it exercises the bug, but I won't interfere if Robert disagrees.

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 01, 2016 at 08:09:07PM -0700, Peter Geoghegan wrote: > On Fri, Jul 1, 2016 at 7:05 PM, Noah Misch wrote: > > I think such a test would suffice to cover this bug if Valgrind Memcheck > > does > > detect the problem in it. > > Are you asking me to produce a

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 7:05 PM, Noah Misch wrote: > I think such a test would suffice to cover this bug if Valgrind Memcheck does > detect the problem in it. Are you asking me to produce a regression test that exercises the bug? I would be happy to do so, but I require some

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 01, 2016 at 12:37:23PM -0700, Peter Geoghegan wrote: > On Fri, Jul 1, 2016 at 12:30 PM, Peter Geoghegan wrote: > > Checkout my gensort tool from github. Build the C tool with "make". > > Then, from the working directory: > > > > ./postgres_load.py -m 250 --skew

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 12:30 PM, Peter Geoghegan wrote: > Checkout my gensort tool from github. Build the C tool with "make". > Then, from the working directory: > > ./postgres_load.py -m 250 --skew --logged > psql -c "CREATE INDEX segfaults on sort_test_skew(sortkey);" > psql -c

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 12:10 PM, Robert Haas wrote: >> I could give you steps to reproduce the bug, but they involve creating >> a large table using my gensort tool [1]. It isn't trivial. Are you >> interested? > > The bug can't very well be so simple that you need not

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 2:23 PM, Peter Geoghegan wrote: > On Fri, Jul 1, 2016 at 6:23 AM, Robert Haas wrote: >> The proposed patch contains no test case and no description of how to >> reproduce the problem. I am not very keen on the idea of trying to >>

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 6:23 AM, Robert Haas wrote: > The proposed patch contains no test case and no description of how to > reproduce the problem. I am not very keen on the idea of trying to > puzzle that out from first principles. I thought that the bug was simple

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Fri, Jul 1, 2016 at 12:06 AM, Noah Misch wrote: > On Sun, Jun 26, 2016 at 09:14:05PM -0700, Peter Geoghegan wrote: >> In general, moving tuplesort.c batch memory caller tuples around >> happens when batch memory needs to be recycled, or freed outright with >> pfree(). >> >>

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

On Sun, Jun 26, 2016 at 09:14:05PM -0700, Peter Geoghegan wrote: > In general, moving tuplesort.c batch memory caller tuples around > happens when batch memory needs to be recycled, or freed outright with > pfree(). > > I failed to take into account that CLUSTER tuplesorts need an extra > step

[HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

In general, moving tuplesort.c batch memory caller tuples around happens when batch memory needs to be recycled, or freed outright with pfree(). I failed to take into account that CLUSTER tuplesorts need an extra step when moving caller tuples to a new location (i.e. when moving HeapTuple caller