Re: Needless additional partition check in INSERT?

On 2018/06/09 3:41, Alvaro Herrera wrote: > BTW, while working on this, I was a bit disturbed by the > execReplication.c changes (namely: if the partitioning is not identical > on both sides, things are likely to blow up pretty badly), but that's a > separate topic. Hmm, yes. If the partition of

Re: Needless additional partition check in INSERT?

On 2018/06/12 10:37, David Rowley wrote: > On 12 June 2018 at 09:13, Alvaro Herrera wrote: >> Hearing no complaints I pushed it with the proposed shape. > > Thanks for working on it and pushing. Thank you David and Alvaro. I think the last solution involving calling ExecPartitionCheck directly

Re: Needless additional partition check in INSERT?

On 12 June 2018 at 09:13, Alvaro Herrera wrote: > Hearing no complaints I pushed it with the proposed shape. Thanks for working on it and pushing. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services

Re: Needless additional partition check in INSERT?

Hearing no complaints I pushed it with the proposed shape. Thanks everyone, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Needless additional partition check in INSERT?

On 2018-Jun-09, David Rowley wrote: > Of course, that could be fixed by adding something like "bool > isinsert" to ExecConstraints(), so that it does not do the needless > check on UPDATE, Yeah, that was my actual suggestion rather than using Amit's patch verbatim. > but I'm strongly against

Re: Needless additional partition check in INSERT?

On 2018-Jun-09, David Rowley wrote: > Of course, that could be fixed by adding something like "bool > isinsert" to ExecConstraints(), so that it does not do the needless > check on UPDATE, Yeah, that was my actual suggestion rather than using Amit's patch verbatim. > but I'm strongly against

Re: Needless additional partition check in INSERT?

tupleRouting argument, which I think is the only missing bit. As far as I see it what Amit Langote is proposing is to shift the needless additional partition check from INSERT to UPDATE. I've tested his patch and confirmed that this is the case. Amit has even written: On 7 June 2018 at 21:37,

Re: Needless additional partition check in INSERT?

On 2018-Jun-09, David Rowley wrote: > On 9 June 2018 at 03:24, Alvaro Herrera wrote: > > I was also wondering about introducing a new function call in this path > > where previously was none. Given the amount of other stuff that's > > happening when a tuple is inserted, I suppose it's not worth

Re: Needless additional partition check in INSERT?

On 9 June 2018 at 03:24, Alvaro Herrera wrote: > I was also wondering about introducing a new function call in this path > where previously was none. Given the amount of other stuff that's > happening when a tuple is inserted, I suppose it's not worth worrying > about in terms of making this an

Re: Needless additional partition check in INSERT?

On 2018-Jun-07, David Rowley wrote: > On 7 June 2018 at 15:57, Alvaro Herrera wrote: > > Hm I was thinking this new function would be companion to ExecConstrains > > (a fact I used in the name I proposed,) so it'd be in the same file > > (probably right after it.) > > Okay. v5 (attached) does

Re: Needless additional partition check in INSERT?

On 7 June 2018 at 17:45, Amit Langote wrote: > On 2018/06/07 13:10, David Rowley wrote: >> On 7 June 2018 at 16:05, Amit Langote wrote: >>> Or we could just not have a separate function and put the logic that >>> determines whether or not to check the partition constraint right before >>> the

Re: Needless additional partition check in INSERT?

On 2018/06/07 13:10, David Rowley wrote: > On 7 June 2018 at 16:05, Amit Langote wrote: >> Or we could just not have a separate function and put the logic that >> determines whether or not to check the partition constraint right before >> the following piece of code in ExecConstraints >> >>

Re: Needless additional partition check in INSERT?

On 7 June 2018 at 15:57, Alvaro Herrera wrote: > Hm I was thinking this new function would be companion to ExecConstrains > (a fact I used in the name I proposed,) so it'd be in the same file > (probably right after it.) Okay. v5 (attached) does it that way. -- David Rowley

Re: Needless additional partition check in INSERT?

