Needless additional partition check in INSERT?

On 2018/05/10 12:55, David Rowley wrote:

Hi,

Scanning ExecInsert, it looks like there's a needless additional
partition constraint check against the tuple. This should only be
required if there's a before row INSERT trigger. The code block up
one from the additional check tries to 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