Re: ON CONFLICT DO UPDATE for partitioned tables

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. >> >> H

Re: ON CONFLICT DO UPDATE for partitioned tables

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 fro

Re: ON CONFLICT DO UPDATE for partitioned tables

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 not both are >> RECORDOID might be sufficient, for

Re: ON CONFLICT DO UPDATE for partitioned tables

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 then. > > How about simply rel

Re: ON CONFLICT DO UPDATE for partitioned tables

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 looked

Re: ON CONFLICT DO UPDATE for partitioned tables

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 too

Re: ON CONFLICT DO UPDATE for partitioned tables

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 to

Re: ON CONFLICT DO UPDATE for partitioned tables

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 ch

Re: ON CONFLICT DO UPDATE for partitioned tables

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 o

Re: ON CONFLICT DO UPDATE for partitioned tables

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 Herrerahttps://www.

Re: ON CONFLICT DO UPDATE for partitioned tables

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 p

Re: ON CONFLICT DO UPDATE for partitioned tables

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 descriptor

Re: ON CONFLICT DO UPDATE for partitioned tables

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 twea

Re: ON CONFLICT DO UPDATE for partitioned tables

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

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 ne

Re: ON CONFLICT DO UPDATE for partitioned tables

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 it

Re: ON CONFLICT DO UPDATE for partitioned tables

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

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 is_

Re: ON CONFLICT DO UPDATE for partitioned tables

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 > instea

Re: ON CONFLICT DO UPDATE for partitioned tables

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 b

Re: ON CONFLICT DO UPDATE for partitioned tables

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 p

Re: ON CONFLICT DO UPDATE for partitioned tables

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/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; boolspecConfl

Re: ON CONFLICT DO UPDATE for partitioned tables

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 tried

Re: ON CONFLICT DO UPDATE for partitioned tables

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 wasn't clear to me either why it w

Re: ON CONFLICT DO UPDATE for partitioned tables

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 co

Re: ON CONFLICT DO UPDATE for partitioned tables

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 test

Re: ON CONFLICT DO UPDATE for partitioned tables

(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 and/o

Re: ON CONFLICT DO UPDATE for partitioned tables

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

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 work.

Re: ON CONFLICT DO UPDATE for partitioned tables

(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, b

Re: ON CONFLICT DO UPDATE for partitioned tables

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 realize

Re: ON CONFLICT DO UPDATE for partitioned tables

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 wor

Re: ON CONFLICT DO UPDATE for partitioned tables

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? >> > > If I understand corr

Re: ON CONFLICT DO UPDATE for partitioned tables

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 it'

Re: ON CONFLICT DO UPDATE for partitioned tables

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 c

Re: ON CONFLICT DO UPDATE for partitioned tables

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 w

Re: ON CONFLICT DO UPDATE for partitioned tables

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 without a corresponding slot initialized with the correct descript

Re: ON CONFLICT DO UPDATE for partitioned tables

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 wonder why does it do that; w

Re: ON CONFLICT DO UPDATE for partitioned tables

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 it > be simpler to extrac

Re: ON CONFLICT DO UPDATE for partitioned tables

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 upthread,

Re: ON CONFLICT DO UPDATE for partitioned tables

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 mt

Re: ON CONFLICT DO UPDATE for partitioned tables

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 PartitionTupleRouting >> int num_subplan_partiti

Re: ON CONFLICT DO UPDATE for partitioned tables

(2018/03/16 19:43), Pavan Deolasee wrote: On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera mailto:alvhe...@2ndquadrant.com>> wrote: @@ -106,6 +120,9 @@ typedef struct PartitionTupleRouting int num_subplan_partition_offsets; TupleTableSlot *partition_tuple_slot; TupleTableS

Re: ON CONFLICT DO UPDATE for partitioned tables

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 **partition_arbiter_indexes; + TupleTabl

Re: ON CONFLICT DO UPDATE for partitioned tables

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 have called expand_targetlist(

Re: ON CONFLICT DO UPDATE for partitioned tables

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

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

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

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 only does the necessary pu

Re: ON CONFLICT DO UPDATE for partitioned tables

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 > specific partition that needs

Re: ON CONFLICT DO UPDATE for partitioned tables

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 execut

ON CONFLICT DO UPDATE for partitioned tables

s idea, yet. Anyway, while this is still WIP, I think it works correctly for the case where there is a single partition level. [1] https://postgr.es/m/c1651d5b-7bd6-b7e7-e1cc-16ecfe2c0...@lab.ntt.co.jp -- Álvaro Herrer >From a2517bd315034de7f6c5a4728f66729136918e88 Mon Sep 17 00:00:00 2001