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

[Alvaro Herrera]

David Rowley wrote:
> On 19 January 2018 at 16:00, Kyotaro HORIGUCHI
>  wrote:
> > And I'd like to ask David to check out his mail environment so
> > that SPF record is available for his message.
> 
> Will investigate

This should be fixed now.  Please let us know if you still see problems.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

[David Rowley]

On 23 January 2018 at 23:22, Amit Langote  wrote:
> On 2018/01/23 15:44, David Rowley wrote:
>> Attached is what I had in mind about how to do this.
>
> Thanks for the delta patch.  I will start looking at it tomorrow.

Thanks. I've been looking more at this and I've made a few more
adjustments in the attached.

This delta patch should be applied against the
faster_partition_prune_v21_delta_drowley_v1.patch one I sent
yesterday. This changes a few comments, also now correctly passes the
context to get_partitions_excluded_by_ne_clauses and fixes a small
error where the patch was failing to record a notnull for the
partition key when it saw a strict <> clause. It was only doing this
for the opposite case, but both seem to be perfectly applicable. I
also made a small adjustment to the regression tests to ensure this is
covered.

I'm now going to start work on basing the partition pruning patch on
top of this.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


faster_partition_prune_v21_delta_drowley_v1_delta.patch
Description: Binary data

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

[David Rowley]

Hi Amit
,
On 19 January 2018 at 04:03, David Rowley  wrote:
> On 18 January 2018 at 23:56, Amit Langote  
> wrote:
>> So, I've been assuming that the planner changes in the run-time pruning
>> patch have to do with selecting clauses (restriction clauses not
>> containing Consts and/or join clauses) to be passed to the executor by
>> recording them in the Append node.  Will they be selected by the planner
>> calling into partition.c?
>
> I had thought so. I only have a rough idea in my head and that's that
> the PartitionPruneInfo struct that I wrote for the run-time pruning
> patch would have the clause List replaced with this new
> PartScanClauseInfo struct (which likely means it needs to go into
> primnodes.h), this struct would contain all the partition pruning
> clauses in a more structured form so that no matching of quals to the
> partition key wouldn't be required during execution. The idea is that
> we'd just need to call; remove_redundant_clauses,
> extract_bounding_datums and get_partitions_for_keys. I think this is
> the bare minimum of work that can be done in execution since we can't
> remove the redundant clauses until we know the values of the Params.
>
> Likely this means there will need to be 2 functions externally
> accessible for this in partition.c. I'm not sure exactly how best to
> do this. Maybe it's fine just to have allpaths.c call
> extract_partition_key_clauses to generate the PartScanClauseInfo then
> call some version of get_partitions_from_clauses which does do the
> extract_partition_key_clauses duties. This is made more complex by the
> code that handles adding the default partition bound to the quals. I'm
> not yet sure where that should live.
>
> I've also been thinking of having some sort of PartitionPruneContext
> struct that we can pass around the functions. Runtime pruning had to
> add structs which store the Param values to various functions which I
> didn't like. It would be good to just add those to the context and
> have them passed down without having to bloat the parameters in the
> functions. I might try and do that tomorrow too. This should make the
> footprint of the runtime pruning patch is a bit smaller.

Attached is what I had in mind about how to do this. Only the planner
will need to call populate_partition_clause_info. The planner and
executor can call get_partitions_from_clauseinfo. I'll just need to
change the run-time prune patch to pass the PartitionClauseInfo into
the executor in the Append node.

I've also added the context struct that I mentioned above. It's
currently not carrying much weight, but the idea is that this context
will be passed around a bit more with the run-time pruning patch and
it will also carry the details about parameter values. I'll need to
modify a few signatures of functions like partkey_datum_from_expr to
pass the context there too. I didn't do that here because currently,
those functions have no use for the context with the fields that they
currently have.

I've also fixed a bug where when you built the commutator OpExpr in
what's now called extract_partition_key_clauses the inputcollid was
not being properly set. The changes I made there go much further than
just that, I've completely removed the OpExpr from the PartClause
struct as only two members were ever used. I thought it was better
just to add those to PartClause instead.

I also did some renaming of variables that always assumed that the
Expr being compared to the partition key was a constant. This is true
now, but with run-time pruning patch, it won't be, so I thought it was
better to do this here rather than having to rename them in the
run-time pruning patch.

One thing I don't yet understand about the patch is the use of
predicate_refuted_by() in get_partitions_from_or_clause_args(). I did
adjust the comment above that code, but I'm still not certain I fully
understand why that function has to be used instead of building the
clauses for the OR being processed along with the remaining clauses.
Is it that this was too hard to extract that you ended up using
predicate_refuted_by()?

I've also removed the makeBoolExpr call that you were encapsulating
the or_clauses in. I didn't really see the need for this since you
just removed it again when looping over the or_clauses.

The only other changes are just streamlining code and comment changes.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


faster_partition_prune_v21_delta_drowley_v1.patch
Description: Binary data

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

[David Rowley]

