I wrote:
> Andres Freund writes:
>> I wonder if we could introduce a debug GUC that makes parallel worker
>> acquisition just retry in a loop, for a time determined by the GUC. That
>> obviously would be a bad idea to do in a production setup, but it could
>> be good enough for regression tests?
Andres Freund writes:
> I wonder if we could introduce a debug GUC that makes parallel worker
> acquisition just retry in a loop, for a time determined by the GUC. That
> obviously would be a bad idea to do in a production setup, but it could
> be good enough for regression tests? There are some
Hi,
On 2020-01-25 14:23:50 -0500, Tom Lane wrote:
> A side effect of this change is that per-worker JIT info is now
> printed one plan level further down: before it was printed on
> the Gather node, but now it's attached to the Gather's child,
> because that's where we print other per-worker
I wrote:
> It's still really unclear to me how we could exercise any of
> this behavior meaningfully in a regression test. I thought
> for a little bit about using the TAP infrastructure instead
> of a traditional-style test, but it seems like that doesn't
> buy anything except for a bias towards
Maciek Sakrejda writes:
> For what it's worth, this version makes sense to me.
Thanks for looking. Here's a version that deals with the JIT
instrumentation. As Andres noted far upthread, that was also
really bogusly done before. Not only could you get multiple "JIT"
subnodes on a Gather node,
Thanks for the thorough review. I obviously missed some critical
issues. I recognized some of the other maintainability problems, but
did not have a sense of how to best avoid them, being unfamiliar with
the code.
For what it's worth, this version makes sense to me.
Maciek Sakrejda writes:
> Okay. Does not getting as many workers as it asks for include
> sometimes getting zero, completely changing the actual output?
Yup :-(. We could possibly avoid that by running the explain
test by itself rather than in a parallel group, but I don't
especially want to
Okay. Does not getting as many workers as it asks for include
sometimes getting zero, completely changing the actual output? If so,
I'll submit a new version of the patch removing all tests--I was
hoping to improve coverage, but I guess this is not the way to start.
If not, can we keep the json
Maciek Sakrejda writes:
> Great, thank you. I noticed in the CF patch tester app, the build
> fails on Windows [1]. Investigating, I realized I had failed to fully
> strip volatile EXPLAIN info (namely buffers) in TEXT mode due to a
> bad regexp_replace.
You haven't gone nearly far enough in
Great, thank you. I noticed in the CF patch tester app, the build
fails on Windows [1]. Investigating, I realized I had failed to fully
strip volatile EXPLAIN info (namely buffers) in TEXT mode due to a
bad regexp_replace. I've fixed this in the attached patch (which I
also rebased against current
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
> Ah, I see. I think I got that from ExplainPrintSettings. I've
> corrected
On Wed, Jan 22, 2020 at 9:37 AM Georgios Kokolatos wrote:
> My bad, I should have been more clear. I meant that it is preferable to use
> the C99 standard which calls for declaring variables in the scope that you
> need them.
Ah, I see. I think I got that from ExplainPrintSettings. I've
>> + int num_workers = planstate->worker_instrument->num_workers;
>> + int n;
>> + worker_strs = (StringInfo *) palloc0(num_workers *
>> sizeof(StringInfo));
>> + for (n = 0; n < num_workers; n++) {
>>
>> I think C99 would be better here. Also no parenthesis needed.
>
>
>
Thanks! I'll fix the brace issues. Re: the other items:
> + int num_workers = planstate->worker_instrument->num_workers;
> + int n;
> + worker_strs = (StringInfo *) palloc0(num_workers *
> sizeof(StringInfo));
> + for (n = 0; n < num_workers; n++) {
>
> I think C99 would
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
> TEXT format was tricky due to its inconsistencies, but I think I have
>
TEXT format was tricky due to its inconsistencies, but I think I have
something working reasonably well. I added a simple test for TEXT
format output as well, using a similar approach as the JSON format
test, and liberally regexp_replacing away any volatile output. I
suppose in theory we could do
> Sounds good, I'll try that format. Any idea how to test YAML? With the
> JSON format, I was able to rely on Postgres' own JSON-manipulating
> functions to strip or canonicalize fields that can vary across
> executions--I can't really do that with YAML.
Yes, this approach was clear in the patch
Sounds good, I'll try that format. Any idea how to test YAML? With the
JSON format, I was able to rely on Postgres' own JSON-manipulating
functions to strip or canonicalize fields that can vary across
executions--I can't really do that with YAML. Or should I run EXPLAIN
with COSTS OFF, TIMING OFF,
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
The current version of the patch (v2) applies cleanly and does what it
Thanks for the review! I looked at and rebased the patch
on current master, ac5bdf6.
I introduced a new test file because this bug is specifically about
EXPLAIN output (as opposed to query execution or planning
functionality), and it didn't seem like a test would fit in any of the
other files. I
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
This is a high level review only. However, seeing that there is a conflict
Done! Thanks!
On Fri, Dec 27, 2019 at 12:31 AM Maciek Sakrejda wrote:
>
> I wanted to follow up on this patch since I received no feedback. What should
> my next steps be (besides rebasing, though I want to confirm there's interest
> before I do that)?
Given Andres' answer I'd say that there's interest in
I wanted to follow up on this patch since I received no feedback. What
should my next steps be (besides rebasing, though I want to confirm there's
interest before I do that)?
On Thu, Oct 24, 2019 at 6:48 PM Andres Freund wrote:
> Unfortunately I think the fix isn't all that trivial, due to the way we
> output the per-worker information at the end of ExplainNode(), by just
> dumping things into a string. It seems to me that a step in the right
> direction would be for
Hi,
On 2019-10-22 11:58:35 -0700, Maciek Sakrejda wrote:
> I originally reported this in pgsql-bugs [0], but there wasn't much
> feedback there, so moving the discussion here. When using JSON, YAML, or
> XML-format EXPLAIN on a plan that uses a parallelized sort, the Sort nodes
> list two
26 matches
Mail list logo
