Обсуждение: 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, providing there's some other constraint. ExecFindPartition should have already located the correct partition and nothing should have changed in the absence of before row insert triggers, so it looks like we're fine to not bother re-checking. Or did I misunderstand? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
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 disable the check, but it goes > ahead regardless, providing there's some other constraint. > > ExecFindPartition should have already located the correct partition > and nothing should have changed in the absence of before row insert > triggers, so it looks like we're fine to not bother re-checking. I think you're right. So if we call ExecConstraints only because there are other tuple constraints (rd_att->constr != NULL), then currently we pass true for whether to check the partition constraint, irrespective of the value of check_partition_constr. I guess 19c47e7c820 should have done it the way your patch teaches it do to begin with. The patch to ExecInsert looks good, but I think we also need to do the same thing in CopyFrom. Thanks, Amit
On 10 May 2018 at 16:13, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 & Services
Вложения
On 2018/05/10 13:33, David Rowley wrote: > On 10 May 2018 at 16:13, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 05:33, David Rowley <david.rowley@2ndquadrant.com> wrote: > On 10 May 2018 at 16:13, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 of comments to explain the original coding, so we need to correct that in this patch, please. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/05/10 14:42, Simon Riggs wrote:
> On 10 May 2018 at 05:33, David Rowley <david.rowley@2ndquadrant.com> wrote:
>> On 10 May 2018 at 16:13, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 of comments to explain the
> original coding, so we need to correct that in this patch, please.
There does exist a comment but perhaps not where one would expect, that
is, not immediately above the call to ExecConstraints. The following
comment is written in both CopyFrom and ExecInsert just above where
check_partition_constr variable is set.
/*
* We always check the partition constraint, including when
* the tuple got here via tuple-routing. However we don't
* need to in the latter case if no BR trigger is defined on
* the partition. Note that a BR trigger might modify the
* tuple such that the partition constraint is no longer
* satisfied, so we need to check in that case.
*/
Thanks,
Amit
On 10 May 2018 at 17:42, Simon Riggs <simon@2ndquadrant.com> 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 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 prevents further mistakes. Patch attached. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
Hi David. On 2018/05/10 18:56, David Rowley wrote: > On 10 May 2018 at 17:42, Simon Riggs <simon@2ndquadrant.com> 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 > 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 prevents further mistakes. > > Patch attached. Thanks. I like this patch, both the rewording of comments and the code revision. 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. Thanks, Amit
Thanks for looking
On 11 May 2018 at 17:48, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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, then that's the first I've heard of it.
Scanning over the result of git grep -E "if \(!\w{1,}\)" it looks like
we have a bit of cleanup work to do before we can comply.
FWIW, I've previously been told off for the opposite.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Fri, May 11, 2018 at 06:12:38PM +1200, David Rowley wrote:
> On 11 May 2018 at 17:48, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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, then that's the first I've heard of it.
>
> Scanning over the result of git grep -E "if \(!\w{1,}\)" it looks like
> we have a bit of cleanup work to do before we can comply.
>
> FWIW, I've previously been told off for the opposite.
NULL maps to 0 so that's not really worth worrying. A lot of code paths
use one way or the other for pointers. 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).
--
Michael
Вложения
On 2018/05/11 15:12, David Rowley wrote:
> Thanks for looking
>
> On 11 May 2018 at 17:48, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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, then that's the first I've heard of it.
No, I don't know of any official coding rule either.
> Scanning over the result of git grep -E "if \(!\w{1,}\)" it looks like
> we have a bit of cleanup work to do before we can comply.
>
> FWIW, I've previously been told off for the opposite.
OK, no problem if you would like to leave it the way it it as it may just
be a matter of personal preference. I just told you what I recall being
told a few times and it was a bit easy to spot in such a small patch.
Thanks,
Amit
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_PartitionRoot do *not* compare to NULL, so what's in the David's patch is better left the way it is, if only for consistency. Sorry about the noise. Thanks, Amit
On 10 May 2018 at 15:26, David Rowley <david.rowley@2ndquadrant.com> 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 prevents further mistakes.
>
> Patch attached.
The patch looks good in terms of handling the redundant constraint check.
Since this patch also does some minor code restructuring with the if
conditions, below are some comments on them :
With the patch, the if condition uses ri_PartitionCheck, and further
ExecConstraints() also uses ri_PartitionCheck to decide whether to
call ExecPartitionCheck(). So instead, if we just don't bother about
ri_PartitionCheck outside ExecConstraints() and let ExecConstraints()
handle it, then the if conditions would get simpler :
if (resultRelInfo->ri_PartitionRoot == NULL ||
(resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_insert_before_row))
check_partition_constr = true;
else
check_partition_constr = false;
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
On 11 May 2018 at 14:50, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 10 May 2018 at 15:26, David Rowley <david.rowley@2ndquadrant.com> 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 prevents further mistakes.
>>
>> Patch attached.
>
> The patch looks good in terms of handling the redundant constraint check.
>
> Since this patch also does some minor code restructuring with the if
> conditions, below are some comments on them :
>
> With the patch, the if condition uses ri_PartitionCheck, and further
> ExecConstraints() also uses ri_PartitionCheck to decide whether to
> call ExecPartitionCheck(). So instead, if we just don't bother about
> ri_PartitionCheck outside ExecConstraints() and let ExecConstraints()
> handle it, then the if conditions would get simpler :
>
> if (resultRelInfo->ri_PartitionRoot == NULL ||
> (resultRelInfo->ri_TrigDesc &&
> resultRelInfo->ri_TrigDesc->trig_insert_before_row))
> check_partition_constr = true;
> else
> check_partition_constr = false;
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 = resultRelInfo->ri_PartitionCheck;
else
check_partition_constr = false;
>
> --
> Thanks,
> -Amit Khandekar
> EnterpriseDB Corporation
> The Postgres Database Company
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
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 = resultRelInfo->ri_PartitionCheck;
You'd be assigning a List pointer to a bool variable with this. Maybe you
meant:
check_partition_constr = (resultRelInfo->ri_PartitionCheck != NIL);
Thanks,
Amit
On 14 May 2018 at 16:49, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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_insert_before_row)) >> check_partition_constr = resultRelInfo->ri_PartitionCheck; > > You'd be assigning a List pointer to a bool variable with this. Maybe you > meant: > > check_partition_constr = (resultRelInfo->ri_PartitionCheck != NIL); I also noticed that. Apart from that, I think your version is correct, but I just don't think it's quite as easy to understand. Although that's certainly debatable. For now, I'll refrain from writing v4 unless there's some consensus that this is a better way to write it. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 14 May 2018 at 10:30, David Rowley <david.rowley@2ndquadrant.com> wrote: > On 14 May 2018 at 16:49, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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_insert_before_row)) >>> check_partition_constr = resultRelInfo->ri_PartitionCheck; >> >> You'd be assigning a List pointer to a bool variable with this. Maybe you >> meant: >> >> check_partition_constr = (resultRelInfo->ri_PartitionCheck != NIL); > > I also noticed that. Yes, I meant (resultRelInfo->ri_PartitionCheck != NIL) > Apart from that, I think your version is correct, > but I just don't think it's quite as easy to understand. Although > that's certainly debatable. > > For now, I'll refrain from writing v4 unless there's some consensus > that this is a better way to write it. Yes. if the simplicity is becoming debatable, I am ok with either. That was my own preference. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
On 10 May 2018 at 21:56, David Rowley <david.rowley@2ndquadrant.com> wrote: > On 10 May 2018 at 17:42, Simon Riggs <simon@2ndquadrant.com> 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 > 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 prevents further mistakes. > > Patch attached. While this does not cause any undesired behaviour, I think it's quite clear that it's unintended, so I've added this to the v11 open items list. If there's consensus that this is not the case then we can remove it from the list. I've just added it to ensure that a proper evaluation has been done. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2018/05/17 14:15, David Rowley wrote: > On 10 May 2018 at 21:56, David Rowley <david.rowley@2ndquadrant.com> wrote: >> On 10 May 2018 at 17:42, Simon Riggs <simon@2ndquadrant.com> 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 >> 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 prevents further mistakes. >> >> Patch attached. > > While this does not cause any undesired behaviour, I think it's quite > clear that it's unintended, so I've added this to the v11 open items > list. > > If there's consensus that this is not the case then we can remove it > from the list. I've just added it to ensure that a proper evaluation > has been done. Yeah, we should try to fix what I too think may just have been an oversight during PG 11 development. Thanks, Amit
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 prevents further mistakes. 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 risk of a divergence. It looks like a very ad-hoc thing to have a function for, but then the new argument to ExecConstraints() *is* pretty ad-hoc already, so encapsulating it seems better. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 7 June 2018 at 09:08, Alvaro Herrera <alvherre@2ndquadrant.com> 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 risk of a divergence. It looks like a > very ad-hoc thing to have a function for, but then the new argument to > ExecConstraints() *is* pretty ad-hoc already, so encapsulating it seems > better. 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. 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. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
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 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 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.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 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.)
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->ri_PartitionCheck &&
!ExecPartitionCheck(resultRelInfo, slot, estate))
ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
It seems that ExecConstraint receives all the information that's needed to
make that happen.
Thanks,
Amit
On 7 June 2018 at 16:05, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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->ri_PartitionCheck && > !ExecPartitionCheck(resultRelInfo, slot, estate)) > ExecPartitionCheckEmitError(resultRelInfo, slot, estate); I don't think so. The function Alvaro is talking about is specific to inserting a tuple into a table. The place you're proposing to put the call to it is not. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 7 June 2018 at 15:57, Alvaro Herrera <alvherre@2ndquadrant.com> 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 http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On 2018/06/07 13:10, David Rowley wrote: > On 7 June 2018 at 16:05, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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->ri_PartitionCheck && >> !ExecPartitionCheck(resultRelInfo, slot, estate)) >> ExecPartitionCheckEmitError(resultRelInfo, slot, estate); > > I don't think so. The function Alvaro is talking about is specific to > inserting a tuple into a table. The place you're proposing to put the > call to it is not. Well, we need to determine whether or not to check the partition constraint exactly before inserting a tuple into a table and that's also when ExecConstraints is called, so this objection is not clear to me. I'm saying that instead of encapsulating that logic in a new function and calling it from a number of places, refactor things such that we call ExecConstraints unconditionally and decide there which constraints to check and which ones to not. Having that logic separated from ExecConstraints is what caused us to have this discussion in the first place. I'm attaching a patch here to show what I'm saying. Thanks, Amit
Вложения
On 7 June 2018 at 17:45, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2018/06/07 13:10, David Rowley wrote: >> On 7 June 2018 at 16:05, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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->ri_PartitionCheck && >>> !ExecPartitionCheck(resultRelInfo, slot, estate)) >>> ExecPartitionCheckEmitError(resultRelInfo, slot, estate); >> >> I don't think so. The function Alvaro is talking about is specific to >> inserting a tuple into a table. The place you're proposing to put the >> call to it is not. > > Well, we need to determine whether or not to check the partition > constraint exactly before inserting a tuple into a table and that's also > when ExecConstraints is called, so this objection is not clear to me. > > I'm saying that instead of encapsulating that logic in a new function and > calling it from a number of places, refactor things such that we call > ExecConstraints unconditionally and decide there which constraints to > check and which ones to not. Having that logic separated from > ExecConstraints is what caused us to have this discussion in the first place. > > I'm attaching a patch here to show what I'm saying. I might not have fully explained what I meant. I'm guilty of thinking it was pretty clear when I wrote my objection. I meant: ExecConstraints is not only called during INSERT and COPY TO, it's also used during UPDATE [1]. What you're proposing seems to be adding a check for trig_insert_before_row into a function that will be called during UPDATE. Can you explain why you think that's a good thing to do? I'm don't really see why UPDATE should care about the presence, (or lack of) a BEFORE ROW INSERT trigger. [1] https://github.com/michaelpq/postgres/blob/master/src/backend/executor/nodeModifyTable.c#L1174 -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2018/06/07 15:02, David Rowley wrote: > On 7 June 2018 at 17:45, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> On 2018/06/07 13:10, David Rowley wrote: >>> On 7 June 2018 at 16:05, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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->ri_PartitionCheck && >>>> !ExecPartitionCheck(resultRelInfo, slot, estate)) >>>> ExecPartitionCheckEmitError(resultRelInfo, slot, estate); >>> >>> I don't think so. The function Alvaro is talking about is specific to >>> inserting a tuple into a table. The place you're proposing to put the >>> call to it is not. >> >> Well, we need to determine whether or not to check the partition >> constraint exactly before inserting a tuple into a table and that's also >> when ExecConstraints is called, so this objection is not clear to me. >> >> I'm saying that instead of encapsulating that logic in a new function and >> calling it from a number of places, refactor things such that we call >> ExecConstraints unconditionally and decide there which constraints to >> check and which ones to not. Having that logic separated from >> ExecConstraints is what caused us to have this discussion in the first place. >> >> I'm attaching a patch here to show what I'm saying. > > I might not have fully explained what I meant. I'm guilty of thinking > it was pretty clear when I wrote my objection. > > I meant: > > ExecConstraints is not only called during INSERT and COPY TO, it's > also used during UPDATE [1]. Thank you for clarifying. > What you're proposing seems to be adding a check for > trig_insert_before_row into a function that will be called during > UPDATE. > > Can you explain why you think that's a good thing to do? I'm don't > really see why UPDATE should care about the presence, (or lack of) a > BEFORE ROW INSERT trigger. trig_insert_before_row is checked only if tuple routing has occurred (resultRelInfo->ri_PartitionRoot != NULL), which can only be true if ExecConstraints is called either from CopyFrom or ExecInsert. You may know that even UPDATE tuple routing internally calls ExecInsert for the actual routing to occur. The point here is that we want to avoid doing the partition constraint check if tuple routing has occurred, except the presence of a BR trigger will require it to be performed. Tuple routing only happens in CopyFrom or ExecInsert, so checking trig_insert_before_row here doesn't sound wildly odd to me. There is one twist though. It may happen that ExecUpdate ends up performing tuple routing for one row and thus will have ri_PartitionRoot set. For another row, tuple routing may not be needed as the updated version of the row satisfies the partition constraint and now because ri_PartitionRoot has been set, ExecConstraint would end up performing the partition constraint check redundantly if trig_insert_before_row is true. Thanks, Amit
On 2018-Jun-07, David Rowley wrote:
> On 7 June 2018 at 15:57, Alvaro Herrera <alvherre@2ndquadrant.com> 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.
Thanks, looks nice (function name is stupidly long, my fault).
I wondered about the refactoring that Amit Khandekar is proposing.
Given the improved API you gave the new function, it appears we can
write it like this (untested):
bool
ExecConstraintsPartConstrNeedsRecheck(ResultRelInfo *resultRelInfo,
bool tupleRouting)
{
if (tupleRouting)
{
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_insert_before_row)
return true;
}
else if (resultRelInfo->ri_PartitionCheck != NIL)
return true;
return false; /* no need to recheck */
}
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 inline function in the header.
BTW I noticed that ExecConstraints() indicates that the given
resultRelInfo is "the original resultRelInfo, before tuple routing", but
that's demostrably false. What's up with that?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9 June 2018 at 03:24, Alvaro Herrera <alvherre@2ndquadrant.com> 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 inline function in the header. I wondered about that too. I've not tested it again, but I do have another patch locally which can about double the speed of COPY FROM for partitioned tables, so I have to admit I did gawk at the additional function call idea, but I'd rather see this fixed than on the shelf, so I went with it. I'll leave it up to you to how you'd like to format the if statement. I've written it the way I'm happy with. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2018-Jun-09, David Rowley wrote: > On 9 June 2018 at 03:24, Alvaro Herrera <alvherre@2ndquadrant.com> 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 inline function in the header. > > I wondered about that too. I've not tested it again, but I do have > another patch locally which can about double the speed of COPY FROM > for partitioned tables, so I have to admit I did gawk at the > additional function call idea, but I'd rather see this fixed than on > the shelf, so I went with it. > > I'll leave it up to you to how you'd like to format the if statement. > I've written it the way I'm happy with. 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 argument, which I think is the only missing bit. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9 June 2018 at 04:52, Alvaro Herrera <alvherre@2ndquadrant.com> 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 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, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > There is one twist though. It may happen that ExecUpdate ends up > performing tuple routing for one row and thus will have ri_PartitionRoot > set. For another row, tuple routing may not be needed as the updated > version of the row satisfies the partition constraint and now because > ri_PartitionRoot has been set, ExecConstraint would end up performing the > partition constraint check redundantly if trig_insert_before_row is true. I'm really struggling to understand why this is being suggested at all. 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, but I'm strongly against the reduction in modularity. I quite like ExecConstraints() the way it is. FWIW, my test case that shows Amit's patch is wrong is: drop table listp; create table listp (a int, b int) partition by list(a); create table listp12 partition of listp for values in(1,2); create table listp34 partition of listp for values in(3,4); create or replace function listp_insert_trigger() returns trigger as $$ begin raise notice '%', NEW.a; return NEW; end; $$ language plpgsql; create trigger listp12_insert before insert on listp12 for each row execute procedure listp_insert_Trigger(); insert into listp values(1,2); update listp set b=1; stick a breakpoint in ExecPartitionCheck() and watch it being hit twice on the update. Revert Amit's patch and you'll hit it once. I'm fine with my v3 patch, or your idea with the function in v5, but with respect to Amit L, I'm strongly against his patch on the grounds that it just moves the problem somewhere else. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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 reduction in modularity. > I quite like ExecConstraints() the way it is. I see. Maybe this business of sticking the partition constraint check in ExecConstraints was a bad idea all along. How about we rip it out and move the responsibility of doing ExecPartitionCheck to the caller instead? See attached. This whole business looks much cleaner to me this way. 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. I didn't see any tests of logical replication with partitioned tables ... Is the current understanding that we don't promise those things to work terribly well together? -- Álvaro Herrera https://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 reduction in modularity. > I quite like ExecConstraints() the way it is. I see. Maybe this business of sticking the partition constraint check in ExecConstraints was a bad idea all along. How about we rip it out and move the responsibility of doing ExecPartitionCheck to the caller instead? See attached. This whole business looks much cleaner to me this way. 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. I didn't see any tests of logical replication with partitioned tables ... Is the current understanding that we don't promise those things to work terribly well together? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Hearing no complaints I pushed it with the proposed shape. Thanks everyone, -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12 June 2018 at 09:13, Alvaro Herrera <alvherre@2ndquadrant.com> 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
On 2018/06/12 10:37, David Rowley wrote: > On 12 June 2018 at 09:13, Alvaro Herrera <alvherre@2ndquadrant.com> 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 seems to be the best. Regards, Amit
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 a given name on subscription side doesn't have the same partition constraint as on the publication side, subscription worker simply fails, which is the right thing to do anyway. ERROR: new row for relation "p1" violates partition constraint DETAIL: Failing row contains (1, 1). LOG: background worker "logical replication worker" (PID 25739) exited with exit code 1 Maybe, it's a user error to set up logical replication that way. > I didn't see any tests of logical replication with > partitioned tables ... Is the current understanding that we don't > promise those things to work terribly well together? As far as the documentation is concerned, there is this note on 31.4. Restrictions (Chapter 31. Logical Replication) https://www.postgresql.org/docs/devel/static/logical-replication-restrictions.html Replication is only possible from base tables to base tables. That is, the tables on the publication and on the subscription side must be normal tables, not views, materialized views, partition root tables, or foreign tables. In the case of partitions, you can therefore replicate a partition hierarchy one-to-one, but you cannot currently replicate to a differently partitioned setup. Attempts to replicate tables other than base tables will result in an error. Thanks, Amit
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 > > separate topic. > > Hmm, yes. If the partition of a given name on subscription side doesn't > have the same partition constraint as on the publication side, > subscription worker simply fails, which is the right thing to do anyway. > > ERROR: new row for relation "p1" violates partition constraint > DETAIL: Failing row contains (1, 1). > LOG: background worker "logical replication worker" (PID 25739) exited > with exit code 1 > > Maybe, it's a user error to set up logical replication that way. To me, the ideal would be that re-routing occurs if the partition constraint fails. The fact that the partition constraint is now separate from the rest of the tuple constraints makes this easy to implement, I think. > > I didn't see any tests of logical replication with > > partitioned tables ... Is the current understanding that we don't > > promise those things to work terribly well together? > > As far as the documentation is concerned, there is this note on > > 31.4. Restrictions (Chapter 31. Logical Replication) > https://www.postgresql.org/docs/devel/static/logical-replication-restrictions.html > > Replication is only possible from base tables to base tables. That is, > the tables on the publication and on the subscription side must be > normal tables, not views, materialized views, partition root tables, or > foreign tables. In the case of partitions, you can therefore replicate a > partition hierarchy one-to-one, but you cannot currently replicate to a > differently partitioned setup. Attempts to replicate tables other than > base tables will result in an error. Ah, okay, so this is already documented not to work. I guess it's a reasonable addition for the next release. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services