Hi,
> > Not really happy with the name. ExecParallelHashTableInsert() calling
> > ExecParallelHashLoadTuple() to insert a tuple into the hashtable doesn't
> > quite strike me as right; the naming similarity to
> > ExecParallelHashTableLoad seems problematic too.
> > ExecParallelHashAllocTuple() or
On 2017-12-14 21:06:34 +1300, Thomas Munro wrote:
> On Thu, Dec 14, 2017 at 11:45 AM, Andres Freund wrote:
> > + if (accessor->write_pointer +
> > accessor->sts->meta_data_size >=
> > + accessor->write_end)
> > + el
Hi,
Looking at the main patch (v28).
First off: This looks pretty good, the code's quite readable now
(especially compared to earlier versions), the comments are good. Really
like the nodeHash split, and the always inline hackery in nodeHashjoin.
Think we're getting really really close.
*
On Thu, Dec 14, 2017 at 11:45 AM, Andres Freund wrote:
> + booloverflow; /* Continuation of previous
> chunk? */
> + chardata[FLEXIBLE_ARRAY_MEMBER];
> +} SharedTuplestoreChunk;
>
> Ah. I was thinking we could have the 'overflow' variable be an in
Hi,
Looking at the latest version of the tuplestore patch:
diff --git a/src/backend/utils/sort/sharedtuplestore.c
b/src/backend/utils/sort/sharedtuplestore.c
new file mode 100644
index 000..d1233221a58
--- /dev/null
+++ b/src/backend/utils/sort/sharedtuplestore.c
@@ -0,0 +1,583 @@
+/*--
On Sat, Dec 2, 2017 at 3:46 PM, Andres Freund wrote:
> Looking at 0004-Add-shared-tuplestores.patch
>
> Comments:
> - I'd rename mutex to lock. Seems quite possible that we end up with shared
> lockers too.
Done.
> - Expand "Simple mechanism for sharing tuples between backends." intro
> comm
On 2017-12-08 12:07:04 +1300, Thomas Munro wrote:
> I suppose if we wanted to make this type of problem go away, but still
> keep files for forensic purposes, we could introduce a "restart
> number", and stick it into the top level temporary directory's name.
> That way you'd never be able to colli
On Fri, Dec 8, 2017 at 12:07 PM, Thomas Munro
wrote:
> I suppose if we wanted to make this type of problem go away, but still
> keep files for forensic purposes, we could introduce a "restart
> number", and stick it into the top level temporary directory's name.
> That way you'd never be able to c
On Sat, Dec 2, 2017 at 4:46 PM, Thomas Munro
wrote:
> On Sat, Dec 2, 2017 at 3:54 PM, Thomas Munro
> wrote:
>> On Sat, Dec 2, 2017 at 1:55 PM, Andres Freund wrote:
>>> - As we don't clean temp files after crash-restarts it isn't impossible
>>> to have a bunch of crash-restarts and end up with
On 2017-12-02 15:54:29 +1300, Thomas Munro wrote:
> On Sat, Dec 2, 2017 at 1:55 PM, Andres Freund wrote:
> > - Right now RemovePgTempFilesInDir() will recurse into appropriately
> > named directories, and when it recurses it doesn't require the same
> > name pattern checks. I think that's good
On Sat, Dec 2, 2017 at 3:54 PM, Thomas Munro
wrote:
> On Sat, Dec 2, 2017 at 1:55 PM, Andres Freund wrote:
>> - As we don't clean temp files after crash-restarts it isn't impossible
>> to have a bunch of crash-restarts and end up with pids *and* per-pid
>> shared file set counters reused. Whi
On 2017-11-27 22:25:12 +1300, Thomas Munro wrote:
> On Thu, Nov 23, 2017 at 12:36 AM, Thomas Munro
> wrote:
> > Here's a new patch set with responses to the last batch of review comments.
Looking at 0004-Add-shared-tuplestores.patch
Comments:
- I'd rename mutex to lock. Seems quite possible that
On Sat, Dec 2, 2017 at 1:55 PM, Andres Freund wrote:
> Pushed 0003-Add-infrastructure-for-sharing-temporary-files-betwe.patch
> after minor adjustments (some including conversing with you).
Thank you!
> Questions:
> - Right now RemovePgTempFilesInDir() will recurse into appropriately
> named d
Hi,
On 2017-11-27 22:25:12 +1300, Thomas Munro wrote:
Pushed 0003-Add-infrastructure-for-sharing-temporary-files-betwe.patch
after minor adjustments (some including conversing with you).
Changes:
- Changed an impossible elog() into an Assert().
- changed SharedFileSet->counter, and the backend st
On 2017-11-30 14:17:51 +1300, Thomas Munro wrote:
> On Mon, Nov 27, 2017 at 10:25 PM, Thomas Munro
> wrote:
> > On Thu, Nov 23, 2017 at 12:36 AM, Thomas Munro
> > wrote:
> >> Here's a new patch set with responses to the last batch of review comments.
> >
> > Rebased on top of the recent SGML->XML
On Mon, Nov 27, 2017 at 10:25 PM, Thomas Munro
wrote:
> On Thu, Nov 23, 2017 at 12:36 AM, Thomas Munro
> wrote:
>> Here's a new patch set with responses to the last batch of review comments.
>
> Rebased on top of the recent SGML->XML change.
Andres asked me off-list how I tested the barrier.c ca
On Thu, Nov 23, 2017 at 12:36 AM, Thomas Munro
wrote:
> Here's a new patch set with responses to the last batch of review comments.
Rebased on top of the recent SGML->XML change. Also:
1. The final patch in the v26 patchset extended EXPLAIN ANALYZE
output to show per-worker information. I'm wi
Hi Andres and Peter,
Here's a new patch set with responses to the last batch of review comments.
On Wed, Nov 15, 2017 at 10:24 AM, Andres Freund wrote:
> Hm. The way you access this doesn't quite seem right:
> ...
> + matches := regexp_matches(line, ' Batches: ([0-9]+)');
> ...
>
> Why not
Hi,
On 2017-11-14 01:30:30 +1300, Thomas Munro wrote:
> New patch attached.
0002-Add-a-barrier-primitive-for-synchronizing-backends.patch
- Intro starts with:
+ *
+ * This implementation of barriers allows for static sets of participants
+ * known up front, or dynamic sets of participants which p
On Fri, Nov 17, 2017 at 1:55 PM, Andres Freund wrote:
> Looking at 0005-Add-infrastructure-for-sharing-temporary-files-betwe.patch:
> - The created path/filenames seem really redundant:
> base/pgsql_tmp/pgsql_tmp11160.9.sharedfileset.d/pgsql_tmp.o3of8.p0.0
>
> Including pgsql_tmp no less than
Hi,
On 2017-11-14 01:30:30 +1300, Thomas Munro wrote:
> New patch attached.
(I've commit some of the preliminary work)
Looking at 0005-Add-infrastructure-for-sharing-temporary-files-betwe.patch:
- The created path/filenames seem really redundant:
base/pgsql_tmp/pgsql_tmp11160.9.sharedfileset.d
On Thu, Nov 16, 2017 at 6:42 PM, Andres Freund wrote:
> What I'm basically worried about is that the *only* reason for some
> plans to choose to use parallelism is that essentially the effective
> amount of work_mem between the plans is that the parallel one uses
> (max_parallel_workers_per_gather
Hi,
On 2017-11-15 14:09:13 -0500, Robert Haas wrote:
> On Wed, Nov 15, 2017 at 1:35 PM, Andres Freund wrote:
> > But this does bug me, and I think it's what made me pause here to make a
> > bad joke. The way that parallelism treats work_mem makes it even more
> > useless of a config knob than it
On Thu, Nov 16, 2017 at 8:09 AM, Robert Haas wrote:
> On Wed, Nov 15, 2017 at 1:35 PM, Andres Freund wrote:
>> But this does bug me, and I think it's what made me pause here to make a
>> bad joke. The way that parallelism treats work_mem makes it even more
>> useless of a config knob than it was
On Thu, Nov 16, 2017 at 7:57 AM, Peter Geoghegan wrote:
> The contrast with the situation with Thomas and his hash join patch is
> interesting. Hash join is *much* more sensitive to the availability of
> memory than a sort operation is.
>
>> I don't really have a good answer to "but what should we
On Thu, Nov 16, 2017 at 7:35 AM, Andres Freund wrote:
> On 2017-11-15 08:37:11 -0500, Robert Haas wrote:
>> I mean, the very first version of this patch that Thomas submitted was
>> benchmarked by Rafia and had phenomenally good performance
>> characteristics. That turned out to be because it was
On Wed, Nov 15, 2017 at 1:35 PM, Andres Freund wrote:
> But this does bug me, and I think it's what made me pause here to make a
> bad joke. The way that parallelism treats work_mem makes it even more
> useless of a config knob than it was before. Parallelism, especially
> after this patch, shou
Hi,
On 2017-11-15 10:57:35 -0800, Peter Geoghegan wrote:
> > I don't really have a good answer to "but what should we otherwise do",
> > but I'm doubtful this is quite the right answer.
>
> I think that the work_mem model should be replaced by something that
> centrally budgets memory. It would m
On Wed, Nov 15, 2017 at 10:35 AM, Andres Freund wrote:
>> I realize you're sort of joking here, but I think it's necessary to
>> care about fairness between pieces of code.
>
> Indeed I kinda was.
When I posted v1 of parallel CREATE INDEX, it followed the hash join
model of giving workMem (mainte
On 2017-11-15 08:37:11 -0500, Robert Haas wrote:
> On Tue, Nov 14, 2017 at 4:24 PM, Andres Freund wrote:
> >> I agree, and I am interested in that subject. In the meantime, I
> >> think it'd be pretty unfair if parallel-oblivious hash join and
> >> sort-merge join and every other parallel plan ge
On Tue, Nov 14, 2017 at 4:24 PM, Andres Freund wrote:
>> I agree, and I am interested in that subject. In the meantime, I
>> think it'd be pretty unfair if parallel-oblivious hash join and
>> sort-merge join and every other parallel plan get to use work_mem * p
>> (and in some cases waste it with
Hi,
On 2017-11-14 01:30:30 +1300, Thomas Munro wrote:
> > +-- The "good" case: batches required, but we plan the right number; we
> > +-- plan for 16 batches, and we stick to that number, and peak memory
> > +-- usage says within our work_mem budget
> > +-- non-parallel
> > +set max_parallel_worke
32 matches
Mail list logo
