Horton

Thread

Commits

  1. Don't needlessly check the partition contraint twice

  1. Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-05-10T03:55:02Z

    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

  2. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-10T04:13:07Z

    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

  3. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-05-10T04:33:47Z

    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

  4. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-10T04:36:57Z

    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

  5. Re: Needless additional partition check in INSERT?

    Simon Riggs <simon@2ndquadrant.com> — 2018-05-10T05:42:03Z

    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

  6. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-10T07:14:14Z

    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

  7. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-05-10T09:56:09Z

    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

  8. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-11T05:48:15Z

    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

  9. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-05-11T06:12:38Z

    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

  10. Re: Needless additional partition check in INSERT?

    Michael Paquier <michael@paquier.xyz> — 2018-05-11T06:27:12Z

    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

  11. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-11T06:31:02Z

    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

  12. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-11T06:38:29Z

    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

  13. Re: Needless additional partition check in INSERT?

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-05-11T09:20:08Z

    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

  14. Re: Needless additional partition check in INSERT?

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-05-11T09:43:03Z

    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

  15. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-14T04:49:52Z

    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

  16. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-05-14T05:00:34Z

    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

  17. Re: Needless additional partition check in INSERT?

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-05-14T05:51:25Z

    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

  18. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-05-17T05:15:48Z

    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

  19. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-17T05:23:04Z

    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

  20. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-06T21:08:47Z

    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

  21. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-07T00:00:44Z

    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

  22. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-07T03:57:32Z

    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

  23. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-06-07T04:05:22Z

    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

  24. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-07T04:10:39Z

    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

  25. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-07T04:13:10Z

    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

  26. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-06-07T05:45:12Z

    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

  27. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-07T06:02:31Z

    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

  28. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-06-07T09:37:18Z

    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

  29. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T15:24:20Z

    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

  30. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-08T16:43:08Z

    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

  31. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T16:52:47Z

    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

  32. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-08T17:41:40Z

    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

  33. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T18:35:56Z

    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

  34. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T18:41:22Z

    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

  35. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-11T21:13:19Z

    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

  36. Re: Needless additional partition check in INSERT?

    David Rowley <david.rowley@2ndquadrant.com> — 2018-06-12T01:37:42Z

    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

  37. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-06-13T02:22:40Z

    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

  38. Re: Needless additional partition check in INSERT?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-06-14T07:49:58Z

    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

  39. Re: Needless additional partition check in INSERT?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-14T14:23:28Z

    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