Amit Kapila writes:
> On Tue, Aug 29, 2017 at 10:05 PM, Tom Lane wrote:
>> If no objections, I'll do the additional legwork and push.
> No objections.
Done. Out of curiosity, I pushed just the rescan-param patch to the
buildfarm to start with, to see if anything would fall over, and indeed
som
On Wed, Aug 30, 2017 at 7:39 AM, Tom Lane wrote:
> Amit Kapila writes:
>> On Tue, Aug 29, 2017 at 10:05 PM, Tom Lane wrote:
>> ! /* Make sure any existing workers are gracefully shut down */
>> ExecShutdownGatherWorkers(node);
>
>> The above call doesn't ensure the shutdown. It just ensures th
Amit Kapila writes:
> On Tue, Aug 29, 2017 at 10:05 PM, Tom Lane wrote:
> ! /* Make sure any existing workers are gracefully shut down */
> ExecShutdownGatherWorkers(node);
> The above call doesn't ensure the shutdown. It just ensures that we
> receive all messages from parallel workers. Basi
On Tue, Aug 29, 2017 at 10:05 PM, Tom Lane wrote:
> Amit Kapila writes:
>> On Tue, Aug 29, 2017 at 8:32 AM, Robert Haas wrote:
>>> There's already ExecParallelReinitialize, which could be made to walk
>>> the nodes in addition to what it does already, but I don't understand
>>> exactly what else
Amit Kapila writes:
> On Tue, Aug 29, 2017 at 8:32 AM, Robert Haas wrote:
>> There's already ExecParallelReinitialize, which could be made to walk
>> the nodes in addition to what it does already, but I don't understand
>> exactly what else needs fixing.
> Sure, but it is not advisable to reset
On Tue, Aug 29, 2017 at 8:32 AM, Robert Haas wrote:
> On Mon, Aug 28, 2017 at 10:17 PM, Tom Lane wrote:
>> In the meantime, I think what we should do is commit the bug fix more or
>> less as I have it, and then work on Amit's concern about losing parallel
>> efficiency by separating the resetting
On Mon, Aug 28, 2017 at 10:17 PM, Tom Lane wrote:
> In the meantime, I think what we should do is commit the bug fix more or
> less as I have it, and then work on Amit's concern about losing parallel
> efficiency by separating the resetting of shared parallel-scan state
> into a new plan tree trav
Robert Haas writes:
> On Mon, Aug 28, 2017 at 6:35 PM, Tom Lane wrote:
>> Maybe parallel_aware should have more than two values, depending
>> on whether the result of the node is context-dependent or not.
> It seems likely the whole concept of parallel_aware is only only a
> zero-order approxima
On Mon, Aug 28, 2017 at 6:35 PM, Tom Lane wrote:
> but probably we should think of a more arm's-length way to do it.
> Maybe parallel_aware should have more than two values, depending
> on whether the result of the node is context-dependent or not.
My original intent for the parallel_aware flag w
Robert Haas writes:
> On Mon, Aug 28, 2017 at 3:00 PM, Tom Lane wrote:
>> Count the "that"s (you're failing to notice the next line).
> OK, true. But "Academic literature" -> "The academic literature" is
> just second-guessing, I think.
No, it was more to avoid reflowing the paragraph (or leav
On Mon, Aug 28, 2017 at 3:00 PM, Tom Lane wrote:
>> That sentence isn't wrong as written.
>
> Count the "that"s (you're failing to notice the next line).
OK, true. But "Academic literature" -> "The academic literature" is
just second-guessing, I think.
>> I don't really understand the part abou
Robert Haas writes:
> On Mon, Aug 28, 2017 at 10:47 AM, Tom Lane wrote:
> - fuller description. Academic literature on parallel query suggests that
> + fuller description. The academic literature on parallel query suggests
> That sentence isn't wrong as written.
Count the "that"s (you're fail
On Mon, Aug 28, 2017 at 10:47 AM, Tom Lane wrote:
> That seems like an unacceptably fragile assumption. Even if it happens to
> be true today, we would need to fix it sooner or later. (And I kinda
> suspect it's possible to break it today, anyway. Treating PARAM_EXEC
> Params as parallel-restri
Amit Kapila writes:
> On Mon, Aug 28, 2017 at 6:34 PM, Tom Lane wrote:
>> If what you're complaining about is that I put back the "if
>> (outerPlan->chgParam == NULL)" test to allow postponement of the
>> recursive ExecReScan call, I'm afraid that it's mere wishful
>> thinking that omitting that
On Mon, Aug 28, 2017 at 6:34 PM, Tom Lane wrote:
> Amit Kapila writes:
>> On Mon, Aug 28, 2017 at 6:01 PM, Tom Lane wrote:
>>> Um, what's different about that than before?
>
>> Earlier, we perform the rescan of all the nodes before ExecProcNode,
>> so workers will always start (restart) after th
Amit Kapila writes:
> On Mon, Aug 28, 2017 at 6:01 PM, Tom Lane wrote:
>> Um, what's different about that than before?
> Earlier, we perform the rescan of all the nodes before ExecProcNode,
> so workers will always start (restart) after the scan descriptor is
> initialized.
If what you're compl
On Mon, Aug 28, 2017 at 6:01 PM, Tom Lane wrote:
> Amit Kapila writes:
>> With this change, it is quite possible that during rescans workers
>> will not do any work.
>
> Um, what's different about that than before?
>
Earlier, we perform the rescan of all the nodes before ExecProcNode,
so workers
Amit Kapila writes:
> With this change, it is quite possible that during rescans workers
> will not do any work.
Um, what's different about that than before?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your
On Mon, Aug 28, 2017 at 1:59 AM, Tom Lane wrote:
> I wrote:
>> I think that the correct fix probably involves marking each parallel scan
>> plan node as dependent on a pseudo executor parameter, which the parent
>> Gather or GatherMerge node would flag as being changed on each rescan.
>> This woul
I wrote:
> I think that the correct fix probably involves marking each parallel scan
> plan node as dependent on a pseudo executor parameter, which the parent
> Gather or GatherMerge node would flag as being changed on each rescan.
> This would cue the plan layers in between that they cannot optimi
I wrote:
> 4. The fact that the test succeeds on many machines implies that the
> leader process is usually doing *all* of the work. This is in itself not
> very good. Even on the machines where it fails, the fact that the tuple
> counts are usually a pretty large fraction of the expected values
On Fri, Aug 18, 2017 at 3:50 AM, Tom Lane wrote:
> I wrote:
>> Ah-hah, I see my dromedary box is one of the ones failing, so I'll
>> have a look there. I can't reproduce it on my other machines.
>
> OK, so this is a whole lot more broken than I thought :-(.
>
> Bear in mind that the plan for this
I wrote:
> Ah-hah, I see my dromedary box is one of the ones failing, so I'll
> have a look there. I can't reproduce it on my other machines.
OK, so this is a whole lot more broken than I thought :-(.
Bear in mind that the plan for this (omitting uninteresting detail) is
Nested Loop Left Join
Robert Haas writes:
> On Thu, Aug 17, 2017 at 2:06 PM, Tom Lane wrote:
>> Nope, spoke too soon. See buildfarm.
> Whoa, that's not good.
Ah-hah, I see my dromedary box is one of the ones failing, so I'll
have a look there. I can't reproduce it on my other machines.
I'm a bit suspicious that i
On Thu, Aug 17, 2017 at 2:06 PM, Tom Lane wrote:
> I wrote:
>> Pushed, with minor tidying of the test case. I think we can now
>> close this open item.
>
> Nope, spoke too soon. See buildfarm.
>
> (Man, am I glad I insisted on a test case.)
Whoa, that's not good.
--
Robert Haas
EnterpriseDB:
I wrote:
> Pushed, with minor tidying of the test case. I think we can now
> close this open item.
Nope, spoke too soon. See buildfarm.
(Man, am I glad I insisted on a test case.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
Amit Kapila writes:
>> This will be fixed by the patch [1] (execrescan_gathermerge_v2.patch)
>> I posted sometime back. The test case is slightly different, but may
>> I can re post the patch with your test case.
> I have kept the fix as it is but changed the test to match your test.
Pushed, wi
Amit Kapila writes:
> I think the another patch posted above to add a new guc for
> enable_gather is still relevant and important.
FWIW, I'm pretty much -1 on that. I don't see that it has any
real-world use; somebody who wants to turn that off would likely
really want to turn off parallelism al
On Thu, Aug 17, 2017 at 8:08 PM, Amit Kapila wrote:
> On Thu, Aug 17, 2017 at 7:49 PM, Tom Lane wrote:
>> Amit Kapila writes:
>>> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane wrote:
I should think it wouldn't be that expensive to create a test
case, if you already have test cases that in
On Thu, Aug 17, 2017 at 7:49 PM, Tom Lane wrote:
> Amit Kapila writes:
>> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane wrote:
>>> I should think it wouldn't be that expensive to create a test
>>> case, if you already have test cases that invoke GatherMerge.
>>> Adding a right join against a VALUES
Amit Kapila writes:
> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane wrote:
>> I should think it wouldn't be that expensive to create a test
>> case, if you already have test cases that invoke GatherMerge.
>> Adding a right join against a VALUES clause with a small number of
>> entries, and a non-merg
On Thu, Aug 17, 2017 at 10:07 AM, Amit Kapila wrote:
> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane wrote:
>>
>>> I believe that between this commit and the test-coverage commit from
>>> Andres, this open item is reasonably well addressed. If someone
>>> thinks more needs to be done, please specify
On Thu, Aug 17, 2017 at 4:37 PM, Amit Kapila wrote:
> Note - enable_gathermerge is not present in postgresql.conf. I think
> we should add it in the postgresql.conf.sample file.
+1
See also
https://www.postgresql.org/message-id/CAEepm=0b7ym9mzsviq1d-hnt4koarvejvsfayvfyknv-pvd...@mail.gmail.com
On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane wrote:
> Robert Haas writes:
>> On Tue, Aug 15, 2017 at 7:31 AM, Amit Kapila wrote:
>>> Attached patch fixes the issue for me. I have locally verified that
>>> the gather merge gets executed in rescan path. I haven't added a test
>>> case for the same
On Tue, Aug 15, 2017 at 9:46 AM, Tom Lane wrote:
> How big a deal do we think test coverage is? It looks like
> ExecReScanGatherMerge is identical logic to ExecReScanGather,
> which *is* covered according to coverage.postgresql.org, but
> it wouldn't be too surprising if they diverge in future.
>
Robert Haas writes:
> On Tue, Aug 15, 2017 at 7:31 AM, Amit Kapila wrote:
>> Attached patch fixes the issue for me. I have locally verified that
>> the gather merge gets executed in rescan path. I haven't added a test
>> case for the same as having gather or gather merge on the inner side
>> of
On Tue, Aug 15, 2017 at 7:31 AM, Amit Kapila wrote:
> On Tue, Aug 15, 2017 at 8:22 AM, Noah Misch wrote:
>> On Fri, Aug 11, 2017 at 11:59:14AM -0400, Tom Lane wrote:
>>> "Adam, Etienne (Nokia-TECH/Issy Les Moulineaux)"
>>> writes:
>>> > ERROR: XX000: unrecognized node type: 90
>>> > LOCATION:
On Tue, Aug 15, 2017 at 8:22 AM, Noah Misch wrote:
> On Fri, Aug 11, 2017 at 11:59:14AM -0400, Tom Lane wrote:
>> "Adam, Etienne (Nokia-TECH/Issy Les Moulineaux)"
>> writes:
>> > ERROR: XX000: unrecognized node type: 90
>> > LOCATION: ExecReScan, execAmi.c:284
>>
>> (gdb) p (NodeTag) 90
>> $1
38 matches
Mail list logo