Hi Andrey,
On Fri, Aug 21, 2020 at 9:19 PM Andrey Lepikhov
<a.lepikhov@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
