Thread: Propose RC1 for Friday ...
It seems to me that about the only major issue right now is testing the various platforms ... would anyone disagree with putting out an RC1 on Friday whose primary purpose is platform testing? I don't believe there is anything outstanding right now that would require us to do a beta6, and RC1 might give enough stability to ppl to get them testing for the remaining platforms ... ?
"Marc G. Fournier" <scrappy@hub.org> writes:
> It seems to me that about the only major issue right now is testing the
> various platforms ... would anyone disagree with putting out an RC1 on
> Friday whose primary purpose is platform testing?
Works for me. We should be able to resolve this awk issue by then,
and hopefully have confirmation on that GB18030 change too.
regards, tom lane
Tom Lane wrote: > "Marc G. Fournier" <scrappy@hub.org> writes: > > It seems to me that about the only major issue right now is testing the > > various platforms ... would anyone disagree with putting out an RC1 on > > Friday whose primary purpose is platform testing? > > Works for me. We should be able to resolve this awk issue by then, > and hopefully have confirmation on that GB18030 change too. Sounds good to me! -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Wed, Nov 13, 2002 at 11:43:14PM -0500, Bruce Momjian wrote: > Tom Lane wrote: > > "Marc G. Fournier" <scrappy@hub.org> writes: > > > It seems to me that about the only major issue right now is testing the > > > various platforms ... would anyone disagree with putting out an RC1 on > > > Friday whose primary purpose is platform testing? > > > > Works for me. We should be able to resolve this awk issue by then, > > and hopefully have confirmation on that GB18030 change too. Sorry to be a pest, but I'd like to re-raise the issue I brought up regarding a performance regression from 7.2.3, when subqueries are pulled up and merged with their parent. What happened is that the default order that WHERE clauses get merged changed. (The original discussion and patch was over on GENERAL, and doesn't seem to be in the FTS archives?): http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&oe=UTF-8&threadm=3D0885E1.8F369ACA%40mascari.com&rnum=3&prev=/groups%3Fq%3DMike%2BMascari%2Bsecurity%2BTom%2BLane%26ie%3DUTF-8%26oe%3DUTF-8%26hl%3Den The reason for doing this was a theoretical hole in VIEW-based data access restrictions. The consequence is that a class of queries got an order of magnitude slower (my particular example goes from 160 ms to 2000 ms). Tom was not excited about making the original change (we don't guarantee the order of WHERE clauses, which is what would be required for this to be a real fix), and is resisting changing it back, partly because neither order is the right thing. My argument is that we can't do the right thing right now, anyway (feature freeze), so let's put it back the way it was in the last stable release, so as not to break (o.k., dramatically slow down) existing queries. (patch attached) Any other opinions? Ross
Attachment
"Ross J. Reedstrom" <reedstrm@rice.edu> writes:
> Sorry to be a pest, but I'd like to re-raise the issue I brought up
> regarding a performance regression from 7.2.3, when subqueries are pulled
> up and merged with their parent.
> ...
> Tom was not excited about making the original change (we don't guarantee
> the order of WHERE clauses, which is what would be required for this to
> be a real fix), and is resisting changing it back, partly because neither
> order is the right thing. My argument is that we can't do the right thing
> right now, anyway (feature freeze), so let's put it back the way it was in
> the last stable release, so as not to break (o.k., dramatically slow down)
> existing queries.
Well, we could define it as a bug ;-) --- that is, a performance regression.
I'd be happier about adding a dozen lines of code to sort quals by
whether or not they contain a subplan than about flip-flopping on the
original patch. That would actually solve the class of problem you
exhibited, whereas the other is just a band-aid that happens to work for
your particular example.
regards, tom lane
I said:
> Well, we could define it as a bug ;-) --- that is, a performance regression.
> I'd be happier about adding a dozen lines of code to sort quals by
> whether or not they contain a subplan than about flip-flopping on the
> original patch. That would actually solve the class of problem you
> exhibited, whereas the other is just a band-aid that happens to work for
> your particular example.
The attached patch does the above. I think it's a very low-risk change,
but am tossing it out on the list to see if anyone objects to applying
it in the 7.3 branch. (I intend to put it in 7.4devel in any case.)
regards, tom lane
*** src/backend/optimizer/plan/createplan.c.orig Wed Nov 6 17:31:24 2002
--- src/backend/optimizer/plan/createplan.c Thu Nov 14 13:18:04 2002
***************
*** 70,75 ****
--- 70,76 ---- IndexOptInfo *index, Oid *opclass); static List
*switch_outer(List*clauses);
+ static List *order_qual_clauses(Query *root, List *clauses); static void copy_path_costsize(Plan *dest, Path *src);
staticvoid copy_plan_costsize(Plan *dest, Plan *src); static SeqScan *make_seqscan(List *qptlist, List *qpqual, Index
scanrelid);
***************
*** 182,187 ****
--- 183,191 ---- */ scan_clauses = get_actual_clauses(best_path->parent->baserestrictinfo);
+ /* Sort clauses into best execution order */
+ scan_clauses = order_qual_clauses(root, scan_clauses);
+ switch (best_path->pathtype) { case T_SeqScan:
***************
*** 359,364 ****
--- 363,369 ---- { Result *plan; List *tlist;
+ List *constclauses; Plan *subplan; if (best_path->path.parent)
***************
*** 371,377 **** else subplan = NULL;
! plan = make_result(tlist, (Node *) best_path->constantqual, subplan); return plan; }
--- 376,384 ---- else subplan = NULL;
! constclauses = order_qual_clauses(root, best_path->constantqual);
!
! plan = make_result(tlist, (Node *) constclauses, subplan); return plan; }
***************
*** 1210,1215 ****
--- 1217,1259 ---- t_list = lappend(t_list, clause); } return t_list;
+ }
+
+ /*
+ * order_qual_clauses
+ * Given a list of qual clauses that will all be evaluated at the same
+ * plan node, sort the list into the order we want to check the quals
+ * in at runtime.
+ *
+ * Ideally the order should be driven by a combination of execution cost and
+ * selectivity, but unfortunately we have so little information about
+ * execution cost of operators that it's really hard to do anything smart.
+ * For now, we just move any quals that contain SubPlan references (but not
+ * InitPlan references) to the end of the list.
+ */
+ static List *
+ order_qual_clauses(Query *root, List *clauses)
+ {
+ List *nosubplans;
+ List *withsubplans;
+ List *l;
+
+ /* No need to work hard if the query is subselect-free */
+ if (!root->hasSubLinks)
+ return clauses;
+
+ nosubplans = withsubplans = NIL;
+ foreach(l, clauses)
+ {
+ Node *clause = lfirst(l);
+
+ if (contain_subplans(clause))
+ withsubplans = lappend(withsubplans, clause);
+ else
+ nosubplans = lappend(nosubplans, clause);
+ }
+
+ return nconc(nosubplans, withsubplans); } /*
"Ross J. Reedstrom" <reedstrm@rice.edu> writes:
> I've tested this under 7.3, and it works beautifully for the cases I've
> built over the last 2 days. I can no longer bugger a plan up mearly
> by reordering the WHERE clauses. Note that 2 of the five parts won't
> patch in (involving constantqual). Looks to be code refactoring between
> here and planmain.c on the 7.4 branch?
Yeah. I'm not going to bother covering the resconstantqual case in the
7.3 version; that's a little-used path anyway.
regards, tom lane
I'd ask for a quick beta6 ... even knowing everyone would hate me :)
On Thu, 14 Nov 2002, Tom Lane wrote:
> I said:
> > Well, we could define it as a bug ;-) --- that is, a performance regression.
> > I'd be happier about adding a dozen lines of code to sort quals by
> > whether or not they contain a subplan than about flip-flopping on the
> > original patch. That would actually solve the class of problem you
> > exhibited, whereas the other is just a band-aid that happens to work for
> > your particular example.
>
> The attached patch does the above. I think it's a very low-risk change,
> but am tossing it out on the list to see if anyone objects to applying
> it in the 7.3 branch. (I intend to put it in 7.4devel in any case.)
>
> regards, tom lane
>
>
> *** src/backend/optimizer/plan/createplan.c.orig Wed Nov 6 17:31:24 2002
> --- src/backend/optimizer/plan/createplan.c Thu Nov 14 13:18:04 2002
> ***************
> *** 70,75 ****
> --- 70,76 ----
> IndexOptInfo *index,
> Oid *opclass);
> static List *switch_outer(List *clauses);
> + static List *order_qual_clauses(Query *root, List *clauses);
> static void copy_path_costsize(Plan *dest, Path *src);
> static void copy_plan_costsize(Plan *dest, Plan *src);
> static SeqScan *make_seqscan(List *qptlist, List *qpqual, Index scanrelid);
> ***************
> *** 182,187 ****
> --- 183,191 ----
> */
> scan_clauses = get_actual_clauses(best_path->parent->baserestrictinfo);
>
> + /* Sort clauses into best execution order */
> + scan_clauses = order_qual_clauses(root, scan_clauses);
> +
> switch (best_path->pathtype)
> {
> case T_SeqScan:
> ***************
> *** 359,364 ****
> --- 363,369 ----
> {
> Result *plan;
> List *tlist;
> + List *constclauses;
> Plan *subplan;
>
> if (best_path->path.parent)
> ***************
> *** 371,377 ****
> else
> subplan = NULL;
>
> ! plan = make_result(tlist, (Node *) best_path->constantqual, subplan);
>
> return plan;
> }
> --- 376,384 ----
> else
> subplan = NULL;
>
> ! constclauses = order_qual_clauses(root, best_path->constantqual);
> !
> ! plan = make_result(tlist, (Node *) constclauses, subplan);
>
> return plan;
> }
> ***************
> *** 1210,1215 ****
> --- 1217,1259 ----
> t_list = lappend(t_list, clause);
> }
> return t_list;
> + }
> +
> + /*
> + * order_qual_clauses
> + * Given a list of qual clauses that will all be evaluated at the same
> + * plan node, sort the list into the order we want to check the quals
> + * in at runtime.
> + *
> + * Ideally the order should be driven by a combination of execution cost and
> + * selectivity, but unfortunately we have so little information about
> + * execution cost of operators that it's really hard to do anything smart.
> + * For now, we just move any quals that contain SubPlan references (but not
> + * InitPlan references) to the end of the list.
> + */
> + static List *
> + order_qual_clauses(Query *root, List *clauses)
> + {
> + List *nosubplans;
> + List *withsubplans;
> + List *l;
> +
> + /* No need to work hard if the query is subselect-free */
> + if (!root->hasSubLinks)
> + return clauses;
> +
> + nosubplans = withsubplans = NIL;
> + foreach(l, clauses)
> + {
> + Node *clause = lfirst(l);
> +
> + if (contain_subplans(clause))
> + withsubplans = lappend(withsubplans, clause);
> + else
> + nosubplans = lappend(nosubplans, clause);
> + }
> +
> + return nconc(nosubplans, withsubplans);
> }
>
> /*
>
If we are going to go for a beta6, I vote we reverse out the patch. Of
course, I prefer neither.
Do we have to do a delay/feature analysis on this?
Marc, there will always be 7.3.1 to fix any problems. They will surely
happen so I think it is safe to push forward for tomorrow's RC1. Of
course, if you disagree, let's back it out.
---------------------------------------------------------------------------
Marc G. Fournier wrote:
>
> I'd ask for a quick beta6 ... even knowing everyone would hate me :)
>
>
>
> On Thu, 14 Nov 2002, Tom Lane wrote:
>
> > I said:
> > > Well, we could define it as a bug ;-) --- that is, a performance regression.
> > > I'd be happier about adding a dozen lines of code to sort quals by
> > > whether or not they contain a subplan than about flip-flopping on the
> > > original patch. That would actually solve the class of problem you
> > > exhibited, whereas the other is just a band-aid that happens to work for
> > > your particular example.
> >
> > The attached patch does the above. I think it's a very low-risk change,
> > but am tossing it out on the list to see if anyone objects to applying
> > it in the 7.3 branch. (I intend to put it in 7.4devel in any case.)
> >
> > regards, tom lane
> >
> >
> > *** src/backend/optimizer/plan/createplan.c.orig Wed Nov 6 17:31:24 2002
> > --- src/backend/optimizer/plan/createplan.c Thu Nov 14 13:18:04 2002
> > ***************
> > *** 70,75 ****
> > --- 70,76 ----
> > IndexOptInfo *index,
> > Oid *opclass);
> > static List *switch_outer(List *clauses);
> > + static List *order_qual_clauses(Query *root, List *clauses);
> > static void copy_path_costsize(Plan *dest, Path *src);
> > static void copy_plan_costsize(Plan *dest, Plan *src);
> > static SeqScan *make_seqscan(List *qptlist, List *qpqual, Index scanrelid);
> > ***************
> > *** 182,187 ****
> > --- 183,191 ----
> > */
> > scan_clauses = get_actual_clauses(best_path->parent->baserestrictinfo);
> >
> > + /* Sort clauses into best execution order */
> > + scan_clauses = order_qual_clauses(root, scan_clauses);
> > +
> > switch (best_path->pathtype)
> > {
> > case T_SeqScan:
> > ***************
> > *** 359,364 ****
> > --- 363,369 ----
> > {
> > Result *plan;
> > List *tlist;
> > + List *constclauses;
> > Plan *subplan;
> >
> > if (best_path->path.parent)
> > ***************
> > *** 371,377 ****
> > else
> > subplan = NULL;
> >
> > ! plan = make_result(tlist, (Node *) best_path->constantqual, subplan);
> >
> > return plan;
> > }
> > --- 376,384 ----
> > else
> > subplan = NULL;
> >
> > ! constclauses = order_qual_clauses(root, best_path->constantqual);
> > !
> > ! plan = make_result(tlist, (Node *) constclauses, subplan);
> >
> > return plan;
> > }
> > ***************
> > *** 1210,1215 ****
> > --- 1217,1259 ----
> > t_list = lappend(t_list, clause);
> > }
> > return t_list;
> > + }
> > +
> > + /*
> > + * order_qual_clauses
> > + * Given a list of qual clauses that will all be evaluated at the same
> > + * plan node, sort the list into the order we want to check the quals
> > + * in at runtime.
> > + *
> > + * Ideally the order should be driven by a combination of execution cost and
> > + * selectivity, but unfortunately we have so little information about
> > + * execution cost of operators that it's really hard to do anything smart.
> > + * For now, we just move any quals that contain SubPlan references (but not
> > + * InitPlan references) to the end of the list.
> > + */
> > + static List *
> > + order_qual_clauses(Query *root, List *clauses)
> > + {
> > + List *nosubplans;
> > + List *withsubplans;
> > + List *l;
> > +
> > + /* No need to work hard if the query is subselect-free */
> > + if (!root->hasSubLinks)
> > + return clauses;
> > +
> > + nosubplans = withsubplans = NIL;
> > + foreach(l, clauses)
> > + {
> > + Node *clause = lfirst(l);
> > +
> > + if (contain_subplans(clause))
> > + withsubplans = lappend(withsubplans, clause);
> > + else
> > + nosubplans = lappend(nosubplans, clause);
> > + }
> > +
> > + return nconc(nosubplans, withsubplans);
> > }
> >
> > /*
> >
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
>
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073
"Marc G. Fournier" <scrappy@hub.org> writes:
> I'd ask for a quick beta6 ... even knowing everyone would hate me :)
What's wrong with calling it "RC1"?
I think pushing out an RC tarball is the only way we'll shake loose any
more port reports. Putting out "beta6" isn't going to attract attention
from anyone who ignored the first five. So I think delaying RC1 is just
going to delay the release, without actually gaining anything.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> If we are going to go for a beta6, I vote we reverse out the patch.
It's not applied yet.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> If we are going to go for a beta6, I vote we reverse out the patch. Of
> course, I prefer neither.
I read this several times and am still not quite sure which path you are
voting for. We can:
1. not apply the patch to fix Ross' problem, and ship RC1 tomorrow;
2. apply the patch, and ship RC1 tomorrow;
3. apply the patch and delay RC1.
Which do you like?
Personally I think this is a low-risk patch and so choice 2 is
appropriate.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > If we are going to go for a beta6, I vote we reverse out the patch. Of > > course, I prefer neither. > > I read this several times and am still not quite sure which path you are > voting for. We can: > > 1. not apply the patch to fix Ross' problem, and ship RC1 tomorrow; > 2. apply the patch, and ship RC1 tomorrow; > 3. apply the patch and delay RC1. > > Which do you like? > > Personally I think this is a low-risk patch and so choice 2 is > appropriate. Sorry, I was vague. I think we should apply and go to RC1 tomorrow. There will always be tweaks and fixes. If we expect it to be perfect, we will never make a final release. We are 2.5 months into beta, and if we don't want +3 months beta, we should get going. We have to start taking some _reasonable_ risks to move this forward. Until we make a final, we will not increase our test pool, and at this point we are all sort of staring at each other. Let's go! -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian wrote: > > Tom Lane wrote: <snip> > > Personally I think this is a low-risk patch and so choice 2 is > > appropriate. If this is the only change, then 2 does seem like the best mix of risk/progress. :-) Regards and best wishes, Justin Clift > Sorry, I was vague. I think we should apply and go to RC1 tomorrow. > There will always be tweaks and fixes. If we expect it to be perfect, > we will never make a final release. We are 2.5 months into beta, and > if we don't want +3 months beta, we should get going. > > We have to start taking some _reasonable_ risks to move this forward. > Until we make a final, we will not increase our test pool, and at this > point we are all sort of staring at each other. Let's go! <snip> -- "My grandfather once told me that there are two kinds of people: those who work and those who take the credit. He told me to try to be in the first group; there was less competition there." - Indira Gandhi
Tom Lane <tgl@sss.pgh.pa.us> writes: > 2. apply the patch, and ship RC1 tomorrow; I think that's the best bet. (That said, the philosophy of "there's always 7.3.1" that Bruce alluded to is not one that I agree with.) Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
> Sorry, I was vague. I think we should apply and go to RC1 tomorrow. > There will always be tweaks and fixes. If we expect it to be perfect, > we will never make a final release. We are 2.5 months into beta, and > if we don't want +3 months beta, we should get going. > > We have to start taking some _reasonable_ risks to move this forward. > Until we make a final, we will not increase our test pool, and at this > point we are all sort of staring at each other. Let's go! Yeah - RC1 guys - I'm really hanging out for 7.3... Chris
Christopher Kings-Lynne wrote: > > Sorry, I was vague. I think we should apply and go to RC1 tomorrow. > > There will always be tweaks and fixes. If we expect it to be perfect, > > we will never make a final release. We are 2.5 months into beta, and > > if we don't want +3 months beta, we should get going. > > > > We have to start taking some _reasonable_ risks to move this forward. > > Until we make a final, we will not increase our test pool, and at this > > point we are all sort of staring at each other. Let's go! > > Yeah - RC1 guys - I'm really hanging out for 7.3... Yes, that was my lame attempt to get some excitement for RC1. :-) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Thu, 14 Nov 2002, Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > If we are going to go for a beta6, I vote we reverse out the patch. Of > > course, I prefer neither. > > I read this several times and am still not quite sure which path you are > voting for. We can: > > 1. not apply the patch to fix Ross' problem, and ship RC1 tomorrow; > 2. apply the patch, and ship RC1 tomorrow; > 3. apply the patch and delay RC1. > > Which do you like? > > Personally I think this is a low-risk patch and so choice 2 is > appropriate. Go for it ... I'd like to get RC1 out this week, and if you feel its low-risk, the worst case, we have to do an RC2 ...
I've tested this under 7.3, and it works beautifully for the cases I've built over the last 2 days. I can no longer bugger a plan up mearly by reordering the WHERE clauses. Note that 2 of the five parts won't patch in (involving constantqual). Looks to be code refactoring between here and planmain.c on the 7.4 branch? I tried to hand-patch it in, and gave up. it _seems_ to work without it, but I probably haven't covered that codepath. Ross On Thu, Nov 14, 2002 at 01:33:05PM -0500, Tom Lane wrote: > I said: > > Well, we could define it as a bug ;-) --- that is, a performance regression. > > I'd be happier about adding a dozen lines of code to sort quals by > > whether or not they contain a subplan than about flip-flopping on the > > original patch. That would actually solve the class of problem you > > exhibited, whereas the other is just a band-aid that happens to work for > > your particular example. > > The attached patch does the above. I think it's a very low-risk change, > but am tossing it out on the list to see if anyone objects to applying > it in the 7.3 branch. (I intend to put it in 7.4devel in any case.) > > regards, tom lane