From: Koichi S. <koi...@gm...> - 2014-02-13 05:31:04
|
Hi Ashutosh; I'm afraid this patch is still pending for three weeks. Could you respond if this looks nice and good to go? Regards; --- Koichi Suzuki 2014-01-16 17:00 GMT+09:00 ZhangJulian <jul...@ou...>: > Hi Ashutosh, > > How about fixing it as the attached patch? > > Thanks > Julian > ________________________________ > Date: Wed, 15 Jan 2014 08:49:33 +0530 > > Subject: Re: [Postgres-xc-bugs] Query are shipped to the data nodes more > than needed. > From: ash...@en... > To: jul...@ou... > CC: pos...@li... > > As you have spotted already changing make_ands_implicit() is not desirable, > since it has not got modified in many revisions of PostgreSQL and has many > callers which comply with the current behaviour. > > The queries you mentioned are correctly inferring the datanode where to ship > the query. I think you will have to worry about queries like SELECT c1, c2, > c3, c4 FROM public.t1 WHERE ((((c1 = 1) AND (c2 = 2)) AND (c3 = 3)) AND (c4 > = 4)), which might not be shipped to only required datanodes. > > I do not have the right solutions at hand, but the existing ones are not > enough. You might want to continue experimenting more. > > > On Tue, Jan 14, 2014 at 12:31 PM, ZhangJulian <jul...@ou...> > wrote: > > Hi Ashutosh, > > I tested two more scenerios with the fix: > postgres=# create table t1 (c1 int, c2 int, c3 int, c4 int); > CREATE TABLE > postgres=# explain verbose select * from t1 where c1=1 and c2=2 and c3=3 and > c4=4; > QUERY PLAN > ------------------------------------------------------------------------------------------------------------------ > Data Node Scan on "__REMOTE_FQS_QUERY__" (cost=0.00..0.00 rows=0 width=0) > Output: t1.c1, t1.c2, t1.c3, t1.c4 > Node/s: datanode1 > Remote query: SELECT c1, c2, c3, c4 FROM public.t1 WHERE ((((c1 = 1) AND > (c2 = 2)) AND (c3 = 3)) AND (c4 = 4)) > (4 rows) > postgres=# explain verbose select * from t1 where c1=1 and c2=2 and (c3=3 or > c4=4); > QUERY PLAN > ----------------------------------------------------------------------------------------------------------------- > Data Node Scan on "__REMOTE_FQS_QUERY__" (cost=0.00..0.00 rows=0 width=0) > Output: t1.c1, t1.c2, t1.c3, t1.c4 > Node/s: datanode1 > Remote query: SELECT c1, c2, c3, c4 FROM public.t1 WHERE (((c1 = 1) AND > (c2 = 2)) AND ((c3 = 3) OR (c4 = 4))) > (4 rows) > > For a complex predicate as "where c1=1 and c2=2 and c3=3 and c4=4", it will > be parsed to "WHERE ((((c1 = 1) AND (c2 = 2)) AND (c3 = 3)) AND (c4 = 4))", > that is: > BoolExpr > BoolExpr > OpExpr (c1=1) > OpExpr (c2=2) > OpExpr (c3=3) > OpExpr (c4=4) > > Do you mean I should fix it in another way by updating make_ands_implicit() > to return the list with all AND legs? It should be a better way, but there > are several invocations to make_ands_implicit(), I can not understand all of > the callers and have the confidence to fix it as it. :) > > The callers to make_ands_implicit(): > make_ands_implicit(Expr *) : List * > ATAddCheckConstraint(List * *, AlteredTableInfo *, Relation, Constraint > *, bool, bool, LOCKMODE) : void > convert_EXISTS_to_ANY(PlannerInfo *, Query *, Node * *, List * *) : > Query * > cost_subplan(PlannerInfo *, SubPlan *, Plan *) : void > DefineIndex(RangeVar *, char *, Oid, Oid, char *, char *, List *, Expr > *, List *, List *, bool, bool, bool, bool, bool, bool, bool, bool, bool, > bool) : Oid > ExecRelCheck(ResultRelInfo *, TupleTableSlot *, EState *) : const char * > get_relation_constraints(PlannerInfo *, Oid, RelOptInfo *, bool) : List > * > pgxc_find_dist_equijoin_qual(List *, List *, Node *) : Expr * > pgxc_find_distcol_expr(Index, AttrNumber, Node *) : Expr * > preprocess_expression(PlannerInfo *, Node *, int) : Node * > RelationGetIndexPredicate(Relation) : List * > set_append_rel_size(PlannerInfo *, RelOptInfo *, Index, RangeTblEntry *) > : void > TriggerEnabled(EState *, ResultRelInfo *, Trigger *, TriggerEvent, > Bitmapset *, HeapTuple, HeapTuple) : bool > validateCheckConstraint(Relation, HeapTuple) : void > > > Thanks > Julian > > ________________________________ > Date: Mon, 13 Jan 2014 15:11:00 +0530 > > Subject: Re: [Postgres-xc-bugs] Query are shipped to the data nodes more > than needed. > From: ash...@en... > To: jul...@ou... > CC: pos...@li... > > Ok, good observation. > > BTW, with your fix, we are still in problem if there are four conditions > ANDED e.g. A and B and C and D. > > What should happen, and which actually happens before starting the planning > phase, is all the ANDs are flattened into a list so that the above tree > looks like > AND > | -> args A, B, C, D > > So, a better solution seems to be that we should pass the quals to > pgxc_find_dist_qual(), by converting them into and ANDed list of args. > > > On Mon, Jan 13, 2014 at 2:43 PM, ZhangJulian <jul...@ou...> > wrote: > > Hi Ashutosh, > > The parser will parse "where c1=1 and c2=1 and c3=1" to: > BoolExpr > |--BoolExpr > | |-OpExpr (c1=1) > | |-OpExpr (c2=1) > | > |--OpExpr (c3=1) > > make_ands_implicit() will translate it to a list containing two elements: > 1. BoolExpr > |-OpExpr (c1=1) > |-OpExpr (c2=1) > > 2. OpExpr (c3=1) > But it will not further split the first element to two OpExpr and add them > to the parent list. > > Thanks > Julian > > ________________________________ > Date: Mon, 13 Jan 2014 14:19:48 +0530 > > Subject: Re: [Postgres-xc-bugs] Query are shipped to the data nodes more > than needed. > From: ash...@en... > To: jul...@ou... > CC: pos...@li... > > Hi Julian, > Can you please send a patch after taking care of my comments in my previous > mail. I am pasting them here for your quick reference, > -- > In function pgxc_find_distcol_expr() there is code > > 828 /* Convert the qualification into List if it's not already so */ > 829 if (!IsA(quals, List)) > 830 lquals = make_ands_implicit((Expr *)quals); > 831 else > 832 lquals = (List *)quals; > > which takes care of the quals which are not in the List form. This part of > the code should have taken care of the case you mentioned. It seems that for > some reason, this part is not converting the ANDed quals into List. Can you > please check why? May be you want to step through make_ands_implicit() > (using gdb or some debugger) to see what's happening there. > > > > > On Mon, Jan 13, 2014 at 2:16 PM, ZhangJulian <jul...@ou...> > wrote: > > Hi Ashutosh, > > I am so sorry that I had sent a patch with wrong format, it is because I > edited it after Git Diff. > I regenerated a patch as the attached file and verified that it can be > applied. > > Thanks > Julian > > ________________________________ > From: jul...@ou... > To: ash...@en... > Date: Sat, 11 Jan 2014 07:52:34 +0800 > CC: pos...@li... > > Subject: Re: [Postgres-xc-bugs] Query are shipped to the data nodes more > than needed. > > Hi Ashutosh, > > I am at home and can not connect to the office to debug it today. I will > double check the problem on Monday. > > I remember when I debugged it, the proplem is, > Parser will parse "where c1=1 and c2=1 and c3=1" to "WHERE (((c1 = 1) AND > (c2 = 1)) AND (c3 = 1))", which is a list containing two elements: > element 1: ((c1 = 1) AND (c2 = 1)) which is a BoolExpr; > element 2: (c3 = 1) > For the element 1, it would not split it into two simple predidates, so the > c1 = 1 is missed. > > If I just move the c1 = 1 to the last position, the query can generate the > correct plan. > > Thanks > Julian > > ________________________________ > Date: Fri, 10 Jan 2014 16:48:47 +0530 > Subject: Re: [Postgres-xc-bugs] Query are shipped to the data nodes more > than needed. > From: ash...@en... > To: jul...@ou... > CC: pos...@li... > > Hi Julian, > I looked at the patch and tried to apply the patch, but there is an error > [ashutosh@ubuntu coderoot]git apply > /mnt/hgfs/tmp/20140110_more_datanode_involved.patch > /mnt/hgfs/tmp/20140110_more_datanode_involved.patch:10: trailing whitespace. > if (and_clause((Node *) qual_expr)) > /mnt/hgfs/tmp/20140110_more_datanode_involved.patch:11: trailing whitespace. > { > /mnt/hgfs/tmp/20140110_more_datanode_involved.patch:12: trailing whitespace. > List *sub_lquals = ((BoolExpr *) qual_expr)->args; > /mnt/hgfs/tmp/20140110_more_datanode_involved.patch:13: trailing whitespace. > distcol_expr = pgxc_find_distcol_expr(varno, attrNum, > sub_lquals); > /mnt/hgfs/tmp/20140110_more_datanode_involved.patch:14: trailing whitespace. > if (distcol_expr != NULL) > fatal: corrupt patch at line 20 > > Did you use git diff for creating the patch? > > BTW, I looked at the patch. The patch seems to be applied to function > pgxc_find_distcol_expr() (functions name usually appears along with the > diff, if you are using git diff). > > In this function there is code > > 828 /* Convert the qualification into List if it's not already so */ > 829 if (!IsA(quals, List)) > 830 lquals = make_ands_implicit((Expr *)quals); > 831 else > 832 lquals = (List *)quals; > > which takes care of the quals which are not in the List form. This part of > the code should have taken care of the case you mentioned. It seems that for > some reason, this part is not converting the ANDed quals into List. Can you > please check why? May be you want to step through make_ands_implicit() > (using gdb or some debugger) to see what's happening there. > > > > > On Fri, Jan 10, 2014 at 3:38 PM, ZhangJulian <jul...@ou...> > wrote: > > > Your name : Julian ZL Zhang > Your email address : jul...@ou... > > > System Configuration: > --------------------- > Architecture (example: Intel Pentium) : Intel Pentium > > Operating System (example: Linux 2.4.18) : 2.6.32-358.el6.x86_64 > > Postgres-XC version (example: Postgres-XC 1.1devel): Github master > Compiler used (example: gcc 3.3.5) : gcc (GCC) 4.4.7 > 20120313 (Red Hat 4.4.7-3) > > > Please enter a FULL description of your problem: > ------------------------------------------------ > I had opened the bug at http://sourceforge.net/p/postgres-xc/bugs/467/ just > descripte it more detailedly here. > Different predicate order results in different plan, the query should be > shipped only to one datanode, but in the first example, two data nodes are > involved. > > Please describe a way to repeat the problem. Please try to provide a > concise reproducible example, if at all possible: > ---------------------------------------------------------------------- > postgres=# create table t1 (c1 int, c2 int, c3 int) distribute by hash(c1); > CREATE TABLE > postgres=# explain verbose select * from t1 where c1=1 and c2=1 and c3=1; > QUERY PLAN > -------------------------------------------------------------------------------- > Data Node Scan on "REMOTE_FQS_QUERY" (cost=0.00..0.00 rows=0 width=0) > Output: t1.c1, t1.c2, t1.c3 > Node/s: datanode1, datanode2 > Remote query: SELECT c1, c2, c3 FROM benchmarksql.t1 WHERE (((c1 = 1) AND > (c2 = 1)) AND (c3 = 1)) > (4 rows) > postgres=# explain verbose select * from t1 where c2=1 and c3=1 and c1=1; > QUERY PLAN > -------------------------------------------------------------------------------- > Data Node Scan on "REMOTE_FQS_QUERY" (cost=0.00..0.00 rows=0 width=0) > Output: t1.c1, t1.c2, t1.c3 > Node/s: datanode1 > Remote query: SELECT c1, c2, c3 FROM benchmarksql.t1 WHERE (((c2 = 1) AND > (c3 = 1)) AND (c1 = 1)) > (4 rows) > > > If you know how this problem might be fixed, list the solution below: > --------------------------------------------------------------------- > I made a fix as the append file, which can resolve this issue, but I am not > sure if it is correct. > When I run MAKE CHECK, 3 test cases failed. The results are appended too. > But when I run MAKE CHECK without the fix, there are also some test cases > failed. > I do not know if the failed test cases are related to the fix. > > Thanks > Julian > > ------------------------------------------------------------------------------ > CenturyLink Cloud: The Leader in Enterprise Cloud Services. > Learn Why More Businesses Are Choosing CenturyLink Cloud For > Critical Workloads, Development Environments & Everything In Between. > Get a Quote or Start a Free Trial Today. > http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk > _______________________________________________ > Postgres-xc-bugs mailing list > Pos...@li... > https://lists.sourceforge.net/lists/listinfo/postgres-xc-bugs > > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > > ------------------------------------------------------------------------------ > CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More > Businesses Are Choosing CenturyLink Cloud For Critical Workloads, > Development Environments & Everything In Between. Get a Quote or Start a > Free Trial Today. > http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk > _______________________________________________ Postgres-xc-bugs mailing > list Pos...@li... > https://lists.sourceforge.net/lists/listinfo/postgres-xc-bugs > > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > > ------------------------------------------------------------------------------ > CenturyLink Cloud: The Leader in Enterprise Cloud Services. > Learn Why More Businesses Are Choosing CenturyLink Cloud For > Critical Workloads, Development Environments & Everything In Between. > Get a Quote or Start a Free Trial Today. > http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk > _______________________________________________ > Postgres-xc-bugs mailing list > Pos...@li... > https://lists.sourceforge.net/lists/listinfo/postgres-xc-bugs > |