On Sat, Mar 2, 2019 at 5:29 PM Tom Lane wrote:
>
> James Coleman writes:
> > On Fri, Mar 1, 2019 at 5:28 PM Tom Lane wrote:
> >> I also tweaked it to recognize the case where we can prove the
> >> array, rather than the scalar, to be null. I'm not sure how useful
> >> that is in practice, but i
James Coleman writes:
> On Fri, Mar 1, 2019 at 5:28 PM Tom Lane wrote:
>> I also tweaked it to recognize the case where we can prove the
>> array, rather than the scalar, to be null. I'm not sure how useful
>> that is in practice, but it seemed weird that we'd exploit that
>> only if we can also
On Fri, Mar 1, 2019 at 5:28 PM Tom Lane wrote:
>
> James Coleman writes:
> > [ saop_is_not_null-v10.patch ]
>
> I went through this again, and this time (after some more rewriting
> of the comments) I satisfied myself that the logic is correct.
> Hence, pushed.
Thanks!
> I also tweaked it to re
James Coleman writes:
> [ saop_is_not_null-v10.patch ]
I went through this again, and this time (after some more rewriting
of the comments) I satisfied myself that the logic is correct.
Hence, pushed.
I stripped down the regression test cases somewhat. Those were
good for development, but they
On Mon, Feb 18, 2019 at 4:53 PM Tom Lane wrote:
> So ... this is actively wrong.
>
> This case shows that you can't ignore the empty-array possibility
> for a ScalarArrayOpExpr with useOr = false, because
> "SELECT null::int = all(array[]::int[])" yields TRUE:
>
> contrib_regression=# select * fro
BTW, I just pushed a fix (e04a3905e) that adds a little more code
to clause_is_strict_for(). This shouldn't cause more than minor
rebasing pain for you, I hope.
regards, tom lane
James Coleman writes:
> I forgot to update in v8 so attaching v9.
So ... this is actively wrong.
This case shows that you can't ignore the empty-array possibility
for a ScalarArrayOpExpr with useOr = false, because
"SELECT null::int = all(array[]::int[])" yields TRUE:
contrib_regression=# selec
> > This comment seems wrong:
> >
> > + * However weak implication fails: e.g., "NULL IS NOT NULL" is false, but
> > + * "NULL = ANY(ARRAY[NULL])" is NULL, so non-falsity does not imply
> > non-falsity.
> >
> > "non-falsity does not imply non-falsity"? I suppose one of those
> > negations should
On Tue, Jan 22, 2019 at 4:26 AM Alvaro Herrera wrote:
>
> Hello, I gave this patch a very quick scan. I didn't check the actual
> logic behind it.
>
> This comment seems wrong:
>
> + * However weak implication fails: e.g., "NULL IS NOT NULL" is false, but
> + * "NULL = ANY(ARRAY[NULL])" is NULL,
Hello, I gave this patch a very quick scan. I didn't check the actual
logic behind it.
This comment seems wrong:
+ * However weak implication fails: e.g., "NULL IS NOT NULL" is false, but
+ * "NULL = ANY(ARRAY[NULL])" is NULL, so non-falsity does not imply
non-falsity.
"non-falsity does not im
On Wed, Jan 16, 2019 at 8:49 AM James Coleman wrote:
>
> On Tue, Jan 15, 2019 at 11:37 PM Tom Lane wrote:
> > Well, as I said upthread, it seems like we need to think a bit more
> > carefully about what it is that clause_is_strict_for is testing ---
> > and if that ends up finding that some other
James Coleman writes:
> One other question on testing: do you think the "calculated array"
> tests are good enough by themselves (i.e., remove the ones with array
> constants of > 100 values)? I dislike that it's not as obvious what's
> going on, but given that the code shouldn't care about array
On Tue, Jan 15, 2019 at 11:37 PM Tom Lane wrote:
> Well, as I said upthread, it seems like we need to think a bit more
> carefully about what it is that clause_is_strict_for is testing ---
> and if that ends up finding that some other name is more apposite,
> I'd not have any hesitation about rena
David Rowley writes:
> On Wed, 16 Jan 2019 at 14:29, James Coleman wrote:
>> Is your preference in this kind of case to comment the code and/or
>> tests but stick with int4pl even though it doesn't demonstrate the
>> "problem", or try to engineer a different test case such that the
>> *_holds res
James Coleman writes:
> On Tue, Jan 15, 2019 at 3:53 PM Tom Lane wrote:
>> I quite dislike taking the responsibility out of clause_is_strict_for
>> and putting it in the callers.
> The reason I moved it was that we're no longer just proving
> strictness, so it seemed odd to put it in a function
On Wed, 16 Jan 2019 at 14:29, James Coleman wrote:
>
> On Tue, Jan 15, 2019 at 8:14 PM David Rowley
> > While int4pl might do what you want, some other strict function might
> > not. A simple example would be a strict function that decided to
> > return NULL when the two ints combined overflowed i
On Tue, Jan 15, 2019 at 8:14 PM David Rowley
wrote:
>
> On Wed, 16 Jan 2019 at 14:05, James Coleman wrote:
> > At the risk of missing something obvious, I'm not sure I see a case
> > where "x is not null" does not imply "(x + x) is not null", at least
> > for integers. Since an integer + an integ
On Wed, 16 Jan 2019 at 14:05, James Coleman wrote:
> At the risk of missing something obvious, I'm not sure I see a case
> where "x is not null" does not imply "(x + x) is not null", at least
> for integers. Since an integer + an integer results in an integer,
> then it must imply the addition of
On Tue, Jan 15, 2019 at 4:36 PM David Rowley
wrote:
> I wasn't suggesting any code changes. I just thought the code was
> sufficiently hard to understand to warrant some additional tests that
> ensure we don't assume that if the int4 column x is not null that also
> x+x is not null. Only the reve
On Tue, Jan 15, 2019 at 3:53 PM Tom Lane wrote:
>
> James Coleman writes:
> > [ saop_is_not_null-v6.patch ]
>
> I quite dislike taking the responsibility out of clause_is_strict_for
> and putting it in the callers. Aside from requiring duplicative code,
> it means that this fails to prove anythi
On Wed, 16 Jan 2019 at 03:33, James Coleman wrote:
> > 2. I was also staring predicate_implied_by_simple_clause() a bit at
> > the use of clause_is_strict_for() to ensure that the IS NOT NULL's
> > operand matches the ScalarArrayOpExpr left operand. Since
> > clause_is_strict_for() = "Can we prov
James Coleman writes:
> [ saop_is_not_null-v6.patch ]
I quite dislike taking the responsibility out of clause_is_strict_for
and putting it in the callers. Aside from requiring duplicative code,
it means that this fails to prove anything for recursive situations
(i.e., where the ScalarArrayOp app
James Coleman writes:
> I'm attaching the current version of the patch with the new tests, but
> I'm not sure I understand the *_holds results mentioned above. Are
> they an artifact of the data under test? Or do they suggest a
> remaining bug? I'm a bit fuzzy on what to expect for *_holds though
On Tue, Jan 15, 2019 at 12:47 AM David Rowley
wrote:
> 1. In:
>
> + if (IsA(clause, ScalarArrayOpExpr))
> + {
> + ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause;
> + Node *subexpr = (Node *) ((NullTest *) predicate)->arg;
> + if (op_strict(saop->opno) &&
> + clause_is_strict_for((Node *) l
On Tue, 15 Jan 2019 at 12:24, James Coleman wrote:
> While I add that though I wanted to get this updated version out to
> get feedback on the approach.
I had a look at this and there's a couple of things I see:
1. In:
+ if (IsA(clause, ScalarArrayOpExpr))
+ {
+ ScalarArrayOpExpr *saop = (Scala
On Mon, Jan 14, 2019 at 11:34 AM Tom Lane wrote:
> >> Hm. That would be annoying, but on reflection I think maybe we don't
> >> actually need to worry about emptiness of the array after all. The
> >> thing that we need to prove, at least for the implication case, is
> >> "truth of the ScalarArra
James Coleman writes:
> Are those invariants we want to keep (and recognize as regression
> tests)? If so, I can confirm that they aren't duplicative of the rest
> of the file, and if not I can remove them.
I can't get terribly excited about them ... if we were changing their
behavior, or if ther
On Mon, Jan 14, 2019 at 11:34 AM Tom Lane wrote:
>
> James Coleman writes:
> > On Mon, Jan 14, 2019 at 11:08 AM Tom Lane wrote:
> >> I think these test cases don't actually prove much about the behavior
> >> of your patch. Wouldn't they be handled by the generic OR-conversion
> >> logic, since
James Coleman writes:
> On Mon, Jan 14, 2019 at 11:08 AM Tom Lane wrote:
>> I think these test cases don't actually prove much about the behavior
>> of your patch. Wouldn't they be handled by the generic OR-conversion
>> logic, since there's fewer than MAX_SAOP_ARRAY_SIZE items?
> Which ones ar
On Mon, Jan 14, 2019 at 11:08 AM Tom Lane wrote:
>
> James Coleman writes:
> > On Sun, Jan 13, 2019 at 3:06 PM Tom Lane wrote:
> >> There's still a logical problem here, which is that in order to
> >> prove that the ScalarArrayOpExpr yields NULL when its LHS does,
> >> you also have to prove tha
James Coleman writes:
> On Sun, Jan 13, 2019 at 3:06 PM Tom Lane wrote:
>> There's still a logical problem here, which is that in order to
>> prove that the ScalarArrayOpExpr yields NULL when its LHS does,
>> you also have to prove that the RHS is not an empty array.
> I've constructed a test ca
On Sun, Jan 13, 2019 at 3:06 PM Tom Lane wrote:
>
> There's still a logical problem here, which is that in order to
> prove that the ScalarArrayOpExpr yields NULL when its LHS does,
> you also have to prove that the RHS is not an empty array.
>
> Otherwise you're up against the fact that the OR of
James Coleman writes:
> [ saop_is_not_null-v3.patch ]
There's still a logical problem here, which is that in order to
prove that the ScalarArrayOpExpr yields NULL when its LHS does,
you also have to prove that the RHS is not an empty array.
Otherwise you're up against the fact that the OR of zero
On Sat, Jan 12, 2019 at 8:52 PM David Rowley
wrote:
>
> Basically, the planner assumes that the WHERE a IS NOT NULL index
> implies WHERE b IN(1,...,101), which is definitely not the case.
>
> Probably your new code needs to be expanded to become:
>
> if (IsA(clause, ScalarArrayOpExpr))
> {
>
On Sun, 13 Jan 2019 at 14:49, David Rowley wrote:
> I've not looked in detail, but perhaps the place to put the tests are
> in src/test/modules/test_predtest. This module adds a function named
> test_predtest() that you can likely use more easily than trying to
> EXPLAIN plans and hoping the plan
On Sun, 11 Nov 2018 at 10:33, James Coleman wrote:
> At first I was imagining having the parse keep track of whether an array const
> expr contained any nulls and perhaps adding generated quals (in an equivalence
> class?) to allow the planner to easily prove the index was correct. I'd been
> goin
James Coleman writes:
> Is there anything I can do to solicit reviewers for the current CF?
There is no active CF, which is why nothing is happening. We'll
start looking at the 2019-01 items in January. Right now, people
are either on vacation or working on their own patches.
Is there anything I can do to solicit reviewers for the current CF?
The patch is quite small and should be fairly simple to review.
- James
Also, my apologies for top posting; I forgot to remove the old email
before clicking send.
I've become more confident that this approach is correct after
discussions with others on my team and have added the patch to the
open commitfest.
I'm attaching v2 of the patch here with a regression test (that fails
on current master, but is green both with my patch and with current
master if you
I've recently been investigating improving our plans for queries like:
SELECT * FROM t WHERE t.foo IN (1, 2..1000);
where the table "t" has a partial index on "foo" where "foo IS NOT NULL".
Currently the planner generates an index [only] scan so long as the number
of
items in the IN expression
41 matches
Mail list logo
