Hi Andrey, On Fri, Aug 21, 2020 at 9:19 PM Andrey Lepikhov <a.lepik...@postgrespro.ru> wrote: > On 8/7/20 2:14 PM, Amit Langote wrote: > > I was playing around with v5 and I noticed an assertion failure which > > I concluded is due to improper setting of ri_usesBulkModify. You can > > reproduce it with these steps. > > > > create extension postgres_fdw; > > create server lb foreign data wrapper postgres_fdw ; > > create user mapping for current_user server lb; > > create table foo (a int, b int) partition by list (a); > > create table foo1 (like foo); > > create foreign table ffoo1 partition of foo for values in (1) server > > lb options (table_name 'foo1'); > > create table foo2 (like foo); > > create foreign table ffoo2 partition of foo for values in (2) server > > lb options (table_name 'foo2'); > > create function print_new_row() returns trigger language plpgsql as $$ > > begin raise notice '%', new; return new; end; $$; > > create trigger ffoo1_br_trig before insert on ffoo1 for each row > > execute function print_new_row(); > > copy foo from stdin csv; > > Enter data to be copied followed by a newline. > > End with a backslash and a period on a line by itself, or an EOF signal. > >>> 1,2 > >>> 2,3 > >>> \. > > NOTICE: (1,2) > > server closed the connection unexpectedly > > This probably means the server terminated abnormally > > before or while processing the request. > > Thnx, I added TAP-test on this problem> However instead of duplicating > the same logic to do so in two places
Good call. > > (CopyFrom and ExecInitPartitionInfo), I think it might be a good idea > > to refactor the code to decide if multi-insert mode can be used for a > > given relation by checking its properties and put it in some place > > that both the main target relation and partitions need to invoke. > > InitResultRelInfo() seems to be one such place. > +1 > > > > Also, it might be a good idea to use ri_usesBulkModify more generally > > than only for foreign relations as the patch currently does, because I > > can see that it can replace the variable insertMethod in CopyFrom(). > > Having both insertMethod and ri_usesBulkModify in each ResultRelInfo > > seems confusing and bug-prone. > > > > Finally, I suggest renaming ri_usesBulkModify to ri_usesMultiInsert to > > reflect its scope. > > > > Please check the attached delta patch that applies on top of v5 to see > > what that would look like. > > I merged your delta patch (see v6 in attachment) to the main patch. > Currently it seems more commitable than before. Thanks for accepting the changes. Actually, I was thinking maybe making the patch to replace CopyInsertMethod enum by ri_usesMultiInsert separate from the rest might be better as I can see it as independent refactoring. Attached is how the division would look like. I would -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
v6-0001-Move-multi-insert-decision-logic-into-executor.patch
Description: Binary data
v6-0002-Fast-COPY-FROM-into-the-foreign-or-sharded-table.patch
Description: Binary data