Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-19 Thread Amit Langote
On 2018/04/20 4:40, Alvaro Herrera wrote: > Alvaro Herrera wrote: >> Amit Langote wrote: >> >>> Yeah, I too have wondered in the past what it would take to make >>> equalTupDescs() return true for parent and partitions. Maybe we can make >>> it work by looking a bit harder than I did then. >> >>

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-19 Thread Alvaro Herrera
Alvaro Herrera wrote: > Amit Langote wrote: > > > Yeah, I too have wondered in the past what it would take to make > > equalTupDescs() return true for parent and partitions. Maybe we can make > > it work by looking a bit harder than I did then. > > How about simply relaxing the tdtypeid test

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-19 Thread Peter Geoghegan
On Thu, Apr 19, 2018 at 12:00 PM, Robert Haas wrote: > On Thu, Apr 19, 2018 at 1:20 PM, Alvaro Herrera > wrote: >> How about simply relaxing the tdtypeid test from equalTupleDescs? I >> haven't looked deeply but I think just checking whether or

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-19 Thread Robert Haas
On Thu, Apr 19, 2018 at 1:20 PM, Alvaro Herrera wrote: > Amit Langote wrote: >> Yeah, I too have wondered in the past what it would take to make >> equalTupDescs() return true for parent and partitions. Maybe we can make >> it work by looking a bit harder than I did

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-19 Thread Alvaro Herrera
Amit Langote wrote: > Yeah, I too have wondered in the past what it would take to make > equalTupDescs() return true for parent and partitions. Maybe we can make > it work by looking a bit harder than I did then. How about simply relaxing the tdtypeid test from equalTupleDescs? I haven't

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-18 Thread Amit Langote
On 2018/04/18 22:40, Alvaro Herrera wrote: > Amit Langote wrote: >> On 2018/04/18 0:04, Alvaro Herrera wrote: >>> Amit Langote wrote: >>> I just confirmed my hunch that this wouldn't somehow do the right thing when the OID system column is involved. Like this case: >>> >>> This looks

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-18 Thread Alvaro Herrera
Amit Langote wrote: > On 2018/04/18 0:04, Alvaro Herrera wrote: > > Amit Langote wrote: > > > >> I just confirmed my hunch that this wouldn't somehow do the right thing > >> when the OID system column is involved. Like this case: > > > > This looks too big a patch to pursue now. I'm inclined

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-17 Thread Amit Langote
On 2018/04/18 0:04, Alvaro Herrera wrote: > Amit Langote wrote: > >> I just confirmed my hunch that this wouldn't somehow do the right thing >> when the OID system column is involved. Like this case: > > This looks too big a patch to pursue now. I'm inclined to just remove > the equalTupdesc

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-17 Thread Amit Langote
On 2018/04/18 0:02, Alvaro Herrera wrote: > Amit Langote wrote: > >> Attached find a patch that does that. When working on this, I noticed >> that when recursing for inheritance children, ATPrepAlterColumnType() >> would use a AlterTableCmd (cmd) that's already scribbled on as if it were >> the

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-17 Thread Alvaro Herrera
Amit Langote wrote: > I just confirmed my hunch that this wouldn't somehow do the right thing > when the OID system column is involved. Like this case: This looks too big a patch to pursue now. I'm inclined to just remove the equalTupdesc changes. -- Álvaro Herrera

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-17 Thread Alvaro Herrera
Amit Langote wrote: > Attached find a patch that does that. When working on this, I noticed > that when recursing for inheritance children, ATPrepAlterColumnType() > would use a AlterTableCmd (cmd) that's already scribbled on as if it were > the original. While I agree that the code here is in

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-17 Thread Amit Langote
On 2018/04/17 16:45, Amit Langote wrote: > Instead of doing this, I think we should try to make > convert_tuples_by_name_map() a bit smarter by integrating the logic in > convert_tuples_by_name() that's used conclude if no tuple conversion is > necessary. So, if it turns that the tuples

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-17 Thread Amit Langote
On 2018/04/17 4:10, Alvaro Herrera wrote: > Amit Langote wrote: > >> The solution I came up with is to call map_variable_attnos() directly, >> instead of going through map_partition_varattnos() every time, after first >> creating the attribute map ourselves. > > Yeah, sounds good. I added a

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-16 Thread Tom Lane
Alvaro Herrera writes: > Pushed now, thanks. Buildfarm doesn't like this even a little bit. regards, tom lane

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-16 Thread Alvaro Herrera
Amit Langote wrote: > The solution I came up with is to call map_variable_attnos() directly, > instead of going through map_partition_varattnos() every time, after first > creating the attribute map ourselves. Yeah, sounds good. I added a tweak: if the tupledescs are equal, there should be no

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-16 Thread Amit Langote
On 2018/04/10 11:56, Amit Langote wrote: > On 2018/03/27 13:27, Amit Langote wrote: >> On 2018/03/26 23:20, Alvaro Herrera wrote: >>> The one thing I wasn't terribly in love with is the four calls to >>> map_partition_varattnos(), creating the attribute map four times ... but >>> we already have

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-04-09 Thread Amit Langote
On 2018/03/27 13:27, Amit Langote wrote: > On 2018/03/26 23:20, Alvaro Herrera wrote: >> The one thing I wasn't terribly in love with is the four calls to >> map_partition_varattnos(), creating the attribute map four times ... but >> we already have it in the TupleConversionMap, no? Looks like we

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-26 Thread Amit Langote
On 2018/03/26 23:20, Alvaro Herrera wrote: > Pushed now. Thank you! > Amit Langote wrote: >> On 2018/03/24 9:23, Alvaro Herrera wrote: > >>> To fix this, I had to completely rework the "get partition parent root" >>> stuff into "get list of ancestors of this partition". >> >> I wondered if a

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-26 Thread Alvaro Herrera
Pushed now. Amit Langote wrote: > On 2018/03/24 9:23, Alvaro Herrera wrote: > > To fix this, I had to completely rework the "get partition parent root" > > stuff into "get list of ancestors of this partition". > > I wondered if a is_partition_ancestor(partrelid, ancestorid) isn't enough >

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-25 Thread Amit Langote
On 2018/03/24 9:23, Alvaro Herrera wrote: > I made a bunch of further edits and I think this v10 is ready to push. > Before doing so I'll give it a final look, particularly because of the > new elog(ERROR) I added. Post-commit review is of course always > appreciated. > > Most notable change is

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-23 Thread Alvaro Herrera
I made a bunch of further edits and I think this v10 is ready to push. Before doing so I'll give it a final look, particularly because of the new elog(ERROR) I added. Post-commit review is of course always appreciated. Most notable change is because I noticed that if you mention an intermediate

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-23 Thread Alvaro Herrera
Thanks for these changes. I'm going over this now, with intention to push it shortly. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-23 Thread Etsuro Fujita
(2018/03/22 18:31), Amit Langote wrote: On 2018/03/20 20:53, Etsuro Fujita wrote: Here are comments on executor changes in (the latest version of) the patch: @@ -421,8 +424,18 @@ ExecInsert(ModifyTableState *mtstate, ItemPointerData conflictTid; bool

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-22 Thread Amit Langote
On 2018/03/22 20:48, Pavan Deolasee wrote: > Thanks. It's looking much better now. Thanks. > I think we can possibly move all ON > CONFLICT related members to a separate structure and just copy the pointer > to the structure if (map == NULL). That might make the code a bit more tidy. OK, I

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-22 Thread Pavan Deolasee
On Thu, Mar 22, 2018 at 3:01 PM, Amit Langote wrote: > > > > > Why do we need to pin the descriptor? If we do need, why don't we need > > corresponding ReleaseTupDesc() call? > > PinTupleDesc was added in the patch as Alvaro had submitted it upthread, > which it

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-22 Thread Amit Langote
Fujita-san, Pavan, Thank you both for reviewing. Replying to both emails here. On 2018/03/20 20:53, Etsuro Fujita wrote: > Here are comments on executor changes in (the latest version of) the patch: > > @@ -421,8 +424,18 @@ ExecInsert(ModifyTableState *mtstate, > ItemPointerData

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-21 Thread Pavan Deolasee
On Tue, Mar 20, 2018 at 10:09 AM, Amit Langote < langote_amit...@lab.ntt.co.jp> wrote: > On 2018/03/20 13:30, Amit Langote wrote: > > I have incorporated your patch in the main patch after updating the > > comments a bit. Also, now that ee49f49 is in [1], the transition > > table related

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-20 Thread Etsuro Fujita
(2018/03/20 13:30), Amit Langote wrote: On 2018/03/19 21:59, Etsuro Fujita wrote: (2018/03/18 13:17), Alvaro Herrera wrote: Alvaro Herrera wrote: The only thing that I remain unhappy about this patch is the whole adjust_and_expand_partition_tlist() thing. I fear we may be doing redundant

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-19 Thread Amit Langote
On 2018/03/20 13:30, Amit Langote wrote: > I have incorporated your patch in the main patch after updating the > comments a bit. Also, now that ee49f49 is in [1], the transition > table related tests I proposed yesterday pass nicely. Instead of posting > as a separate patch, I have merged it

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-19 Thread Amit Langote
Fujita-san, On 2018/03/19 21:59, Etsuro Fujita wrote: > (2018/03/18 13:17), Alvaro Herrera wrote: >> Alvaro Herrera wrote: >> The only thing that I remain unhappy about this patch is the whole >> adjust_and_expand_partition_tlist() thing.  I fear we may be doing >> redundant and/or misplaced

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-19 Thread Etsuro Fujita
(2018/03/18 13:17), Alvaro Herrera wrote: Alvaro Herrera wrote: The only thing that I remain unhappy about this patch is the whole adjust_and_expand_partition_tlist() thing. I fear we may be doing redundant and/or misplaced work. I'll look into it next week. I'm still reviewing the patches,

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-19 Thread Amit Langote
On 2018/03/19 16:45, Amit Langote wrote: > I have tried to make these changes and attached are the updated patches > containing those, including the change I suggested for 0001 (that is, > getting rid of mt_onconflict). I also expanded some comments in 0003 > while making those changes. I

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-19 Thread Amit Langote
Thanks for the updated patches. On 2018/03/18 13:17, Alvaro Herrera wrote: > Alvaro Herrera wrote: > >> I think what I should be doing is the same as the returning stuff: keep >> a tupdesc around, and use a single slot, whose descriptor is changed >> just before the projection. > > Yes, this

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-19 Thread Amit Langote
On 2018/03/05 18:04, Pavan Deolasee wrote: > On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera > wrote: >> Re. the "ugly hack" comments in adjust_inherited_tlist(), I'm confused: >> expand_targetlist() runs *after*, not before, so how could it have >> affected the result?

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-17 Thread Alvaro Herrera
Alvaro Herrera wrote: > I think what I should be doing is the same as the returning stuff: keep > a tupdesc around, and use a single slot, whose descriptor is changed > just before the projection. Yes, this works, though it's ugly. Not any uglier than what's already there, though, so I think

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Alvaro Herrera
Andres Freund wrote: > Hi, > > On 2018-03-16 18:23:44 -0300, Alvaro Herrera wrote: > > Another thing I noticed is that the split of the ON CONFLICT slot and > > its corresponding projection is pretty weird. The projection is in > > ResultRelInfo, but the slot itself is in ModifyTableState. You

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Andres Freund
Hi, On 2018-03-16 18:23:44 -0300, Alvaro Herrera wrote: > Another thing I noticed is that the split of the ON CONFLICT slot and > its corresponding projection is pretty weird. The projection is in > ResultRelInfo, but the slot itself is in ModifyTableState. You can't > make the projection work

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Alvaro Herrera
Peter Geoghegan wrote: > On Fri, Mar 16, 2018 at 11:21 AM, Alvaro Herrera > wrote: > > So ExecInsert receives the ModifyTableState, and separately it receives > > arbiterIndexes and the OnConflictAction, both of which are members of > > the passed ModifyTableState. I

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Peter Geoghegan
On Fri, Mar 16, 2018 at 11:21 AM, Alvaro Herrera wrote: > So ExecInsert receives the ModifyTableState, and separately it receives > arbiterIndexes and the OnConflictAction, both of which are members of > the passed ModifyTableState. I wonder why does it do that; wouldn't

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Alvaro Herrera
So ExecInsert receives the ModifyTableState, and separately it receives arbiterIndexes and the OnConflictAction, both of which are members of the passed ModifyTableState. I wonder why does it do that; wouldn't it be simpler to extract those members from the node? With the patch proposed

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Alvaro Herrera
Pavan Deolasee wrote: > Hmm, yeah, probably you're right. The reason I got confused is because the > patch uses ri_onConflictSetProj/ri_onConflictSetWhere to store the > converted projection info/where clause for a given partition in its > ResultRelationInfo. So I was wondering if we can > move

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Pavan Deolasee
On Fri, Mar 16, 2018 at 5:13 PM, Etsuro Fujita wrote: > (2018/03/16 19:43), Pavan Deolasee wrote: > >> On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera > > wrote: >> > > @@ -106,6 +120,9 @@ typedef struct

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Etsuro Fujita
(2018/03/16 19:43), Pavan Deolasee wrote: On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera > wrote: @@ -106,6 +120,9 @@ typedef struct PartitionTupleRouting int num_subplan_partition_offsets; TupleTableSlot

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-16 Thread Pavan Deolasee
On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera wrote: > > @@ -106,6 +120,9 @@ typedef struct PartitionTupleRouting int num_subplan_partition_offsets; TupleTableSlot *partition_tuple_slot; TupleTableSlot *root_tuple_slot; + List

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-05 Thread Pavan Deolasee
On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera wrote: > > > Re. the "ugly hack" comments in adjust_inherited_tlist(), I'm confused: > expand_targetlist() runs *after*, not before, so how could it have > affected the result? > If I understand correctly, planner must

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-04 Thread Amit Langote
On 2018/03/03 0:36, Alvaro Herrera wrote: > Amit Langote wrote: > >> Actually, after your comment on my original patch [1], I did make it work >> for multiple levels by teaching the partition initialization code to find >> a given partition's indexes that are inherited from the root table (that

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-02 Thread Alvaro Herrera
Amit Langote wrote: > Actually, after your comment on my original patch [1], I did make it work > for multiple levels by teaching the partition initialization code to find > a given partition's indexes that are inherited from the root table (that > is the table mentioned in command). So, after a

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-03-02 Thread Alvaro Herrera
Amit Langote wrote: > Actually, after your comment on my original patch [1], I did make it work > for multiple levels by teaching the partition initialization code to find > a given partition's indexes that are inherited from the root table (that > is the table mentioned in command). So, after a

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-02-28 Thread Amit Langote
On 2018/03/01 1:03, Robert Haas wrote: > On Tue, Feb 27, 2018 at 7:46 PM, Alvaro Herrera > wrote: >> I updated Amit Langote's patch for INSERT ON CONFLICT DO UPDATE[1]. >> Following the lead of edd44738bc88 ("Be lazier about partition tuple >> routing.") this incarnation

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-02-28 Thread Robert Haas
On Tue, Feb 27, 2018 at 7:46 PM, Alvaro Herrera wrote: > I updated Amit Langote's patch for INSERT ON CONFLICT DO UPDATE[1]. > Following the lead of edd44738bc88 ("Be lazier about partition tuple > routing.") this incarnation only does the necessary push-ups for the >

Re: ON CONFLICT DO UPDATE for partitioned tables

2018-02-28 Thread Amit Langote
On 2018/02/28 9:46, Alvaro Herrera wrote: > I updated Amit Langote's patch for INSERT ON CONFLICT DO UPDATE[1]. > Following the lead of edd44738bc88 ("Be lazier about partition tuple > routing.") this incarnation only does the necessary push-ups for the > specific partition that needs it, at