On 19 January 2018 at 16:00, Kyotaro HORIGUCHI
 wrote:
> And I'd like to ask David to check out his mail environment so
> that SPF record is available for his message.

Will investigate

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

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

[Kyotaro HORIGUCHI]

Hello,

At Thu, 18 Jan 2018 11:41:00 -0800, Andres Freund <and...@anarazel.de> wrote in 
<20180118194100.dy3kxdtktsbvm...@alap3.anarazel.de>
> Hi Amit,
> 
> It seems your mail system continually adds "[Sender Address Forgery]"
> prefixes to messages. E.g. this mail now has
> Subject: Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [Sender 
> Address Forgery]Re: [HACKERS] path toward faster partition pruning
> as its subject, whereas the mail you're replying to only had
> Subject: Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: 
> [HACKERS] path toward faster partition pruning
> two of them.
> 
> I think the two previous occurances of this also are from you.
> 
> This is somewhat annoying, could you try to figure out a) what the
> problem is b) how to prevent the subject being edited like that?

Our mail server is failing to fetch SPF record for David's mails
that received directly (not via -hakders ML) and the server adds
the subject header.  It is failing to fetch SPF record for
2ndquadrant.com. The reason might be that the envelope-from of
his mails is not consistent with his server's IP address.

Anyway, mails via -hackers ML doesn't suffer so, what Amit (and
I) side can do by myself is one of the following.

- Being careful to reply to the mails comming via the ML.
- Remove the added header by hand..


And I'd like to ask David to check out his mail environment so
that SPF record is available for his message.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

[Andres Freund]

Hi Amit,

It seems your mail system continually adds "[Sender Address Forgery]"
prefixes to messages. E.g. this mail now has
Subject: Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [Sender 
Address Forgery]Re: [HACKERS] path toward faster partition pruning
as its subject, whereas the mail you're replying to only had
Subject: Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] 
path toward faster partition pruning
two of them.

I think the two previous occurances of this also are from you.

This is somewhat annoying, could you try to figure out a) what the
problem is b) how to prevent the subject being edited like that?

Regards,

Andres

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

[David Rowley]

On 18 January 2018 at 23:56, Amit Langote  wrote:
>> I've not fully worked out how run-time pruning
>> will use this as it'll need another version of
>> get_partitions_from_clauses but passes in a PartScanClauseInfo
>> instead, and does not call extract_partition_key_clauses. That area
>> probably  needs some shuffling around so that does not end up a big
>> copy and paste of all that new logic.
>>
> So, I've been assuming that the planner changes in the run-time pruning
> patch have to do with selecting clauses (restriction clauses not
> containing Consts and/or join clauses) to be passed to the executor by
> recording them in the Append node.  Will they be selected by the planner
> calling into partition.c?

I had thought so. I only have a rough idea in my head and that's that
the PartitionPruneInfo struct that I wrote for the run-time pruning
patch would have the clause List replaced with this new
PartScanClauseInfo struct (which likely means it needs to go into
primnodes.h), this struct would contain all the partition pruning
clauses in a more structured form so that no matching of quals to the
partition key wouldn't be required during execution. The idea is that
we'd just need to call; remove_redundant_clauses,
extract_bounding_datums and get_partitions_for_keys. I think this is
the bare minimum of work that can be done in execution since we can't
remove the redundant clauses until we know the values of the Params.

Likely this means there will need to be 2 functions externally
accessible for this in partition.c. I'm not sure exactly how best to
do this. Maybe it's fine just to have allpaths.c call
extract_partition_key_clauses to generate the PartScanClauseInfo then
call some version of get_partitions_from_clauses which does do the
extract_partition_key_clauses duties. This is made more complex by the
code that handles adding the default partition bound to the quals. I'm
not yet sure where that should live.

I've also been thinking of having some sort of PartitionPruneContext
struct that we can pass around the functions. Runtime pruning had to
add structs which store the Param values to various functions which I
didn't like. It would be good to just add those to the context and
have them passed down without having to bloat the parameters in the
functions. I might try and do that tomorrow too. This should make the
footprint of the runtime pruning patch is a bit smaller.

> Meanwhile, here is a revised version (v21) that incorporates your changes.
>  I added you as the author in 0002 and 0005 patches.  I guess a v22 will
> have to follow very soon...

Thanks for merging that in.

I'll have a try at making this work tomorrow, although I'm not sure
yet how much time I'll have to dedicate to it as I have a few other
things to do too.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

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

[Amit Langote]

David,

On 2018/01/12 12:30, David Rowley wrote:
> Can you also perform a self-review of the patch? Some of the things
> I'm picking up are leftovers from a previous version of the patch. We
> might never get through this review if you keep leaving those around!

Sorry, I will look more closely before posting the next version.  I guess
I may have rushed a bit too much when posting the v18/v19 patches, partly
because it's been 3 weeks since v17 and I felt I needed to catch up
quickly given the activity on the run-time pruning thread which depends on
the patches here.

Thanks,
Amit