Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1q0oz4-0002gw-CM for pgsql-hackers@arkaria.postgresql.org; Sun, 21 May 2023 19:44:50 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1q0oz2-0008PH-Ea for pgsql-hackers@arkaria.postgresql.org; Sun, 21 May 2023 19:44:48 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1q0oz1-0008Oz-W5 for pgsql-hackers@lists.postgresql.org; Sun, 21 May 2023 19:44:48 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1q0oyz-0014ZQ-9s for pgsql-hackers@postgresql.org; Sun, 21 May 2023 19:44:46 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 34LJihXh395265; Sun, 21 May 2023 15:44:43 -0400 From: Tom Lane To: Richard Guo cc: PostgreSQL-development Subject: Re: Assert failure of the cross-check for nullingrels In-reply-to: <841511.1684596270@sss.pgh.pa.us> References: <3563470.1684352094@sss.pgh.pa.us> <3853324.1684427579@sss.pgh.pa.us> <841511.1684596270@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Sat, 20 May 2023 11:24:30 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <395226.1684698244.0@sss.pgh.pa.us> Date: Sun, 21 May 2023 15:44:43 -0400 Message-ID: <395264.1684698283@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <395226.1684698244.1@sss.pgh.pa.us> I wrote: > If they have the same clause_relids, then clearly in its current > form clause_is_computable_at() cannot distinguish them. So what > I now think we should do is have clause_is_computable_at() examine > their required_relids instead. Those will be different, by > construction in deconstruct_distribute_oj_quals(), ensuring that > we select only one of the group of clones. Since we're hard up against the beta1 wrap deadline, I went ahead and pushed the v5 patch. I doubt that it's perfect yet, but it's a small change and demonstrably fixes the cases we know about. As I said in the commit message, the main knock I'd lay on v5 is "why not use required_relids all the time?". I tried to do that and soon found that the answer is that we're not maintaining required_relids very accurately. I found two causes so far: 1. equivclass.c sometimes generates placeholder constant-true join clauses, and it's being sloppy about that. It copies the required_relids of the original clause, but fails to copy is_pushed_down, making the clause look like it's been assigned to the wrong side of the join-clause-vs-filter-clause divide. I found that we need to copy has_clone/is_clone as well. The attached quick-hack patch avoids the bugs, but now I feel like it was a mistake to not add has_clone/is_clone as full-fledged arguments of make_restrictinfo. I'm inclined to change that, but not right before beta1 when we have no evidence of a reachable bug. (Mind you, there might *be* a reachable bug, but ...) 2. When distribute_qual_to_rels forces a qual up to a particular syntactic level, it applies a relid set that very possibly refers to rels the clause doesn't actually depend on. This is problematic because if the clause gets postponed to above some outer join that nulls those rels, then it looks like it's being evaluated in an unsafe location. I think that when we detect commutability of two outer joins, we need to adjust the relevant min_xxxhand sets more thoroughly than we do now. I've not managed to write a patch for that yet. One problem is that if we insist on removing all unreferenced rels from required_relids, we might end up with a set that mentions *none* of the RHS and therefore fails to keep the clause from dropping into the LHS where it must not go. Not sure about a nice way to handle that. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="clean-up-const-true-clauses-wip.patch"; charset="us-ascii" Content-ID: <395226.1684698244.2@sss.pgh.pa.us> Content-Description: clean-up-const-true-clauses-wip.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimiz= er/path/equivclass.c index 2db1bf6448..e0077aa1e4 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -1993,20 +1993,24 @@ reconsider_outer_join_clauses(PlannerInfo *root) if (reconsider_outer_join_clause(root, ojcinfo, true)) { RestrictInfo *rinfo =3D ojcinfo->rinfo; + RestrictInfo *ninfo; = found =3D true; /* remove it from the list */ root->left_join_clauses =3D foreach_delete_current(root->left_join_clauses, cell); /* throw back a dummy replacement clause (see notes above) */ - rinfo =3D make_restrictinfo(root, + ninfo =3D make_restrictinfo(root, (Expr *) makeBoolConst(true, false), - true, /* is_pushed_down */ - false, /* pseudoconstant */ + rinfo->is_pushed_down, + false, /* !pseudoconstant */ 0, /* security_level */ rinfo->required_relids, rinfo->outer_relids); - distribute_restrictinfo_to_rels(root, rinfo); + /* copy clone marking, too */ + ninfo->has_clone =3D rinfo->has_clone; + ninfo->is_clone =3D rinfo->is_clone; + distribute_restrictinfo_to_rels(root, ninfo); } } = @@ -2018,20 +2022,24 @@ reconsider_outer_join_clauses(PlannerInfo *root) if (reconsider_outer_join_clause(root, ojcinfo, false)) { RestrictInfo *rinfo =3D ojcinfo->rinfo; + RestrictInfo *ninfo; = found =3D true; /* remove it from the list */ root->right_join_clauses =3D foreach_delete_current(root->right_join_clauses, cell); /* throw back a dummy replacement clause (see notes above) */ - rinfo =3D make_restrictinfo(root, + ninfo =3D make_restrictinfo(root, (Expr *) makeBoolConst(true, false), - true, /* is_pushed_down */ - false, /* pseudoconstant */ + rinfo->is_pushed_down, + false, /* !pseudoconstant */ 0, /* security_level */ rinfo->required_relids, rinfo->outer_relids); - distribute_restrictinfo_to_rels(root, rinfo); + /* copy clone marking, too */ + ninfo->has_clone =3D rinfo->has_clone; + ninfo->is_clone =3D rinfo->is_clone; + distribute_restrictinfo_to_rels(root, ninfo); } } = @@ -2043,20 +2051,24 @@ reconsider_outer_join_clauses(PlannerInfo *root) if (reconsider_full_join_clause(root, ojcinfo)) { RestrictInfo *rinfo =3D ojcinfo->rinfo; + RestrictInfo *ninfo; = found =3D true; /* remove it from the list */ root->full_join_clauses =3D foreach_delete_current(root->full_join_clauses, cell); /* throw back a dummy replacement clause (see notes above) */ - rinfo =3D make_restrictinfo(root, + ninfo =3D make_restrictinfo(root, (Expr *) makeBoolConst(true, false), - true, /* is_pushed_down */ - false, /* pseudoconstant */ + rinfo->is_pushed_down, + false, /* !pseudoconstant */ 0, /* security_level */ rinfo->required_relids, rinfo->outer_relids); - distribute_restrictinfo_to_rels(root, rinfo); + /* copy clone marking, too */ + ninfo->has_clone =3D rinfo->has_clone; + ninfo->is_clone =3D rinfo->is_clone; + distribute_restrictinfo_to_rels(root, ninfo); } } } while (found); diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optim= izer/util/restrictinfo.c index 760d24bebf..8558c8acd4 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -513,8 +513,8 @@ extract_actual_join_clauses(List *restrictinfo_list, { /* joinquals shouldn't have been marked pseudoconstant */ Assert(!rinfo->pseudoconstant); - Assert(!rinfo_is_constant_true(rinfo)); - *joinquals =3D lappend(*joinquals, rinfo->clause); + if (!rinfo_is_constant_true(rinfo)) + *joinquals =3D lappend(*joinquals, rinfo->clause); } } } ------- =_aaaaaaaaaa0--