On 2018-Jun-14, Amit Langote wrote:
> 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
> > s
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
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
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
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
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 the
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 the
On 9 June 2018 at 04:52, Alvaro Herrera wrote:
> Truth is, the more I look at this, the more I think it should be done in
> the way Amit Langote was proposing: do away with the extra function, and
> check all those conditions inside ExecConstraints itself. We can add a
> new boolean tupleRouting
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
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 i
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 it
On 2018/06/07 15:02, David Rowley wrote:
> 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 th
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 fol
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
>>
>> if
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 h
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 && resultRelInfo->r
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 t
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 funct
On 7 June 2018 at 09:08, Alvaro Herrera wrote:
> I wonder if we should create a new small function that takes the two
> resultRelInfos and returns the correct boolean --maybe something like
> ExecConstraintsPartConstrNeedsRecheck()-- and then the smarts are in a
> single place and we diminish the
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 pr
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 coding, so we need to correct that in this patch, pleas
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, please.
>
> Thanks for looking.
>
> Yeah, the comment
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 (resultRelInfo->ri_PartitionRoot == NULL ||
>>> (resultRelInfo->ri_T
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 &&
>> resultRelInfo->ri_TrigDesc->trig_
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 = r
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
>> now done with an if/else if/else clause for both COPY
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.
>
> Hopefully, that's easier to understan
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 ri_
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
>> gives an impression that ri_Partiti
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 an impression that ri_PartitionR
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.
If this is some new coding rule
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 looking.
>
> Yeah, the
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 it a bit easier to
docume
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 CopyFrom.
>>
>> I think so too.
>>
>> Updated patch attached.
>
> Pa
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 attached.
Patch is good.
The cause of this oversight is the lack
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.
Regards,
Amit
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 Development, 24x7 Support, Training
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 dis
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, providi
39 matches
Mail list logo
