On Thu, Nov 26, 2020 at 11:07 AM Greg Nancarrow wrote:
>
> On Thu, Nov 26, 2020 at 3:43 AM Tom Lane wrote:
> >
> > Thanks for looking! Pushed.
> >
> > At this point the cfbot is going to start complaining that the last-posted
> > patch in this thread no lo
better and more intuitive event name
than "session_start", or the suggested "user_backend_start".
I've therefore attached an updated patch with these suggested minor
improvements, please take a look and see what you think (please
compare with the original patch).
Regards,
Greg Nancarrow
Fujitsu Australia
on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch
Description: Binary data
ion. I tested this with pgAdmin4 and it worked fine for me,
to allow login to a database for which login was previously prevented
due to a badly-defined logon trigger.
Pavel, is this an acceptable solution or do you still see problems
with this approach?
Regards,
Greg Nancarrow
Fujitsu Australia
ns
>
I've created a separate patch to address question (1), rather than
include it in the main patch, which I've adjusted accordingly. I'll
leave question (2) until another time, as you suggest.
See the attached patches.
Regards,
Greg Nancarrow
Fujitsu Australia
v1-0001-Add-ne
ts or maybe rephrase it.
>
That is Amit's comment, and I'm reluctant to change it because it is
still applicable even after application of this patch.
Amit has previously suggested that I add an Assert here, to match the
comment (to replace the original Parallel-worker error-check that I
removed), so I am looking into that.
Regards,
Greg Nancarrow
Fujitsu Australia
On Thu, Dec 10, 2020 at 1:23 PM tsunakawa.ta...@fujitsu.com
wrote:
>
> From: Greg Nancarrow
> > Firstly, in order to perform parallel-safety checks in the case of
> > partitions, the
> > patch currently recursively locks/unlocks
> > (AccessShareLock) each partiti
o parallel-mode (so then there won't be an attempted
XID assignment).
If you remove the GetTransactionId() call from PrepareParallelMode()
and run "make installcheck-world" with "force_parallel_mode=regress"
in effect, many tests will fail with:
ERROR: cannot assign XIDs during a parallel operation
Regards,
Greg Nancarrow
Fujitsu Australia
OG_CONTENTS:
- fprintf(conn->Pfdebug, "%c ", v);
+ if (v != '\0')
+ fprintf(conn->Pfdebug, "%c ", v);
pqTraceMaybeBreakLine(sizeof(v), conn);
break;
Regards,
Greg Nancarrow
Fujitsu Australia
at one!
Thanks.
This was only ever meant to be a temporary fix for this bug that
affects this patch.
Regards,
Greg Nancarrow
Fujitsu Australia
ORIGINAL)" code block - even the so
called "original" query doesn't maintain the setting correctly (even
though the actual original query sent into the query rewriter does).
And also then the "if (!hasModifyingCTE)" test on the code following
immediately below it can be removed.
Regards,
Greg Nancarrow
Fujitsu Australia
nner then chose a
serial Insert + bitmap heap scan, because it then had a cheaper cost.
Of course we need to better understand the problem and observed
patters in order to get a better feel of how those costs should be
adjusted.
Regards,
Greg Nancarrow
Fujitsu Australia
On Sat, Feb 13, 2021 at 12:17 AM Amit Langote wrote:
>
> On Thu, Feb 11, 2021 at 4:43 PM Greg Nancarrow wrote:
> > On Thu, Feb 11, 2021 at 5:33 PM Greg Nancarrow wrote:
> > > On Tue, Feb 9, 2021 at 1:04 AM Amit Langote
> > > wrote:
> > > >
>
uot; -> "values" and "is" -> "are")
>
> Thanks for the comments, this is handled in the v23 patch attached.
> Thoughts?
>
I've marked this as "Ready for Committer".
(and also added you to the author list)
Regards,
Greg Nancarrow
Fujitsu Australia
s=1)
-> Bitmap Index Scan on x_a_idx (cost=0.00..1365.94
rows=73783 width=0) (actual time=9.097..9.097 rows=7 lo
ops=1)
Index Cond: (x.a < 8)
-> Bitmap Index Scan on x_a_idx (cost=0.00..1790.21
rows=96752 width=0) (actual time=11.591..11.591 rows=10
loops=1)
Index Cond: (x.a > 19990)
Planning Time: 0.205 ms
Execution Time: 607.734 ms
(14 rows)
Even when I changed the queries to return more rows from the scan, to
the point where it chose not to use a parallel INSERT bitmap heap scan
(in favour of parallel seq scan), and then forced it to by disabling
seqscan, I found that it was still at least as fast as serial INSERT
with bitmap heap scan.
Regards,
Greg Nancarrow
Fujitsu Australia
On Wed, Feb 17, 2021 at 12:19 AM Amit Langote wrote:
>
> On Mon, Feb 15, 2021 at 4:39 PM Greg Nancarrow wrote:
> > On Sat, Feb 13, 2021 at 12:17 AM Amit Langote
> > wrote:
> > > On Thu, Feb 11, 2021 at 4:43 PM Greg Nancarrow
> > > wrote:
> > > &g
pping that trigger and then
again using EXPLAIN to see what plan is generated, then I'd expect a
parallel-plan to be generated, but with the setrefs-v3.patch it still
generates a non-parallel plan. So I think the "&&
glob->parallelModeNeeded" part of test needs to be removed.
Regards,
Greg Nancarrow
Fujitsu Australia
relying on.
I'll give it a go and see if I can detect any issues with this method.
Regards,
Greg Nancarrow
Fujitsu Australia
ther node handling).
>
I think we should try to be consistent with current plan-cache
functionality, rather than possibly inventing new rules for
partitions.
I'm finding that with current Postgres functionality (master branch),
if there are two parallel-unsafe triggers defined on a normal table
and one is dropped, it results in replanning and it yields the same
(non-parallel) plan. This would seem to go against what you are
suggesting.
Regards,
Greg Nancarrow
Fujitsu Australia
elationGetIndexPredicate(index_rel);
+if (ii_Predicate != NIL)
+{
+if (max_parallel_hazard_walker((Node *)ii_Predicate, context))
+{
+found_max_hazard = true;
+}
+}
+ }
Also will need a bit of renaming of that function.
I'll include this in the next patch update.
Regards,
Greg Nancarrow
Fujitsu Australia
On Tue, Feb 23, 2021 at 12:33 AM Amit Kapila wrote:
>
> On Mon, Feb 22, 2021 at 8:41 AM Greg Nancarrow wrote:
> >
> > On Fri, Feb 19, 2021 at 9:38 PM Amit Kapila wrote:
> > >
> > > On Thu, Feb 18, 2021 at 11:05 AM Amit Langote
> > > wrote:
>
On Tue, Feb 23, 2021 at 2:30 PM Amit Kapila wrote:
>
> On Tue, Feb 23, 2021 at 6:37 AM Greg Nancarrow wrote:
> >
> > On Tue, Feb 23, 2021 at 12:33 AM Amit Kapila
> > wrote:
> > >
> > > On Mon, Feb 22, 2021 at 8:41 AM Greg Nancarrow
> > > wro
On Tue, Feb 23, 2021 at 10:53 PM Amit Kapila wrote:
>
> On Tue, Feb 23, 2021 at 4:47 PM Greg Nancarrow wrote:
> >
> > On Tue, Feb 23, 2021 at 2:30 PM Amit Kapila wrote:
> > >
> > > On Tue, Feb 23, 2021 at 6:37 AM Greg Nancarrow
> > > wrote:
>
On Wed, Feb 24, 2021 at 3:12 PM Amit Kapila wrote:
>
> On Wed, Feb 24, 2021 at 8:41 AM Greg Nancarrow wrote:
> >
> > On Tue, Feb 23, 2021 at 10:53 PM Amit Kapila
> > wrote:
> > >
> > > > But the non-parallel plan was chosen (instead of a parallel pl
On Wed, Feb 24, 2021 at 8:39 PM Amit Kapila wrote:
>
> On Fri, Feb 19, 2021 at 6:56 AM Greg Nancarrow wrote:
> >
> > Posting a new version of the patches, with the following updates:
> >
>
> I am not happy with the below code changes, I think we need a
posed by you quite ad-hoc and don't think we
> can go that way.
>
At least my current fix is very specific, efficient and clear in its
purpose, and suitably documented, so it is very clear when and how it
is to be removed, when the issue is fixed in the query rewriter.
Another concern with the alternative fix is that it always searches
for a modifying CTE, even when parse->hasModifyingCTE is true after
the query rewriter processing.
Regards,
Greg Nancarrow
Fujitsu Australia
On Thu, Feb 25, 2021 at 12:19 AM Amit Kapila wrote:
>
> On Wed, Feb 24, 2021 at 6:21 PM Greg Nancarrow wrote:
> >
> > On Wed, Feb 24, 2021 at 10:38 PM Amit Kapila
> > wrote:
> > > >
> > > > Thanks, I'll try it.
> > > > I di
result in a cleaner patch overall.
>
Thanks Amit,
I was able to reproduce the problem using your instructions (though I
found I had to run that explain an extra time, in order to hit the
breakpoint).
Also, I can confirm that the problem doesn't occur after application
of your patch.
I'll leave it to your better judgement as to what to do next - if you
feel the current tracking method is not sufficient
Regards,
Greg Nancarrow
Fujitsu Australia
On Wed, Mar 3, 2021 at 2:49 PM vignesh C wrote:
>
>
>
> On Wed, Mar 3, 2021 at 7:37 AM Tom Lane wrote:
> >
> > Greg Nancarrow writes:
> > > I've marked this as "Ready for Committer".
> >
> > I've pushed this after whacking it aro
On Tue, Mar 2, 2021 at 11:19 PM Amit Kapila wrote:
>
> On Mon, Mar 1, 2021 at 9:08 AM Greg Nancarrow wrote:
> >
> > Posting an updated set of patches that includes Amit Langote's patch
> > to the partition tracking scheme...
> >
>
> Few comments:
&
On Wed, Mar 3, 2021 at 10:45 PM Dilip Kumar wrote:
>
> On Mon, Mar 1, 2021 at 9:08 AM Greg Nancarrow wrote:
> >
> > Posting an updated set of patches that includes Amit Langote's patch
> > to the partition tracking scheme...
> > (the alternative of adding
On Thu, Mar 4, 2021 at 1:07 AM Dilip Kumar wrote:
>
> On Wed, Mar 3, 2021 at 5:50 PM Greg Nancarrow wrote:
> >
> > Asserts are normally only enabled in a debug-build, so for a
> > release-build that Assert has no effect.
> > The Assert is being used as a sanity-c
right? If so, why not just maintain them in plannedstmt->partitionOids
> and then make PlanCacheRelCallback consider it?
Maybe Amit Langote can kindly comment on this, as it would involve
updates to his prior partition-related fixes.
>Also, in
> standard_planner, we should add these partitionOids only for
> parallel-mode.
>
It is doing that in v20 patch (what makes you think it isn't?).
Regards,
Greg Nancarrow
Fujitsu Australia
On Thu, Mar 4, 2021 at 11:05 PM Amit Kapila wrote:
>
> On Thu, Mar 4, 2021 at 5:24 PM Greg Nancarrow wrote:
> >
> > On Thu, Mar 4, 2021 at 10:07 PM Amit Kapila wrote:
> > >
> >
> > >Also, in
> > > standard_planner, we should add these partition
On Sat, Mar 6, 2021 at 9:19 PM Amit Kapila wrote:
>
> On Fri, Mar 5, 2021 at 6:34 PM Greg Nancarrow wrote:
> >
> > For the time being at least, I am posting an updated set of patches,
> > as I found that the additional parallel-safety checks on DOMAIN check
> &
RowExclusiveLock.
> You can see that that's what AcquireExecutorLocksOnPartitions() will
> do in the generic plan case.
>
OK, I see what you mean, best to use the target partitioned table's
RTE's rellockmode in this case then.
I'll update the patch accordingly.
Regards,
Greg Nancarrow
Fujitsu Australia
t this functionality will
> get tested and if we found reports of regression then we can turn it
> off for this release as well.
>
I'd agree to keeping it on by default (and it can be selectively
turned off for a particular table using the reloption, if needed).
Regards,
Greg Nancarrow
Fujitsu Australia
ead of just the trigdesc pointer.
>
> BTW, having special logic for FK triggers in
> target_rel_trigger_max_parallel_hazard seems quite loony to me.
> Why isn't that handled by setting appropriate proparallel values
> for those trigger functions?
>
Thanks Tom for your investigation, detailed analysis and suggested code fixes.
Will work on getting these issues corrected ASAP (and, er, removing
the looniness ...).
Regards,
Greg Nancarrow
Fujitsu Australia
azard seems quite loony to me.
> Why isn't that handled by setting appropriate proparallel values
> for those trigger functions?
>
... and also attached a patch to update the code for this issue.
(2nd patch relies on application of the 1st patch)
Thanks again for pointing out these p
ted
> cases during this stage of the planner.
>
Posting an updated Parallel INSERT patch which (mostly) addresses
previously-identified issues and suggestions.
More work needs to be done in order to support parallel UPDATE and
DELETE (even after application of Thomas Munro's combo-cid
para
fe) and a partition may itself be a partitioned table.
> 4. Have you checked the overhead of this on the planner for different
> kinds of statements like inserts into tables having 100 or 500
> partitions? Similarly, it is good to check the overhead of domain
> related checks added in the pa
t too bad).
Some representative timings for a parallel INSERT of a millions rows
into 100,250 and 500 partitions are shown below.
(1) Without patch
# PartitionsPlanning Time (ms)
Execution Time (ms)
1001.014
4176.435
250
27;t invent that error check, it's found in several other places
in the Postgres code (that error should only ever occur if the
database has been corrupted or intentionally invalidated).
Having said that, I agree that perhaps it's best to switch to
non-parallel mode in this case, but this wouldn't stop it erroring out
when the plan is actually run.
> -> We could include a few tests for this in regression.
>
Looking at adding relevant test cases.
> -> We might need some documentation update like in
> parallel-query.html/parallel-plans.html, etc
>
Looking at doc updates.
Regards,
Greg Nancarrow
Fujitsu Australia
assume it's parallel-unsafe
because the parallel-safety can't be determined without blocking on
the lock. This is not ideal, but I'm not sure of what other approach
could be used and I am somewhat reluctant to change that test. If
anybody is familiar with the "partition-concurrent-at
hed a small patch that corrects the issue.
Regards,
Greg Nancarrow
Fujitsu Australia
v1-0001-Fix-crash-in-virtual-file-descriptor-FDDEBUG-code.patch
Description: Binary data
nd (cost=0.00..747235.71 rows=29166714 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
-> Parallel Seq Scan on drop_me
(cost=0.00..601402.13 rows=29166713 width=0)
(7 rows)
Regards,
Greg Nancarrow
Fujitsu Australia
s is it valid to use __has_attribute as a preprocessor
operator. As a result, combining the two tests into a single
expression as shown below would only be valid with a compiler that
supports the operator but not with others that don’t. "
(Thanks to my colleague Peter Smith for finding the doc explanation)
Regards,
Greg Nancarrow
Fujitsu Australia
-> Result (cost=0.00..0.01 rows=1 width=0)
-> Parallel Seq Scan on drop_me
(cost=0.00..601402.13 rows=29166713 width=0)
(7 rows)
Regards,
Greg Nancarrow
Fujitsu Australia
On Wed, Nov 25, 2020 at 12:07 PM Tom Lane wrote:
>
>
> Here's a v2 that does it like that.
>
Looks OK to me.
Regards,
Greg Nancarrow
Fujitsu Australia
or Robert Haas (to
name two) might like to comment on whether what we're looking at is indeed
a bug/deficiency and worth fixing, and whether Luc is correct in his
expressed approach on what would be required to fix it?
Regards,
Greg Nancarrow
Fujitsu Australia
over to the next CF with the WOA status.
I am actively working on processing the feedback and updating the
patch, and hope to post an update next week sometime.
Regards,
Greg Nancarrow
Fujitsu Australia
coercion is required and can be done, the
parse/analyze code doesn't actually modify the nodes in place "for
fear of changing the semantics", so when the types don't exactly match
it's all left up to the planner, but for this parse tree it fails to
produce a parallel p
On Fri, Jan 8, 2021 at 8:25 PM vignesh C wrote:
>
> For one of the test you can validate that the same transaction has
> been used by all the parallel workers, you could use something like
> below to validate:
> SELECT COUNT(*) FROM (SELECT DISTINCT cmin,xmin FROM para_insert_p1) as dt;
>
> Few
with. Such
cases could surely ever only happen if the DB had been corrupted, so
extremely rare IMHO and most likely to have caused an ERROR elsewhere
before ever reaching here...
I can add some Asserts to the current code, to better alert for such
cases, for when asserts are enabled, but otherwise I think that the
current code is OK (cleaning up other Postgres code is beyond the
scope of the task here).
Regards,
Greg Nancarrow
Fujitsu Australia
ts
> of code? If we don't have a solid reason then I suggest keeping these
> checks and errors the same as in other parts of the code.
>
The checks are the same as done in existing Postgres source - but
instead of failing with an ERROR (i.e. whole backend dies), in the
middle of parallel-safety-checking, it has been changed to regard the
operation as parallel-unsafe, so that it will try to execute in
non-parallel mode, where it will most likely fail too when those
corrupted attributes are accessed - but it will fail in the way that
it currently does in Postgres, should that very rare condition ever
happen. This was suggested by Vignesh, and I agree with him. So in
effect, it's just allowing it to use the existing error paths in the
code, rather than introducing new ERROR points.
Regards,
Greg Nancarrow
Fujitsu Australia
ch will stop dead with an ERROR in the middle of
parallel-safety checking should one of these rare conditions ever
occur.
Regards,
Greg Nancarrow
Fujitsu Australia
n expression (which could be altered
from being parallel-safe to parallel-unsafe).
Regards,
Greg Nancarrow
Fujitsu Australia
ed in nodeModifyTable.c
>
I think you meant nodeGather.c (not nodeModifyTable.c).
However, the include file (executor/nodeModifyTable.h) is actually
required here, otherwise there are build warnings.
Regards,
Greg Nancarrow
Fujitsu Australia
I think it is better to add comments along with this change. In this
> form, this looks quite hacky to me.
>
The targetlist on the ModifyTable node has been setup correctly, but
it hasn't been propagated to the Gather node.
Of course, I have previously tried to elegantly fix this, but struck
various problems, using different approaches.
Perhaps this code could just be moved into set_plan_refs().
For the moment, I'll just keep the current code, but I'll add a FIXME
comment for this.
I'll investigate Antonin's suggestions as a lower-priority side-task.
Regards,
Greg Nancarrow
Fujitsu Australia
de).
Note that Vignesh originally suggested changing it from
"elog(ERROR,...)" to "elog(WARNING,...)", and I agree with him.
Regards,
Greg Nancarrow
Fujitsu Australia
ease wrap long line.
>
OK.
I thought I ran pg_indent fairly recently, but maybe it chose not to
wrap that line.
> + uint64 *processed_count_space;
>
> If I read the code correctly, it seems it can be dropped (use
> pei->processed_count directly).
>
You're ri
ate and address them in the next version of
the patch I post.
Regards,
Greg Nancarrow
Fujitsu Australia
*/
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
RelationUsesLocalBuffers(rel))
{
- table_close(rel, lockmode);
- context->max_hazard = PROPARALLEL_UNSAFE;
- return true;
+ if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+ {
+ table_close(rel, lockmode);
+ return true;
+ }
}
Regards,
Greg Nancarrow
Fujitsu Australia
that there seems to be quite a few places in the
Postgres code where RelationGetIndexList() is being called without a
corresponding list_free() being called - obviously instead relying on
it being deallocated when the caller's memory-context is destroyed.
Regards,
Greg Nancarrow
Fujitsu Australia
en the memory context is destroyed.
Anyway, perhaps the concerns of many partitions and the recursive
nature of these checks overrides that, because, as you say, possible
high memory usage.
Regards,
Greg Nancarrow
Fujitsu Australia
unsafe */
glob->maxParallelHazard = PROPARALLEL_UNSAFE;
glob->parallelModeOK = false;
}
When I debugged this (transformWithClause()), the WITH clause was
found to contain a modifying CTE and for the INSERT
query->hasModifyingCTE was set true.
But somehow in the re-writer code, this got lost.
Bug?
Ideas?
Regards,
Greg Nancarrow
Fujitsu Australia
he
> IndexInfo.
> It can avoid some unnecessary cost.
> And in this solution we do not need to remove expression_planner.
>
OK, maybe this is a good idea, but I do recall trying to minimize this
kind of processing before, but there were cases which broke it.
Have you actually tried your idea and run all regression tests and
verified that they passed?
In any case, I'll look into it.
Regards,
Greg Nancarrow
Fujitsu Australia
Based on above, May be we do not need to call BuildIndexInfo to build the
> IndexInfo.
> It can avoid some unnecessary cost.
> And in this solution we do not need to remove expression_planner.
>
Hmmm, when I debugged my simple test case, I found rel->rd_index was
NULL, so it seems that the call to BuildIndexInfo is needed.
(have I understood your suggestion correctly?)
Regards,
Greg Nancarrow
Fujitsu Australia
the Assert above is changed to not test the commandType, the same
error occurs as before.
This appears to possibly be some kind of bug in which hasModifyingCTE
is not getting set, at least in this particular INSERT case, but in
the current Postgres code it doesn't matter because all INSERTs are
considered parallel-unsafe, so won't be executed in parallel-mode
anyway.
I notice that if I execute "SELECT * from t1" instead of "INSERT INTO
bug6051 SELECT * from t1", then "hasModifyingCTE" is getting set to
true for the query, and thus is always considered parallel-unsafe.
Regards,
Greg Nancarrow
Fujitsu Australia
}
-index_expr_item =
lnext(index_info->ii_Expressions, index_expr_item);
+index_expr_item = lnext(ii_Expressions, index_expr_item);
}
}
Regards,
Greg Nancarrow
Fujitsu Australia
worried that
> PrepareParallelMode() call is missing there. Can we just add an argument to
> EnterParallelMode()? Other call sites can use CMD_UNKNOWN or CMD_UTILITY, if
> we want to use CMD_XX.
>
I really can't see a problem. PrepareParallelMode() is only needed
prior to execution of a parallel plan, so it's not needed for "other
call sites" using EnterParallelMode().
Perhaps the name can be changed to disassociate it from generic
EnterParallelMode() usage. So far, I've only thought of long names
like: PrepareParallelModePlanExec().
Ideas?
Regards,
Greg Nancarrow
Fujitsu Australia
.
has a foreign
key, and an insert into such a table will generate a new commandId (so we
must avoid that, as we don't currently have the technology to support
sharing of new command IDs across the participants in the parallel
operation). This is what the code comment says, It does not say that such a
trigger generates a new command ID.
See Amit's updated comment here:
https://github.com/postgres/postgres/commit/0d32511eca5aec205cb6b609638ea67129ef6665
In addition, the 2nd patch has an explicit test case for this (testing
insert into a table that has a FK).
If you have a test case that breaks the existing patch, please let me know.
Regards,
Greg Nancarrow
Fujitsu Australia
o test parallel unsafe partition ?
>
>
>
OK, I have to create a more complex table to test those other potential
parallel-safety issues of partitions (other than what was tested before the
recursive call, or support functions and expression in index key), but
since it's a recursive call, invoking code that's already been tested, I
would not anticipate any problems.
Thanks,
Greg Nancarrow
Fujitsu Australia
e can just be changed to
something specific to plan execution, so nobody gets confused with a
name like "PrepareParallelMode()", which as you point out sounds
generic to all callers of EnterParallelMode().
Regards,
Greg Nancarrow
Fujitsu Australia
-0003-Enable-parallel-INSERT-and-or-SELECT-for-INSERT-INTO.patch),
which implements parallel INSERT, the wording for this comment is
again altered, to reflect the fact that parallel workers also write
into the table.
Regards,
Greg Nancarrow
Fujitsu Australia
ms do not need the lockmode param with the above define.
>
>
Yeah, the repeated cleanup at the point of return is a bit ugly.
It could be solved by changing the function to do cleanup at a common
return point, but I agree with you that in this case it could simply
be done outside the function.
Thanks, I'll make that change.
Regards,
Greg Nancarrow
Fujitsu Australia
mongst the workers, even if the function/expression is
parallel-safe).
For the additional parallel-safety checks for INSERT, currently we
check that RTE_SUBQUERY is in the range-table. So I think we can
additionally check that RTE_RELATION is in the subquery range-table
(otherwise treat it as unsafe).
Regards,
Greg Nancarrow
Fujitsu Australia
here
are some issues in the documentation updates (grammatical issues and
consistency issues with current documentation), and also some of the
explanations are not clear. I guess I can address these separately.
Regards,
Greg Nancarrow
Fujitsu Australia
Query *query = (Query *) node;
>
> /* Recurse into subselects */
> return query_tree_walker(query, relation_walker,
> NULL,
> QTW_EXAMINE_RTES_BEFORE);
> }
>
>
re
against your v2 patches, and see whether you agree with the changes to
the wording.
In the documentation, you will also notice that in your V2 patch, it
says that the "parallel_dml_enabled" table option defaults to false.
As it actually defaults to true, I changed that in the docu
specific to our needs, I think we should not try to do
this optimization, because there's too much overhead. Also, keep in
mind that for the current parallel SELECT functionality in Postgres, I
don't see any similar optimization being attempted (and such
optimization should be attempted at the SELECT level). So I don't
think we should be attempting such optimization in this patch (but
could be attempted in a separate patch, just related to current
parallel SELECT functionality).
Regards,
Greg Nancarrow
Fujitsu Australia
(expected,
because it does extra checks), but the Execution Time was very similar
for both the patched and unpatched versions.
Regards,
Greg Nancarrow
Fujitsu Australia
tter way to fix the issue -
for example, during the re-writing, rather than after the query has
been re-written.
Regards,
Greg Nancarrow
Fujitsu Australia
ewriteRuleAction(Query *parsetree,
*sub_action_ptr = sub_action;
else
rule_action = sub_action;
+
+if (parsetree->hasModifyingCTE)
+sub_action->hasModifyingCTE = true;
}
/*
I'll do some further checks, because the rewriting is recursive and
tricky, so don't want to miss any cases ...
Regards,
Greg Nancarrow
Fujitsu Australia
_action
in this case - but not 100% sure on it yet - still checking that. All
tests run so far pass without doing that though.
This is one reason for my original approach (though I admit, it was
not optimal) because at least it was reliable and detected the
modifyingCTE after all the rewriting and kludgy code had finished.
Regards,
Greg Nancarrow
Fujitsu Australia
rue;
+if (sub_action_ptr)
+ rule_action->hasModifyingCTE = true;
+}
}
/*
Regards,
Greg Nancarrow
Fujitsu Australia
n->hasModifyingCTE = true;
+ else
+ Assert(sub_action == rule_action);
+ }
}
/*
Regards,
Greg Nancarrow
Fujitsu Australia
LECT * FROM t1;
produces the error:
ERROR: cannot assign XIDs during a parallel operation
Regards,
Greg Nancarrow
Fujitsu Australia
v1-0001-Fix-bug-in-the-query-rewriter-hasModifyingCTE-not-set.patch
Description: Binary data
On Sun, Feb 7, 2021 at 10:03 AM Tom Lane wrote:
>
> Greg Nancarrow writes:
> > I found a bug in the query rewriter. If a query that has a modifying
> > CTE is re-written, the hasModifyingCTE flag is not getting set in the
> > re-written query.
>
> Ugh.
>
>
n your testing?
When I ran these tests with the Parallel INSERT patch applied, it did
not naturally choose a parallel plan for any of these cases.
So we can hardly blame the parallel insert with bitmap heap scan for
having worse performance, when based on costings, it doesn't actually
choose to use a parallel plan in this case.
Regards,
Greg Nancarrow
Fujitsu Australia
uery plan for the
> +INSERT statement. For more information about Parallel
> Safety,
> +see .
> +
>
Thanks. You're right, I should probably update the docs to clarify
those two cases.
(I had removed them from the list of parallel-unsafe things, but not
pointed ou
rite transactions. The standby mode state is
determined by either
the value of the in_hot_standby configuration
parameter, that is reported by the server (if supported) upon
successful connection, or is otherwise explicitly queried by sending
SELECT pg_is_in_recovery() after succ
On Tue, Feb 9, 2021 at 1:04 AM Amit Langote wrote:
>
> Greg, all
>
> Thanks a lot for your work on this.
>
> On Mon, Feb 8, 2021 at 3:53 PM Greg Nancarrow wrote:
> > Posting an updated set of patches.
>
> I've been looking at these patches, initially with
conf file, maybe you can have a look. Besides, I didn't
> > do any parameters tuning in my test session.
>
> I reproduced this on my machine.
>
> I think we'd better do "analyze" before insert which helps reproduce this
> easier.
> Like:
>
> -
> analyze;
> e
are
emitted separately and are not included in each error message string
(as parameters in a format string), I think those functions should use
appendPQExpBufferStr() instead of appendPQExpBuffer(), as it's more
efficient if there is just a single string argument.
Regards,
Greg Nancarrow
Fujitsu Australia
t; at the bottom of set_plan_refs(), simply
pointing the Gather node's targetlist to subplan's ModifyTable
targetlist.
if (nodeTag(plan) == T_Gather)
{
Plan *subplan = plan->lefttree;
if (IsA(subplan, ModifyTable) &&
castNode(ModifyTable, subplan)->returningLists != NIL)
{
plan->targetlist = subplan->targetlist;
}
}
Regards,
Greg Nancarrow
Fujitsu Australia
r the "INSERT...VALUES" case
(apart from the fact that spawning off parallel workers to insert that
data would almost always result in worse performance than a
non-parallel plan...)
The division of work for parallel workers is part of the table AM
(scan) implementation, which is not invoked for "INSERT...VALUES".
Regards,
Greg Nancarrow
Fujitsu Australia
ssible_for_modify(Query *parse)
>
>
>
>
>
> And I put the function at earlier place like the following:
>
>
> if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
> IsUnderPostmaster &&
> (parse->commandType == CMD_SELECT ||
> - (enable_parallel_dml &&
> - IsModifySupportedInParallelMode(parse->commandType))) &&
> + is_parallel_possible_for_modify(parse)) &&
> !parse->hasModifyingCTE &&
> max_parallel_workers_per_gather > 0 &&
> !IsParallelWorker())
>
>
> If this looks good, maybe we can merge this change.
>
If I've understood correctly, you're suggesting to merge the
"is_parallel_possible_for_modify()" code from your parallel_dml patch
into the main parallel INSERT patch, right?
If so, I think that's a good idea, as it will help simplify both
patches (and then, if need be, we can still discuss where best to
place certain checks ...).
I'll post an update soon that includes that change (then the
parallel_dml patch will need to be rebased accordingly).
Regards,
Greg Nancarrow
Fujitsu Australia
OIDs seem to be handled through a different mechanism)..
Can you elaborate on what you believe is required here, so that the
partition OID dependency is registered and the altered partition
results in the plan being invalidated?
Thanks in advance for any help you can provide here.
Regards,
Greg Nancarrow
Fujitsu Australia
On Thu, Feb 11, 2021 at 5:33 PM Greg Nancarrow wrote:
>
> On Tue, Feb 9, 2021 at 1:04 AM Amit Langote wrote:
> >
> > * I think that the concerns raised by Tsunakawa-san in:
> >
> > https://www.postgresql.org/message-id/TYAPR01MB2990CCB6E24B10D35D28B949
alues of the
+default_transaction_read_only and
+in_hot_standby configuration parameters, that are
+reported by the server (if supported) upon successful connection. If
(i.e. "value" -> "values" and "is" -> "are")
Regards,
Greg Nancarrow
Fujitsu Australia
1 - 100 of 431 matches
Mail list logo
