fixing subplan/subquery confusion
On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
In practice, we don't yet have the ability for
parallel-safe paths from subqueries to affect planning at higher query
levels, but that's because the pathification stuff landed too late in
the cycle for me to fully react to it.I wonder if that's not just from confusion between subplans and
subqueries. I don't believe any of the claims made in the comment
adjusted in the patch below (other than "Subplans currently aren't passed
to workers", which is true but irrelevant). Nested Gather nodes is
something that you would need, and already have, a defense for anyway.[Action required within 72 hours. This is a generic notification.]
The above-described topic is currently a PostgreSQL 9.6 open item ("fix
possible confusion between subqueries and subplans"). Robert, since you
committed the patch believed to have created it, you own this open item. If
some other commit is more relevant or if this does not belong as a 9.6 open
item, please let us know. Otherwise, please observe the policy on open item
ownership[1] and send a status update within 72 hours of this message.
Include a date for your subsequent status update. Testers may discover new
open items at any time, and I want to plan to get them all fixed well in
advance of shipping 9.6rc1. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
This open item comes with a patch submitted by Tom Lane. If Tom wants
me to review and (if no problems are found) commit that patch to
resolve this open item, I'm willing to do that. But generally I don't
commit patches submitted by other committers unless that person and I
have agreed on it in advance, which is not currently the case here.
Tom, do you want to commit this, or do you want me to handle it, or
something else?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
The above-described topic is currently a PostgreSQL 9.6 open item ("fix
possible confusion between subqueries and subplans").
This open item comes with a patch submitted by Tom Lane. If Tom wants
me to review and (if no problems are found) commit that patch to
resolve this open item, I'm willing to do that. But generally I don't
commit patches submitted by other committers unless that person and I
have agreed on it in advance, which is not currently the case here.
Tom, do you want to commit this, or do you want me to handle it, or
something else?
I was not sure if you'd agreed that the patch was correct, and in any
case I thought you wanted to fold it into the upperrel consider_parallel
patch. I feel no great need to commit it myself, if that's what you
meant.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 27, 2016 at 4:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
The above-described topic is currently a PostgreSQL 9.6 open item ("fix
possible confusion between subqueries and subplans").This open item comes with a patch submitted by Tom Lane. If Tom wants
me to review and (if no problems are found) commit that patch to
resolve this open item, I'm willing to do that. But generally I don't
commit patches submitted by other committers unless that person and I
have agreed on it in advance, which is not currently the case here.Tom, do you want to commit this, or do you want me to handle it, or
something else?I was not sure if you'd agreed that the patch was correct, and in any
case I thought you wanted to fold it into the upperrel consider_parallel
patch. I feel no great need to commit it myself, if that's what you
meant.
OK, I'll set aside some time for further review and either commit it
or send an update by COB Thursday US time.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 28, 2016 at 1:42 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jun 27, 2016 at 4:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
The above-described topic is currently a PostgreSQL 9.6 open item ("fix
possible confusion between subqueries and subplans").This open item comes with a patch submitted by Tom Lane. If Tom wants
me to review and (if no problems are found) commit that patch to
resolve this open item, I'm willing to do that. But generally I don't
commit patches submitted by other committers unless that person and I
have agreed on it in advance, which is not currently the case here.Tom, do you want to commit this, or do you want me to handle it, or
something else?I was not sure if you'd agreed that the patch was correct, and in any
case I thought you wanted to fold it into the upperrel consider_parallel
patch. I feel no great need to commit it myself, if that's what you
meant.OK, I'll set aside some time for further review and either commit it
or send an update by COB Thursday US time.
I had couple of questions [1]/messages/by-id/CAA4eK1+yGs-onuJDy+TTqnrnT0hty_QQPC1GipS+ce-W3872QQ@mail.gmail.com related to that patch. See if you find
those as relevant?
[1]: /messages/by-id/CAA4eK1+yGs-onuJDy+TTqnrnT0hty_QQPC1GipS+ce-W3872QQ@mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Kapila <amit.kapila16@gmail.com> writes:
I had couple of questions [1] related to that patch. See if you find
those as relevant?
I do not think those cases are directly relevant: you're talking about
appendrels not single, unflattened RTE_SUBQUERY rels.
In the subquery case, my view of how it ought to work is that Paths coming
up from the subquery would be marked as not-parallel-safe if they contain
references to unsafe functions. It might be that that doesn't happen for
free, but my guess is that it would already work that way given a change
similar to what I proposed.
In the appendrel case, I tend to agree that the easiest solution is to
scan all the children of the appendrel and just mark the whole thing as
not consider_parallel if any of them have unsafe functions.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 28, 2016 at 8:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
I had couple of questions [1] related to that patch. See if you find
those as relevant?I do not think those cases are directly relevant: you're talking about
appendrels not single, unflattened RTE_SUBQUERY rels.
Right, but still I think we shouldn't leave the appendrel case unattended.
In the subquery case, my view of how it ought to work is that Paths coming
up from the subquery would be marked as not-parallel-safe if they contain
references to unsafe functions. It might be that that doesn't happen for
free, but my guess is that it would already work that way given a change
similar to what I proposed.
This makes sense to me.
In the appendrel case, I tend to agree that the easiest solution is to
scan all the children of the appendrel and just mark the whole thing as
not consider_parallel if any of them have unsafe functions.
Thats what I had in mind as well, but not sure which is the best place
to set it. Shall we do it in set_append_rel_size() after setting the
size of each relation (after foreach loop) or is it better to do it in
set_append_rel_pathlist(). Is it better to do it as a separate patch
or to enhance your patch for this change? If you are okay, I can
update the patch or write a new one based on what is preferred?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 28, 2016 at 5:22 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Jun 28, 2016 at 8:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
In the appendrel case, I tend to agree that the easiest solution is to
scan all the children of the appendrel and just mark the whole thing as
not consider_parallel if any of them have unsafe functions.Thats what I had in mind as well, but not sure which is the best place
to set it. Shall we do it in set_append_rel_size() after setting the
size of each relation (after foreach loop) or is it better to do it in
set_append_rel_pathlist(). Is it better to do it as a separate patch
or to enhance your patch for this change?
I have done it as a separate patch. I think doing it in
set_append_rel_size() has an advantage that we don't need to scan the
child rels separately. If you think that attached patch is on right
lines, then I can add test cases as well.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
set-consider-parallel-append-rels-v1.patchapplication/octet-stream; name=set-consider-parallel-append-rels-v1.patchDownload
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 017cafb..0bd8bc9 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -966,9 +966,6 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
continue;
}
- /* Copy consider_parallel flag from parent. */
- childrel->consider_parallel = rel->consider_parallel;
-
/*
* CE failed, so finish copying/modifying targetlist and join quals.
*
@@ -989,6 +986,24 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
appinfo);
/*
+ * baserestrictinfo or reltarget for childrel may have been modified,
+ * so check if any one of those contains parallel-restricted clause.
+ * Even if one of the child rels is not parallel safe, parent rel
+ * should not be considred for parallelization.
+ */
+ if (root->glob->parallelModeOK && rel->consider_parallel)
+ {
+ if (has_parallel_hazard((Node *) childrel->baserestrictinfo, false) ||
+ has_parallel_hazard((Node *) childrel->reltarget->exprs, false))
+ {
+ childrel->consider_parallel = false;
+ rel->consider_parallel = false;
+ }
+ else
+ childrel->consider_parallel = true;
+ }
+
+ /*
* We have to make child entries in the EquivalenceClass data
* structures as well. This is needed either if the parent
* participates in some eclass joins (because we will want to consider
@@ -1250,7 +1265,7 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
/*
* Consider an append of partial unordered, unparameterized partial paths.
*/
- if (partial_subpaths_valid)
+ if (partial_subpaths_valid && rel->consider_parallel)
{
AppendPath *appendpath;
ListCell *lc;
On Mon, Jun 27, 2016 at 4:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Tom, do you want to commit this, or do you want me to handle it, or
something else?I was not sure if you'd agreed that the patch was correct, and in any
case I thought you wanted to fold it into the upperrel consider_parallel
patch. I feel no great need to commit it myself, if that's what you
meant.OK, I'll set aside some time for further review and either commit it
or send an update by COB Thursday US time.
I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 30, 2016 at 6:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.
I dug into this a bit and found more problems. I wondered why Tom's
patch did this:
! if (has_parallel_hazard((Node *) rte->subquery, false))
! return;
! break;
Instead of just doing this:
- return;
+ break;After all, the code that built subquery paths ought to be sufficient
to find any parallel hazards during subquery planning; there seems to
be no especially-good reason why we should walk the whole query tree
searching for the again. So I changed that and ran the regression
tests. With force_parallel_mode=on, things got unhappy on exactly one
query:
SELECT * FROM
(SELECT a || b AS ab FROM t1
UNION ALL
SELECT ab FROM t2) t
ORDER BY 1 LIMIT 8;
This failed with a complaint about parallel workers trying to touch
temporary relations, which turns out to be pretty valid since t1 and
t2 are BOTH temporary relations. This turns out to be another facet
of my ignorance about how subqueries can be pulled up to create
appendrels with crazy things in them. set_rel_consider_parallel()
looks at the appendrel and thinks everything is fine, because the
reference to temporary tables are buried inside the appendrel members,
which are note examined.
I think it's hard to avoid the conclusion that
set_rel_consider_parallel() needs to run on other member rels as well
as baserels. We might be able to do that only for cases where the
parent is a subquery RTE, since if the parent is a baserel then I
think we must have just a standard inheritance hierarchy and things
might be OK, but even then, I fear there might be problems with RLS.
Anyway, the attached patch solves the problem in a fairly "brute
force" fashion. We loop over all basrels and other member rels and
set consider_parallel according to their properties. Then, when
setting base rel sizes, we clear consider_parallel for any parents if
it's clear for any of the children. Finally, before setting base rel
pathlists for appendrel children, we clear the flag for the child if
it's meanwhile been cleared for the parent. Thus, the parents and
children always agree and only consider parallel paths for any of the
rels if they're all OK. This seems a bit grotty to me so suggestions
are welcome.
(Official status update: I'm not prepared to commit this without some
review. So I intend to wait for a while and see whether I get some
review. I realize these status updates are supposed to contain a date
by which further action will be taken, but I don't know how to
meaningfully do that in this type of case.)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
more-allpaths-fixes-v1.patchinvalid/octet-stream; name=more-allpaths-fixes-v1.patchDownload
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 2e4b670..57fd06e 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -242,6 +242,37 @@ set_base_rel_sizes(PlannerInfo *root)
{
Index rti;
+ /*
+ * If parallel query is a possibility, tentatively set consider_parallel
+ * for those base and "other member" relations for which it seems
+ * plausible. We must do this before set_rel_size() is called for
+ * inheritance relations, because part of it's job is to clear the
+ * consider_parallel flag if any child is not so marked.
+ */
+ if (root->glob->parallelModeOK)
+ {
+ for (rti = 1; rti < root->simple_rel_array_size; rti++)
+ {
+ RelOptInfo *rel = root->simple_rel_array[rti];
+ RangeTblEntry *rte;
+
+ /* there may be empty slots corresponding to non-baserel RTEs */
+ if (rel == NULL)
+ continue;
+ if (rel->reloptkind != RELOPT_BASEREL &&
+ rel->reloptkind != RELOPT_OTHER_MEMBER_REL)
+ continue;
+ Assert(rel->relid == rti); /* sanity check on array */
+
+ rte = root->simple_rte_array[rti];
+ set_rel_consider_parallel(root, rel, rte);
+ }
+ }
+
+ /*
+ * Now set size estimates, possibly clearing consider_parallel for some
+ * parent rels as a side effect.
+ */
for (rti = 1; rti < root->simple_rel_array_size; rti++)
{
RelOptInfo *rel = root->simple_rel_array[rti];
@@ -259,16 +290,6 @@ set_base_rel_sizes(PlannerInfo *root)
rte = root->simple_rte_array[rti];
- /*
- * If parallelism is allowable for this query in general, see whether
- * it's allowable for this rel in particular. We have to do this
- * before set_rel_size, because that if this is an inheritance parent,
- * set_append_rel_size will pass the consider_parallel flag down to
- * inheritance children.
- */
- if (root->glob->parallelModeOK)
- set_rel_consider_parallel(root, rel, rte);
-
set_rel_size(root, rel, rti, rte);
}
}
@@ -504,8 +525,9 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
/* Don't call this if parallelism is disallowed for the entire query. */
Assert(root->glob->parallelModeOK);
- /* Don't call this for non-baserels. */
- Assert(rel->reloptkind == RELOPT_BASEREL);
+ /* This should only be called for base or "other member" relations. */
+ Assert(rel->reloptkind == RELOPT_BASEREL ||
+ rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
/* Assorted checks based on rtekind. */
switch (rte->rtekind)
@@ -562,15 +584,12 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
case RTE_SUBQUERY:
/*
- * Subplans currently aren't passed to workers. Even if they
- * were, the subplan might be using parallelism internally, and we
- * can't support nested Gather nodes at present. Finally, we
- * don't have a good way of knowing whether the subplan involves
- * any parallel-restricted operations. It would be nice to relax
- * this restriction some day, but it's going to take a fair amount
- * of work.
+ * There's no intrinsic problem with scanning a subquery from a
+ * parallel worker. If the subquery doesn't happen to have any
+ * parallel-safe paths, then flagging it as consider_parallel
+ * won't help, but that's true for plain tables, too.
*/
- return;
+ break;
case RTE_JOIN:
/* Shouldn't happen; we're only considering baserels here. */
@@ -966,8 +985,9 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
continue;
}
- /* Copy consider_parallel flag from parent. */
- childrel->consider_parallel = rel->consider_parallel;
+ /* If any childrel is not parallel-safe, neither is parent. */
+ if (!childrel->consider_parallel)
+ rel->consider_parallel = false;
/*
* CE failed, so finish copying/modifying targetlist and join quals.
@@ -1141,6 +1161,17 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
childrel = root->simple_rel_array[childRTindex];
/*
+ * If the parent isn't eligible for parallelism, there's no point
+ * in considering it for the children. (This might change someday
+ * if we have a way to build an Append plan where some of the child
+ * plans are forced to run in the parent and others can run in any
+ * process, but for now there's no point in expending cycles building
+ * childrel paths we can't use.)
+ */
+ if (!rel->consider_parallel)
+ childrel->consider_parallel = false;
+
+ /*
* Compute the child's access paths.
*/
set_rel_pathlist(root, childrel, childRTindex, childRTE);
On Sat, Jul 2, 2016 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 30, 2016 at 6:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.
I dug into this a bit and found more problems. I wondered why Tom's
patch did this:! if (has_parallel_hazard((Node *) rte->subquery, false))
! return;
! break;Instead of just doing this:
- return; + break;After all, the code that built subquery paths ought to be sufficient
to find any parallel hazards during subquery planning; there seems to
be no especially-good reason why we should walk the whole query tree
searching for the again. So I changed that and ran the regression
tests. With force_parallel_mode=on, things got unhappy on exactly one
query:SELECT * FROM
(SELECT a || b AS ab FROM t1
UNION ALL
SELECT ab FROM t2) t
ORDER BY 1 LIMIT 8;This failed with a complaint about parallel workers trying to touch
temporary relations, which turns out to be pretty valid since t1 and
t2 are BOTH temporary relations. This turns out to be another facet
of my ignorance about how subqueries can be pulled up to create
appendrels with crazy things in them. set_rel_consider_parallel()
looks at the appendrel and thinks everything is fine, because the
reference to temporary tables are buried inside the appendrel members,
which are note examined.I think it's hard to avoid the conclusion that
set_rel_consider_parallel() needs to run on other member rels as well
as baserels. We might be able to do that only for cases where the
parent is a subquery RTE, since if the parent is a baserel then I
think we must have just a standard inheritance hierarchy and things
might be OK, but even then, I fear there might be problems with RLS.
Anyway, the attached patch solves the problem in a fairly "brute
force" fashion. We loop over all basrels and other member rels and
set consider_parallel according to their properties. Then, when
setting base rel sizes, we clear consider_parallel for any parents if
it's clear for any of the children. Finally, before setting base rel
pathlists for appendrel children, we clear the flag for the child if
it's meanwhile been cleared for the parent. Thus, the parents and
children always agree and only consider parallel paths for any of the
rels if they're all OK. This seems a bit grotty to me so suggestions
are welcome.
@@ -966,8 +985,9 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
continue;
}
- /* Copy consider_parallel flag from parent. */
- childrel->consider_parallel = rel->consider_parallel;
+ /* If any childrel is not parallel-safe, neither is parent. */
+ if (!childrel->consider_parallel)
+ rel->consider_parallel = false;Doing this way misses the point that adjust_appendrel_attrs() can
change the targetlist. We should consider it for parallel-safety after
it gets modified. I think that point can be addressed if try to set
consider_parallel for child rels as I have done in patch [1]/messages/by-id/CAA4eK1Jg_GvaTEjJSaV5vZY6acDmi-B3iXWvdiXa+pUFbnkyTg@mail.gmail.com.
/*
+ * If the parent isn't eligible for parallelism, there's no point
+ * in considering it for the children. (This might change someday
+ * if we have a way to build an Append plan where some of the child
+ * plans are forced to run in the parent and others can run in any
+ * process, but for now there's no point in expending cycles building
+ * childrel paths we can't use.)
+ */+ if (!rel->consider_parallel)
+ childrel->consider_parallel = false;
+What exactly makes Append plan to not able to run some of the child
nodes is other process?
[1]: /messages/by-id/CAA4eK1Jg_GvaTEjJSaV5vZY6acDmi-B3iXWvdiXa+pUFbnkyTg@mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
I dug into this a bit and found more problems. I wondered why Tom's
patch did this:
! if (has_parallel_hazard((Node *) rte->subquery, false))
! return;
! break;
Instead of just doing this:
- return; + break;
After all, the code that built subquery paths ought to be sufficient
to find any parallel hazards during subquery planning; there seems to
be no especially-good reason why we should walk the whole query tree
searching for the again.
Yeah, I think you are right. I modeled my patch on the nearby handling of
function RTEs, wherein function_rte_parallel_ok checks the RTE contents
for safety. But we must do that because create_functionscan_path relies
entirely on the rel's consider_parallel flag. In contrast,
create_subqueryscan_path looks at both rel->consider_parallel and
subpath->parallel_safe, so we can rely on the subpath(s)' markings as an
indication of the subquery's safety. (rel->consider_parallel then tells
us just about the restriction and projection expressions on the subquery
rel itself, and we'll end up with the right choice if those are unsafe.)
BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
"return;" in the tablesample stanza, allpaths.c:541 as of HEAD? Looks to
me like we're failing to ever treat tablesampling as parallel-safe.
I'm rather dubious about whether any of our tablesample methods actually
are parallel-safe, but that should be down to the method to say. If this
code's been like this all along, we've never tested them.
I think it's hard to avoid the conclusion that
set_rel_consider_parallel() needs to run on other member rels as well
as baserels. We might be able to do that only for cases where the
parent is a subquery RTE, since if the parent is a baserel then I
think we must have just a standard inheritance hierarchy and things
might be OK, but even then, I fear there might be problems with RLS.
Anyway, the attached patch solves the problem in a fairly "brute
force" fashion.
I agree, and I also agree that this way is pretty brute-force.
I think a cleaner way is to have set_append_rel_size() invoke
set_rel_consider_parallel() on the child rels and then propagate their
parallel-unsafety up to the parent. That seems fairly analogous to
the way we already deal with their sizes: in particular, set_rel_size
is invoked on an appendrel child from there, not from an extra pass in
set_base_rel_sizes. Then set_append_rel_pathlist can propagate unsafety
down again, as you've done here.
(Official status update: I'm not prepared to commit this without some
review. So I intend to wait for a while and see whether I get some
review. I realize these status updates are supposed to contain a date
by which further action will be taken, but I don't know how to
meaningfully do that in this type of case.)
You mentioned that you'll be on vacation for much of July. If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
"return;" in the tablesample stanza, allpaths.c:541 as of HEAD? Looks to
me like we're failing to ever treat tablesampling as parallel-safe.
I'm rather dubious about whether any of our tablesample methods actually
are parallel-safe, but that should be down to the method to say. If this
code's been like this all along, we've never tested them.
You are correct.
I think it's hard to avoid the conclusion that
set_rel_consider_parallel() needs to run on other member rels as well
as baserels. We might be able to do that only for cases where the
parent is a subquery RTE, since if the parent is a baserel then I
think we must have just a standard inheritance hierarchy and things
might be OK, but even then, I fear there might be problems with RLS.
Anyway, the attached patch solves the problem in a fairly "brute
force" fashion.I agree, and I also agree that this way is pretty brute-force.
I think a cleaner way is to have set_append_rel_size() invoke
set_rel_consider_parallel() on the child rels and then propagate their
parallel-unsafety up to the parent. That seems fairly analogous to
the way we already deal with their sizes: in particular, set_rel_size
is invoked on an appendrel child from there, not from an extra pass in
set_base_rel_sizes. Then set_append_rel_pathlist can propagate unsafety
down again, as you've done here.
I was reluctant to do it that way because of the relatively stupid way
that we have to go about finding the inheritance children to visit,
but maybe I shouldn't let that dominate my thinking. We could always
replace that with some more efficient data structure at some point
down the road.
(Official status update: I'm not prepared to commit this without some
review. So I intend to wait for a while and see whether I get some
review. I realize these status updates are supposed to contain a date
by which further action will be taken, but I don't know how to
meaningfully do that in this type of case.)You mentioned that you'll be on vacation for much of July. If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.
That would be much appreciated.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think a cleaner way is to have set_append_rel_size() invoke
set_rel_consider_parallel() on the child rels and then propagate their
parallel-unsafety up to the parent. That seems fairly analogous to
the way we already deal with their sizes: in particular, set_rel_size
is invoked on an appendrel child from there, not from an extra pass in
set_base_rel_sizes. Then set_append_rel_pathlist can propagate unsafety
down again, as you've done here.
I was reluctant to do it that way because of the relatively stupid way
that we have to go about finding the inheritance children to visit,
but maybe I shouldn't let that dominate my thinking. We could always
replace that with some more efficient data structure at some point
down the road.
That might be a reasonable objection if we needed to add a third such
loop, but I'm talking about putting the set_rel_consider_parallel call in
the loop that already exists. So there shouldn't be any added overhead.
(It's definitely true that we could improve on the append_rel_list-based
search mechanism; but so far I have never seen any indication that that
was a bottleneck, so it's pretty far down my to-do list.)
You mentioned that you'll be on vacation for much of July. If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.
That would be much appreciated.
OK, will do.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
You mentioned that you'll be on vacation for much of July. If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.
That would be much appreciated.
OK, will do.
I've reviewed, adjusted, and pushed this patch, and accordingly am
marking the open item closed. (Any remaining bugs should become
new open items.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jul 3, 2016 at 6:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
You mentioned that you'll be on vacation for much of July. If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.That would be much appreciated.
OK, will do.
I've reviewed, adjusted, and pushed this patch, and accordingly am
marking the open item closed. (Any remaining bugs should become
new open items.)
Thanks. Looks good to me.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers