public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v18 1/5] introduce routine for checking mutually exclusive string GUCs 6+ messages / 3 participants [nested] [flat]
* [PATCH v18 1/5] introduce routine for checking mutually exclusive string GUCs @ 2023-02-15 22:28 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Nathan Bossart @ 2023-02-15 22:28 UTC (permalink / raw) --- src/backend/postmaster/pgarch.c | 8 +++----- src/backend/utils/misc/guc.c | 22 ++++++++++++++++++++++ src/include/utils/guc.h | 3 +++ 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index f97035ca03..dcb5222bbe 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -815,11 +815,9 @@ LoadArchiveLibrary(void) { ArchiveModuleInit archive_init; - if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0') - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("both archive_command and archive_library set"), - errdetail("Only one of archive_command, archive_library may be set."))); + (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library", + XLogArchiveCommand, "archive_command", + ERROR); /* * If shell archiving is enabled, use our special initialization function. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index dd5a46469a..b5c8e3702a 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -2660,6 +2660,28 @@ ReportGUCOption(struct config_generic *record) pfree(val); } +/* + * If both parameters are set, emits a log message at 'elevel' and returns + * false. Otherwise, returns true. + */ +bool +CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name, + const char *p2val, const char *p2name, + int elevel) +{ + if (p1val[0] != '\0' && p2val[0] != '\0') + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot set both %s and %s", p1name, p2name), + errdetail("Only one of %s or %s may be set.", + p1name, p2name))); + return false; + } + + return true; +} + /* * Convert a value from one of the human-friendly units ("kB", "min" etc.) * to the given base unit. 'value' and 'unit' are the input value and unit diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 3712aba09b..3088ada610 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -376,6 +376,9 @@ extern void RestrictSearchPath(void); extern void AtEOXact_GUC(bool isCommit, int nestLevel); extern void BeginReportingGUCOptions(void); extern void ReportChangedGUCOptions(void); +extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name, + const char *p2val, const char *p2name, + int elevel); extern void ParseLongOption(const char *string, char **name, char **value); extern const char *get_config_unit_name(int flags); extern bool parse_int(const char *value, int *result, int flags, -- 2.25.1 --IJpNTDwzlM2Ie8A6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v18-0002-refactor-code-for-restoring-via-shell.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: wrong query result due to wang plan @ 2023-02-17 07:35 Richard Guo <[email protected]> 2023-02-17 20:26 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Richard Guo @ 2023-02-17 07:35 UTC (permalink / raw) To: tender wang <[email protected]>; +Cc: pgsql-hackers On Thu, Feb 16, 2023 at 5:50 PM Richard Guo <[email protected]> wrote: > It seems we still need to check whether a variable-free qual comes from > somewhere that is below the nullable side of an outer join before we > decide that it can be evaluated at join domain level, just like we did > before. So I wonder if we can add a 'below_outer_join' flag in > JoinTreeItem, fill its value during deconstruct_recurse, and check it in > distribute_qual_to_rels() > It occurs to me that we can leverage JoinDomain to tell if we are below the nullable side of a higher-level outer join if the clause is not an outer-join clause. If we are belew outer join, the current JoinDomain is supposed to be a proper subset of top JoinDomain. Otherwise the current JoinDomain must be equal to top JoinDomain. And that leads to a fix as code changes below. --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -2269,8 +2269,16 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause, } else { - /* eval at join domain level */ - relids = bms_copy(jtitem->jdomain->jd_relids); + JoinDomain *top_jdomain = + linitial_node(JoinDomain, root->join_domains); + + /* + * eval at original syntactic level if we are below an outer join, + * otherwise eval at join domain level, which is actually the top + * of tree + */ + relids = jtitem->jdomain == top_jdomain ? + bms_copy(jtitem->jdomain->jd_relids) : bms_copy(qualscope); Thanks Richard ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: wrong query result due to wang plan 2023-02-17 07:35 Re: wrong query result due to wang plan Richard Guo <[email protected]> @ 2023-02-17 20:26 ` Tom Lane <[email protected]> 2023-02-19 19:01 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2023-02-17 20:26 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: tender wang <[email protected]>; pgsql-hackers Richard Guo <[email protected]> writes: > It occurs to me that we can leverage JoinDomain to tell if we are below > the nullable side of a higher-level outer join if the clause is not an > outer-join clause. If we are belew outer join, the current JoinDomain > is supposed to be a proper subset of top JoinDomain. Otherwise the > current JoinDomain must be equal to top JoinDomain. And that leads to a > fix as code changes below. That doesn't look right at all: surely this situation can occur further down in a join tree, not only just below top level. I thought about this some more and realized that the whole business of trying to push a qual up the join tree is probably obsolete. In the first place, there's more than one problem with assigning the ON FALSE condition the required_relids {t2, t3, t4, t2/t4}. As you say, it causes bogus conclusions about which joinrel can be considered empty if we commute the outer joins' order. But also, doing it this way loses the information that t2/t3 can be considered empty, if we do the joins in an order where that is useful to know. In the second place, I think recording the info that t2/t3 is empty is probably sufficient now, because of the mark_dummy_rel/is_dummy_rel bookkeeping in joinrels.c (which did not exist when we first added this "push to top of tree" hack). If we know t2/t3 is empty then we will propagate that knowledge to {t2, t3, t4, t2/t4} when it's formed, without needing to have a clause that can be applied at that join level. So that leads to a conclusion that we could just forget the whole thing and always use the syntactic qualscope here. I tried that and it doesn't break any existing regression tests, which admittedly doesn't prove a lot in this area :-(. However, we'd then need to decide what to do in process_implied_equality, which is annotated as * "qualscope" is the nominal syntactic level to impute to the restrictinfo. * This must contain at least all the rels used in the expressions, but it * is used only to set the qual application level when both exprs are * variable-free. (Hence, it should usually match the join domain in which * the clause applies.) and indeed equivclass.c is passing a join domain relid set. It's not hard to demonstrate that that path is also broken, for instance explain (costs off) select * from int8_tbl t1 left join (int8_tbl t2 inner join int8_tbl t3 on (t2.q1-t3.q2) = 0 and (t2.q1-t3.q2) = 1 left join int8_tbl t4 on t2.q2 = t4.q2) on t1.q1 = t2.q1; One idea I played with is that we could take the join domain relid set and subtract off the OJ relid and RHS relids of any fully-contained SpecialJoinInfos, reasoning that we can reconstruct the fact that those won't make the join domain's overall result nondummy, and thereby avoiding the possibility of confusion if we end up commuting with one of those joins. This feels perhaps overly complicated, though, compared to the brain-dead-simple "use the syntactic scope" approach. Maybe we can get equivclass.c to do something equivalent to that? Or maybe we only need to use this complex rule in process_implied_equality? (I'm also starting to realize that the current basically-syntactic definition of join domains isn't really going to work with commutable outer joins, so maybe the ultimate outcome is that the join domain itself is defined in this more narrowly scoped way. But that feels like a task to tackle later.) regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: wrong query result due to wang plan 2023-02-17 07:35 Re: wrong query result due to wang plan Richard Guo <[email protected]> 2023-02-17 20:26 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> @ 2023-02-19 19:01 ` Tom Lane <[email protected]> 2023-02-20 09:51 ` Re: wrong query result due to wang plan Richard Guo <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2023-02-19 19:01 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: tender wang <[email protected]>; pgsql-hackers I wrote: > So that leads to a conclusion that we could just forget the whole > thing and always use the syntactic qualscope here. I tried that > and it doesn't break any existing regression tests, which admittedly > doesn't prove a lot in this area :-(. Hmm, I thought it worked, but when I tried it again just now I see a failure (well, a less efficient plan) for one of the recently added test cases. I also tried the idea of stripping off lower outer joins, but that doesn't work in distribute_qual_to_rels, because we haven't yet formed the SpecialJoinInfos for all the outer joins that are below the top of the join domain. So for now I agree with your solution in distribute_qual_to_rels. It's about equivalent to what we did in older branches, and it's not real clear that cases with pseudoconstant quals in lower join domains are common enough to be worth sweating over. We still have to fix process_implied_equality though, and in that context we do have all the SpecialJoinInfos, so the strip-the-outer-joins fix seems to work. See attached. regards, tom lane Attachments: [text/x-diff] fix-pseudoconstant-qual-placement-some-more.patch (8.9K, ../../[email protected]/2-fix-pseudoconstant-qual-placement-some-more.patch) download | inline diff: diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index 9132ce235f..faabe4d065 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -1248,11 +1248,11 @@ generate_base_implied_equalities_no_const(PlannerInfo *root, /* * The expressions aren't constants, so the passed qualscope will * never be used to place the generated clause. We just need to - * be sure it covers both expressions, so ec_relids will serve. + * be sure it covers both expressions, which em_relids should do. */ rinfo = process_implied_equality(root, eq_op, ec->ec_collation, prev_em->em_expr, cur_em->em_expr, - ec->ec_relids, + cur_em->em_relids, ec->ec_min_security, false); diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index fe4c8c63c9..8452720a18 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -125,6 +125,7 @@ static void distribute_qual_to_rels(PlannerInfo *root, Node *clause, bool is_clone, List **postponed_oj_qual_list); static bool check_redundant_nullability_qual(PlannerInfo *root, Node *clause); +static Relids get_join_domain_min_rels(PlannerInfo *root, Relids domain_relids); static void check_mergejoinable(RestrictInfo *restrictinfo); static void check_hashjoinable(RestrictInfo *restrictinfo); static void check_memoizable(RestrictInfo *restrictinfo); @@ -2250,8 +2251,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause, * RestrictInfo lists for the moment, but eventually createplan.c will * pull it out and make a gating Result node immediately above whatever * plan node the pseudoconstant clause is assigned to. It's usually best - * to put a gating node as high in the plan tree as possible, which we can - * do by assigning it the full relid set of the current JoinDomain. + * to put a gating node as high in the plan tree as possible. */ if (bms_is_empty(relids)) { @@ -2269,8 +2269,19 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause, } else { - /* eval at join domain level */ - relids = bms_copy(jtitem->jdomain->jd_relids); + /* + * If we are in the top-level join domain, we can push the qual to + * the top of the plan tree. Otherwise, be conservative and eval + * it at original syntactic level. (Ideally we'd push it to the + * top of the current join domain in all cases, but that causes + * problems if we later rearrange outer-join evaluation order. + * Pseudoconstant quals below the top level are a pretty odd case, + * so it's not clear that it's worth working hard on.) + */ + if (jtitem->jdomain == (JoinDomain *) linitial(root->join_domains)) + relids = bms_copy(jtitem->jdomain->jd_relids); + else + relids = bms_copy(qualscope); /* mark as gating qual */ pseudoconstant = true; /* tell createplan.c to check for gating quals */ @@ -2734,12 +2745,13 @@ process_implied_equality(PlannerInfo *root, /* * If the clause is variable-free, our normal heuristic for pushing it * down to just the mentioned rels doesn't work, because there are none. - * Apply it as a gating qual at the given qualscope. + * Apply it as a gating qual at the appropriate level (see comments for + * get_join_domain_min_rels). */ if (bms_is_empty(relids)) { - /* eval at join domain level */ - relids = bms_copy(qualscope); + /* eval at join domain's safe level */ + relids = get_join_domain_min_rels(root, qualscope); /* mark as gating qual */ pseudoconstant = true; /* tell createplan.c to check for gating quals */ @@ -2856,6 +2868,45 @@ build_implied_join_equality(PlannerInfo *root, return restrictinfo; } +/* + * get_join_domain_min_rels + * Remove any lower outer joins from a JoinDomain's relid set. + * + * When we derive a pseudoconstant (Var-free) clause from an EquivalenceClass, + * we'd ideally apply the clause at the top level of the EC's join domain. + * However, if there are any outer joins inside that domain that get commuted + * with joins outside it, that leads to not finding a correct place to apply + * the clause. Instead, remove any lower outer joins from the relid set, + * and apply the clause to just the remaining rels. This still results in a + * correct answer, since if the clause produces FALSE then the LHS of these + * joins will be empty leading to an empty join result. + * + * Note: it's tempting to use this in distribute_qual_to_rels where it's + * dealing with pseudoconstant quals; but we can't because the necessary + * SpecialJoinInfos aren't all formed at that point. + * + * The result is always freshly palloc'd; we do not modify domain_relids. + */ +static Relids +get_join_domain_min_rels(PlannerInfo *root, Relids domain_relids) +{ + Relids result = bms_copy(domain_relids); + ListCell *lc; + + foreach(lc, root->join_info_list) + { + SpecialJoinInfo *sjinfo = (SpecialJoinInfo *) lfirst(lc); + + if (sjinfo->jointype == JOIN_LEFT && + bms_is_member(sjinfo->ojrelid, result)) + { + result = bms_del_member(result, sjinfo->ojrelid); + result = bms_del_members(result, sjinfo->syn_righthand); + } + } + return result; +} + /* * match_foreign_keys_to_quals diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 16318d9da2..5a2756b333 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5119,6 +5119,67 @@ from int8_tbl t1 -> Seq Scan on onek t4 (13 rows) +-- More tests of correct placement of pseudoconstant quals +-- simple constant-false condition +explain (costs off) +select * from int8_tbl t1 left join + (int8_tbl t2 inner join int8_tbl t3 on false + left join int8_tbl t4 on t2.q2 = t4.q2) +on t1.q1 = t2.q1; + QUERY PLAN +-------------------------------------- + Hash Left Join + Hash Cond: (t1.q1 = q1) + -> Seq Scan on int8_tbl t1 + -> Hash + -> Result + One-Time Filter: false +(6 rows) + +-- deduce constant-false from an EquivalenceClass +explain (costs off) +select * from int8_tbl t1 left join + (int8_tbl t2 inner join int8_tbl t3 on (t2.q1-t3.q2) = 0 and (t2.q1-t3.q2) = 1 + left join int8_tbl t4 on t2.q2 = t4.q2) +on t1.q1 = t2.q1; + QUERY PLAN +-------------------------------------- + Hash Left Join + Hash Cond: (t1.q1 = q1) + -> Seq Scan on int8_tbl t1 + -> Hash + -> Result + One-Time Filter: false +(6 rows) + +-- pseudoconstant based on an outer-level Param +explain (costs off) +select exists( + select * from int8_tbl t1 left join + (int8_tbl t2 inner join int8_tbl t3 on x0.f1 = 1 + left join int8_tbl t4 on t2.q2 = t4.q2) + on t1.q1 = t2.q1 +) from int4_tbl x0; + QUERY PLAN +--------------------------------------------------------------------- + Seq Scan on int4_tbl x0 + SubPlan 1 + -> Nested Loop Left Join + Join Filter: (t2.q2 = t4.q2) + -> Nested Loop Left Join + Join Filter: (t1.q1 = t2.q1) + -> Seq Scan on int8_tbl t1 + -> Materialize + -> Result + One-Time Filter: (x0.f1 = 1) + -> Nested Loop + -> Seq Scan on int8_tbl t2 + -> Materialize + -> Seq Scan on int8_tbl t3 + -> Materialize + -> Seq Scan on int8_tbl t4 +(16 rows) + -- check that join removal works for a left join when joining a subquery -- that is guaranteed to be unique by its GROUP BY clause explain (costs off) diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 2b2e094955..a38034bce7 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1846,6 +1846,31 @@ from int8_tbl t1 left join onek t4 on t2.q2 < t3.unique2; +-- More tests of correct placement of pseudoconstant quals + +-- simple constant-false condition +explain (costs off) +select * from int8_tbl t1 left join + (int8_tbl t2 inner join int8_tbl t3 on false + left join int8_tbl t4 on t2.q2 = t4.q2) +on t1.q1 = t2.q1; + +-- deduce constant-false from an EquivalenceClass +explain (costs off) +select * from int8_tbl t1 left join + (int8_tbl t2 inner join int8_tbl t3 on (t2.q1-t3.q2) = 0 and (t2.q1-t3.q2) = 1 + left join int8_tbl t4 on t2.q2 = t4.q2) +on t1.q1 = t2.q1; + +-- pseudoconstant based on an outer-level Param +explain (costs off) +select exists( + select * from int8_tbl t1 left join + (int8_tbl t2 inner join int8_tbl t3 on x0.f1 = 1 + left join int8_tbl t4 on t2.q2 = t4.q2) + on t1.q1 = t2.q1 +) from int4_tbl x0; + -- check that join removal works for a left join when joining a subquery -- that is guaranteed to be unique by its GROUP BY clause explain (costs off) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: wrong query result due to wang plan 2023-02-17 07:35 Re: wrong query result due to wang plan Richard Guo <[email protected]> 2023-02-17 20:26 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 2023-02-19 19:01 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> @ 2023-02-20 09:51 ` Richard Guo <[email protected]> 2023-02-22 17:41 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Richard Guo @ 2023-02-20 09:51 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: tender wang <[email protected]>; pgsql-hackers On Mon, Feb 20, 2023 at 3:01 AM Tom Lane <[email protected]> wrote: > We still have to fix process_implied_equality though, and in that > context we do have all the SpecialJoinInfos, so the strip-the-outer-joins > fix seems to work. See attached. Yeah, process_implied_equality is also broken for variable-free clause. I failed to notice that :-(. I'm looking at the strip-the-outer-joins codes. At first I wondered why it only removes JOIN_LEFT outer joins from below a JoinDomain's relids. After a second thought I think it's no problem here since only left outer joins have chance to commute with joins outside the JoinDomain. I'm thinking that maybe we can do the strip-the-outer-joins work only when it's not the top JoinDomain. When we are in the top JoinDomain, it seems to me that it's safe to push the qual to the top of the tree. - /* eval at join domain level */ - relids = bms_copy(qualscope); + /* eval at join domain's safe level */ + if (!bms_equal(qualscope, + ((JoinDomain *) linitial(root->join_domains))->jd_relids)) + relids = get_join_domain_min_rels(root, qualscope); + else + relids = bms_copy(qualscope); Thanks Richard ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: wrong query result due to wang plan 2023-02-17 07:35 Re: wrong query result due to wang plan Richard Guo <[email protected]> 2023-02-17 20:26 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 2023-02-19 19:01 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 2023-02-20 09:51 ` Re: wrong query result due to wang plan Richard Guo <[email protected]> @ 2023-02-22 17:41 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-02-22 17:41 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: tender wang <[email protected]>; pgsql-hackers Richard Guo <[email protected]> writes: > I'm thinking that maybe we can do the strip-the-outer-joins work only > when it's not the top JoinDomain. When we are in the top JoinDomain, it > seems to me that it's safe to push the qual to the top of the tree. Yeah, because there's nothing to commute with. Might as well do that for consistency with the distribute_qual_to_rels behavior, although I felt it was better to put it inside get_join_domain_min_rels. Pushed, thanks for the report! regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-02-22 17:41 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-15 22:28 [PATCH v18 1/5] introduce routine for checking mutually exclusive string GUCs Nathan Bossart <[email protected]> 2023-02-17 07:35 Re: wrong query result due to wang plan Richard Guo <[email protected]> 2023-02-17 20:26 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 2023-02-19 19:01 ` Re: wrong query result due to wang plan Tom Lane <[email protected]> 2023-02-20 09:51 ` Re: wrong query result due to wang plan Richard Guo <[email protected]> 2023-02-22 17:41 ` Re: wrong query result due to wang plan Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox