Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning

Hi David.

On Wed, Jan 17, 2018 at 12:32 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> On 16 January 2018 at 21:08, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2018/01/12 12:30, David Rowley wrote:
>>> 8. The code in get_partitions_from_ne_clauses() does perform quite a
>>> few nested loops. I think a more simple way to would be to track the
>>> offsets you've seen in a Bitmapset. This would save you having to
>>> check for duplicates, as an offset can only contain a single datum.
>>> You'd just need to build a couple of arrays after that, one to sum up
>>> the offsets found per partition, and one for the total datums allowed
>>> in the partition. If the numbers match then you can remove the
>>> partition.
>>>
>>> I've written this and attached it to this email. It saves about 50
>>> lines of code and should perform much better for complex cases, for
>>> example, a large NOT IN list. This also implements #6.
>>
>> I liked your patch, so incorporated it, except, I feel slightly
>> uncomfortable about the new name that you chose for the function because
>> it sounds a bit generic.
>
> You're right. I only renamed it because I inverted the meaning of the
> function in the patch. It no longer did
> "get_partitions_from_ne_clauses", it did the opposite and give the
> partitions which can't match. Please feel free to think of a new
> better name. Is "get_partitions_excluded_by_ne_clauses" too long?
>
>>> I think you quite often use "the same" to mean "it". Can you change that?
>>
>> I guess that's just one of my many odd habits when writing English, all of
>> which I'm trying to get rid of, but apparently with limited success.  Must
>> try harder. :)
>
> Oops, on re-reading that it sounded as though I was asking you to
> change some habit, but I just meant the comments. I understand there
> will be places that use English where that's normal. It's just I don't
> recall seeing that in PostgreSQL code before.

No worries, I too slightly misread what you'd said.

When I double checked, I too couldn't find "the same" used the way as
I did in the patch.  So I actually ended up finding and replacing more
"the same"s with "it" than you had pointed out in your review in the
latest v20 patch.

> American English is
> pretty much the standard for the project, despite that not always
> being strictly applied (e.g we have a command called ANALYSE which is
> an alias for ANALYZE). I always try and do my best to spell words in
> American English (which is not where I'm from), which for me stretches
> about as far as putting 'z' in the place of some of my 's'es.

I see.

>>> 15. "the latter" is normally used when you're referring to the last
>>> thing in a list which was just mentioned. In this case, leftarg_const
>>> and rightarg_const is the list, so "the latter" should mean
>>> rightarg_const, but I think you mean to compare them using the
>>> operator.
>>>
>>> * If the leftarg_const and rightarg_const are both of the type expected
>>> * by op's operator, then compare them using the latter.
>>
>> Rewrote it as:
>>
>>      * We can compare leftarg_const and rightarg_const using op's operator
>>      * only if both are of the type expected by it.
>
> I'd probably write "expected type." instead of "type expected by it."

OK, will do.

>>> 17. The following example will cause get_partitions_for_keys_hash to misbehave:
>>>
>>> create table hashp (a int, b int) partition by hash (a, b);
>>> create table hashp1 partition of hashp for values with (modulus 4, remainder 0);
>>> create table hashp2 partition of hashp for values with (modulus 4, remainder 1);
>>> create table hashp3 partition of hashp for values with (modulus 4, remainder 3);
>>> create table hashp4 partition of hashp for values with (modulus 4, remainder 2);
>>> explain select * from hashp where a = 1 and a is null;

[ ... ]

>>> The above code will need to be made smarter. It'll likely crash if you
>>> change "b" to a pass-by-ref type.
>>
>> Hmm, not sure why.  It seems to work:
>
> Yeah, works now because you've added new code to test for
> contradictions in the quals, e.g a = 1 and a is null is now rejected
> as constfalse.

Oh, I see.  I thought you were talking of it as an independent issue.

Thanks,
Amit