On 7 June 2018 at 16:05, Amit Langote wrote: > Or we could just not have a separate function and put the logic that > determines whether or not to check the partition constraint right before > the following piece of code in ExecConstraints > > if (check_partition_constraint &&

Re: Needless additional partition check in INSERT?

On 2018/06/07 12:57, Alvaro Herrera wrote: > On 2018-Jun-07, David Rowley wrote: >> I'm personally not really for or against having the function. I agree >> that it's slightly weird, but anyway, here's the patch. I'll leave it >> up to you to which one you prefer, v3 or v4. > > Hm I was thinking

Re: Needless additional partition check in INSERT?

On 2018-Jun-07, David Rowley wrote: > Hi Alvaro, > > Thanks for looking at this. I thought it was strange to pass in both > resultRelInfos. I ended up just making the 2nd param a bool to > indicate of tuple routing was used. Good call. > I'm personally not really for or against having the

Re: Needless additional partition check in INSERT?

On 2018-May-10, David Rowley wrote: > Yeah, the comments do need work. In order to make it a bit easier to > document I changed the way that check_partition_constr is set. This is > now done with an if/else if/else clause for both COPY and INSERT. > > Hopefully, that's easier to understand and

Re: Needless additional partition check in INSERT?

On 2018/05/17 14:15, David Rowley wrote: > On 10 May 2018 at 21:56, David Rowley wrote: >> On 10 May 2018 at 17:42, Simon Riggs wrote: >>> Patch is good. >>> >>> The cause of this oversight is the lack of comments to explain the >>> original

Re: Needless additional partition check in INSERT?

On 10 May 2018 at 21:56, David Rowley wrote: > On 10 May 2018 at 17:42, Simon Riggs wrote: >> Patch is good. >> >> The cause of this oversight is the lack of comments to explain the >> original coding, so we need to correct that in this patch,

Re: Needless additional partition check in INSERT?

On 14 May 2018 at 10:30, David Rowley wrote: > On 14 May 2018 at 16:49, Amit Langote wrote: >> On 2018/05/11 18:43, Amit Khandekar wrote: >>> This looks better (it will avoid unnecessary ExecConstraints() call) : >>> >>> if

Re: Needless additional partition check in INSERT?

On 14 May 2018 at 16:49, Amit Langote wrote: > On 2018/05/11 18:43, Amit Khandekar wrote: >> This looks better (it will avoid unnecessary ExecConstraints() call) : >> >> if (resultRelInfo->ri_PartitionRoot == NULL || >> (resultRelInfo->ri_TrigDesc && >>

Re: Needless additional partition check in INSERT?

On 2018/05/11 18:43, Amit Khandekar wrote: > This looks better (it will avoid unnecessary ExecConstraints() call) : > > if (resultRelInfo->ri_PartitionRoot == NULL || > (resultRelInfo->ri_TrigDesc && > resultRelInfo->ri_TrigDesc->trig_insert_before_row)) > check_partition_constr =

Re: Needless additional partition check in INSERT?

On 11 May 2018 at 14:50, Amit Khandekar wrote: > On 10 May 2018 at 15:26, David Rowley wrote: >> Yeah, the comments do need work. In order to make it a bit easier to >> document I changed the way that check_partition_constr is set. This is >>

Re: Needless additional partition check in INSERT?

On 10 May 2018 at 15:26, David Rowley wrote: > Yeah, the comments do need work. In order to make it a bit easier to > document I changed the way that check_partition_constr is set. This is > now done with an if/else if/else clause for both COPY and INSERT. > >

Re: Needless additional partition check in INSERT?

On 2018/05/11 15:27, Michael Paquier wrote: > That's really up to the patch > author at the end (I prefer matching with NULL, but usually it is better > to comply with the surroundings for consistency). Yeah. I think in this case I'll have to withdraw my comment because most places that check

Re: Needless additional partition check in INSERT?

On 2018/05/11 15:12, David Rowley wrote: > Thanks for looking > > On 11 May 2018 at 17:48, Amit Langote wrote: >> By the way, >> >> +!resultRelInfo->ri_PartitionRoot) >> >> This should be resultRelInfo->ri_PartitionRoot == NULL, because the above >>

Re: Needless additional partition check in INSERT?

On Fri, May 11, 2018 at 06:12:38PM +1200, David Rowley wrote: > On 11 May 2018 at 17:48, Amit Langote wrote: >> By the way, >> >> +!resultRelInfo->ri_PartitionRoot) >> >> This should be resultRelInfo->ri_PartitionRoot == NULL, because the above >> gives

Re: Needless additional partition check in INSERT?

Thanks for looking On 11 May 2018 at 17:48, Amit Langote wrote: > By the way, > > +!resultRelInfo->ri_PartitionRoot) > > This should be resultRelInfo->ri_PartitionRoot == NULL, because the above > gives an impression that ri_PartitionRoot is a Boolean.

Re: Needless additional partition check in INSERT?

Hi David. On 2018/05/10 18:56, David Rowley wrote: > On 10 May 2018 at 17:42, Simon Riggs wrote: >> Patch is good. >> >> The cause of this oversight is the lack of comments to explain the >> original coding, so we need to correct that in this patch, please. > > Thanks for

Re: Needless additional partition check in INSERT?

On 10 May 2018 at 17:42, Simon Riggs wrote: > Patch is good. > > The cause of this oversight is the lack of comments to explain the > original coding, so we need to correct that in this patch, please. Thanks for looking. Yeah, the comments do need work. In order to make

Re: Needless additional partition check in INSERT?

On 2018/05/10 14:42, Simon Riggs wrote: > On 10 May 2018 at 05:33, David Rowley wrote: >> On 10 May 2018 at 16:13, Amit Langote wrote: >>> The patch to ExecInsert looks good, but I think we also need to do the >>> same thing in

Re: Needless additional partition check in INSERT?

On 10 May 2018 at 05:33, David Rowley wrote: > On 10 May 2018 at 16:13, Amit Langote wrote: >> The patch to ExecInsert looks good, but I think we also need to do the >> same thing in CopyFrom. > > I think so too. > > Updated patch

Re: Needless additional partition check in INSERT?

On 2018/05/10 13:33, David Rowley wrote: > On 10 May 2018 at 16:13, Amit Langote wrote: >> The patch to ExecInsert looks good, but I think we also need to do the >> same thing in CopyFrom. > > I think so too. > > Updated patch attached. Thanks, looks good.

Re: Needless additional partition check in INSERT?

On 10 May 2018 at 16:13, Amit Langote wrote: > The patch to ExecInsert looks good, but I think we also need to do the > same thing in CopyFrom. I think so too. Updated patch attached. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL

Re: Needless additional partition check in INSERT?

On 2018/05/10 12:55, David Rowley wrote: > Hi, > > Scanning ExecInsert, it looks like there's a needless additional > partition constraint check against the tuple. This should only be > required if there's a before row INSERT trigger. The code block up > one from the additional check tries to

Needless additional partition check in INSERT?

Hi, Scanning ExecInsert, it looks like there's a needless additional partition constraint check against the tuple. This should only be required if there's a before row INSERT trigger. The code block up one from the additional check tries to disable the check, but it goes ahead regardless,