Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Sep 14, 2017 at 12:59 AM, Amit Langote wrote: > Since Jeevan Ladhe mentioned this patch [1] earlier this week, sending the > rebased patches here for consideration. Actually there are only 2 patches > now, because 0002 above is rendered unnecessary by

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/08/07 11:05, Amit Langote wrote: > By the way, bulk of 0004 is refactoring which it seems is what Jeevan's > default partition patch set also includes as one of the patches [1]. It > got a decent amount review from Ashutosh. I broke it down into a separate > patch, so that the patch to

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/08/05 11:05, Robert Haas wrote: > On Thu, Aug 3, 2017 at 8:45 PM, Amit Langote > wrote: >>> 0003 needs a rebase. >> >> Rebased patch attached. > > Committed. Thank you. > I think 0004 is a new feature, so I'm leaving that for v11. Sure. By the way, bulk

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Aug 3, 2017 at 8:45 PM, Amit Langote wrote: >> 0003 needs a rebase. > > Rebased patch attached. Committed. I think 0004 is a new feature, so I'm leaving that for v11. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/08/04 3:29, Robert Haas wrote: > On Thu, Aug 3, 2017 at 1:04 AM, Amit Langote > wrote: >> Alright, attached updated 0001 does that. > > Committed 0001 and 0002. Thanks. > 0003 needs a rebase. Rebased patch attached. Thanks, Amit From

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Aug 3, 2017 at 1:04 AM, Amit Langote wrote: > Alright, attached updated 0001 does that. Committed 0001 and 0002. 0003 needs a rebase. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/08/02 20:35, Robert Haas wrote: > On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote > wrote: >> I too dislike the shape of attachRel. How about we rename attachRel to >> attachrel? So, attachrel_children, attachrel_constr, etc. It's still >> long though... :)

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote wrote: > I too dislike the shape of attachRel. How about we rename attachRel to > attachrel? So, attachrel_children, attachrel_constr, etc. It's still > long though... :) OK, I can live with that, I guess. -- Robert

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/08/02 10:27, Robert Haas wrote: > On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote > wrote: >> Since ATExecAttachPartition() deals with the possibility that the table >> being attached itself might be partitioned, someone reading the code might >> find it

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote wrote: > Since ATExecAttachPartition() deals with the possibility that the table > being attached itself might be partitioned, someone reading the code might > find it helpful to get some clue about whose

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for reviewing. On 2017/08/02 2:54, Robert Haas wrote: > On Mon, Jul 31, 2017 at 11:10 PM, Amit Langote > wrote: >> OK, these cosmetic changes are now in attached patch 0001. > > Regarding 0001: > > -List *childrels; > +List

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Mon, Jul 31, 2017 at 11:10 PM, Amit Langote wrote: > OK, these cosmetic changes are now in attached patch 0001. Regarding 0001: -List *childrels; +List *attachRel_children; I sorta don't see why this is necessary, or better. /* It's

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for taking a look at this. On 2017/08/01 6:26, Robert Haas wrote: > On Wed, Jul 26, 2017 at 9:50 PM, Amit Langote > wrote: >> At least patch 0001 does address a bug. Not sure if we can say that 0002 >> addresses a bug; it implements a feature that might be

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Wed, Jul 26, 2017 at 9:50 PM, Amit Langote wrote: > At least patch 0001 does address a bug. Not sure if we can say that 0002 > addresses a bug; it implements a feature that might be a > nice-to-have-in-PG-10. I think 0001 is actually several fixes that should

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for looking at this again. On 2017/07/26 23:31, Ashutosh Bapat wrote: > On Wed, Jul 12, 2017 at 7:17 AM, Amit Langote > wrote: >> >> Thanks for the review. Patch updated taking care of the comments. > > The patches still apply and compile. make check runs

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Wed, Jul 12, 2017 at 7:17 AM, Amit Langote wrote: > > Thanks for the review. Patch updated taking care of the comments. The patches still apply and compile. make check runs well. I do not have any further review comments. Given that they address a bug, should

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/07/11 19:49, Ashutosh Bapat wrote: > On Tue, Jul 4, 2017 at 9:51 AM, Amit Langote > wrote: > >> >> Attached updated patches. > > There's an extra "we" in > +* Note that attachRel's OID is in this list. If it's partitioned, we > +* we don't

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Tue, Jul 4, 2017 at 9:51 AM, Amit Langote wrote: > > Attached updated patches. There's an extra "we" in +* Note that attachRel's OID is in this list. If it's partitioned, we +* we don't need to schedule it to be scanned (would be a noop anyway

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for the review. On 2017/07/03 20:13, Ashutosh Bapat wrote: > Thanks for working on the previous comments. The code really looks good now. > On Fri, Jun 23, 2017 at 2:29 PM, Amit Langote > wrote: >> >>> Don't we need an exclusive lock to >>> make sure that

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for working on the previous comments. The code really looks good now. On Fri, Jun 23, 2017 at 2:29 PM, Amit Langote wrote: > >> Don't we need an exclusive lock to >> make sure that the constraints are not changed while we are validating those? > > If I

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for the review again. On 2017/06/22 19:55, Ashutosh Bapat wrote: > On Thu, Jun 15, 2017 at 4:06 PM, Amit Langote > wrote: >> >> Anyway, I tried to implement the refactoring in patch 0002, which is not >> all of the patch 0001 that Jeevan posted. Please take

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Jun 15, 2017 at 4:06 PM, Amit Langote wrote: > > Anyway, I tried to implement the refactoring in patch 0002, which is not > all of the patch 0001 that Jeevan posted. Please take a look. I wondered > if we should emit a NOTICE when an individual leaf

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/15 18:05, Ashutosh Bapat wrote: > On Thu, Jun 15, 2017 at 2:30 PM, Amit Langote > wrote: >> On 2017/06/15 17:53, Ashutosh Bapat wrote: >>> On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote: > Both of the above comments are not related to the bug

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Jun 15, 2017 at 2:30 PM, Amit Langote wrote: > On 2017/06/15 17:53, Ashutosh Bapat wrote: >> On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote: Both of the above comments are not related to the bug that is being fixed, but they apply to the

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/15 17:53, Ashutosh Bapat wrote: > On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote: >>> Both of the above comments are not related to the bug that is being fixed, >>> but >>> they apply to the same code where the bug exists. So instead of fixing it >>> twice, may be we should expand

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote: > Thanks for the review. > > On 2017/06/15 16:08, Ashutosh Bapat wrote: >> On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote: >>> If we end up having to perform the validation scan and the table being >>>

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for the review. On 2017/06/15 16:08, Ashutosh Bapat wrote: > On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote: >> If we end up having to perform the validation scan and the table being >> attached is a partitioned table, we will scan its leaf partitions. Each >> of those leaf

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote: > Thanks for taking a look. > > On 2017/06/14 20:06, Ashutosh Bapat wrote: >> On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote >> wrote: >>> >>> By the way, I mentioned an existing

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for taking a look. On 2017/06/14 20:06, Ashutosh Bapat wrote: > On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote > wrote: >> >> By the way, I mentioned an existing problem in one of the earlier emails >> on this thread about differing attribute numbers in the

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Wed, Jun 14, 2017 at 6:15 AM, Ashutosh Bapat wrote: > PFA patch set addressing comments by Tom and Amit. LGTM. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote wrote: > > By the way, I mentioned an existing problem in one of the earlier emails > on this thread about differing attribute numbers in the table being > attached causing predicate_implied_by() to give up due to

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

PFA patch set addressing comments by Tom and Amit. 0001 is same as Robert's patch. On Wed, Jun 14, 2017 at 7:20 AM, Robert Haas wrote: > On Tue, Jun 13, 2017 at 5:28 PM, Tom Lane wrote: >> Robert Haas writes: >>> OK, I think I

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/14 5:36, Robert Haas wrote: > On Tue, Jun 13, 2017 at 10:24 AM, Robert Haas wrote: >> I think that's going to come as an unpleasant surprise to more than >> one user. I'm not sure exactly how we need to restructure things here >> so that this works properly, and

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/13 23:24, Robert Haas wrote: > On Mon, Jun 12, 2017 at 4:09 AM, Amit Langote > wrote: >> On 2017/06/09 20:49, Ashutosh Bapat wrote: >>> May be we should pass a flag to predicate_implied_by() to handle NULL >>> behaviour for CHECK constraints. Partitioning

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Tue, Jun 13, 2017 at 5:28 PM, Tom Lane wrote: > Robert Haas writes: >> OK, I think I see the problem here. predicate_implied_by() and >> predicate_refuted_by() differ in what they assume about the predicate >> evaluating to NULL, but both of them

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Robert Haas writes: > OK, I think I see the problem here. predicate_implied_by() and > predicate_refuted_by() differ in what they assume about the predicate > evaluating to NULL, but both of them assume that if the clause > evaluates to NULL, that's equivalent to false.

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Tue, Jun 13, 2017 at 10:24 AM, Robert Haas wrote: > I think that's going to come as an unpleasant surprise to more than > one user. I'm not sure exactly how we need to restructure things here > so that this works properly, and maybe modifying > predicate_implied_by()

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Mon, Jun 12, 2017 at 4:09 AM, Amit Langote wrote: > On 2017/06/09 20:49, Ashutosh Bapat wrote: >> May be we should pass a flag to predicate_implied_by() to handle NULL >> behaviour for CHECK constraints. Partitioning has shown that it needs >> to use

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/09 20:49, Ashutosh Bapat wrote: > May be we should pass a flag to predicate_implied_by() to handle NULL > behaviour for CHECK constraints. Partitioning has shown that it needs > to use predicate_implied_by() for comparing constraints and there may > be other cases that can come up in

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/09 20:47, Ashutosh Bapat wrote: > Thanks for the long explanation. I guess, this should be written in > comments somewhere in the code there. I see following comment in > ATExecAttachPartition() > -- > * > * There is a case in which we cannot rely on just the result of the >

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

May be we should pass a flag to predicate_implied_by() to handle NULL behaviour for CHECK constraints. Partitioning has shown that it needs to use predicate_implied_by() for comparing constraints and there may be other cases that can come up in future. Instead of handling it outside

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Fri, Jun 9, 2017 at 10:31 AM, Amit Langote wrote: > On 2017/06/08 19:25, Ashutosh Bapat wrote: >> On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote I think this code could be removed entirely in light of commit 3ec76ff1f2cf52e9b900349957b42d28128b7bc7. >>>

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/08 18:43, Amit Langote wrote: > On 2017/06/08 1:44, Robert Haas wrote: >> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat >> wrote: >>> In ATExecAttachPartition() there's following code >>> >>> 13715 partnatts = get_partition_natts(key); >>>

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/08 19:25, Ashutosh Bapat wrote: > On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote >>> I think this code could be removed entirely in light of commit >>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7. >> >> I am assuming you think that because now we emit IS NOT NULL constraint >> internally for

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote wrote: > On 2017/06/08 1:44, Robert Haas wrote: >> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat >> wrote: >>> In ATExecAttachPartition() there's following code >>> >>> 13715

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/08 1:44, Robert Haas wrote: > On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat > wrote: >> In ATExecAttachPartition() there's following code >> >> 13715 partnatts = get_partition_natts(key); >> 13716 for (i = 0; i < partnatts; i++) >>

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat wrote: > In ATExecAttachPartition() there's following code > > 13715 partnatts = get_partition_natts(key); > 13716 for (i = 0; i < partnatts; i++) > 13717 { > 13718 AttrNumber

[HACKERS] A bug in mapping attributes in ATExecAttachPartition()

In ATExecAttachPartition() there's following code 13715 partnatts = get_partition_natts(key); 13716 for (i = 0; i < partnatts; i++) 13717 { 13718 AttrNumber partattno; 13719 13720 partattno = get_partition_col_attnum(key, i); 13721 13722