From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: PostgreSQL-Dev <pgsql-hackers@postgresql.org>
Subject: Re: [POC] Fast COPY FROM command for the table with foreign partitions
Date: Tue, 08 Sep 2020 12:29:14 +0300
Message-ID: <0c67618c2e43b1dccb260dd6f90eeac2@postgrespro.ru> (raw)
In-Reply-To: <CA+HiwqEoSMpzMhzz1Dv7qFsFtzG71t5Wrgy4F+MoVUVR_5HdDQ@mail.gmail.com>
References: <ae25e79e-6122-720e-1349-fb832c6021df@postgrespro.ru>
<CA+HiwqH2ehGurL9bYUydXBxVUH6aHN5yXXxESAOs9O6-f_+Qeg@mail.gmail.com>
<cf1e92eb-9a8e-f5c1-f0cc-598882f1e546@postgrespro.ru>
<CA+HiwqHRqG8X9W6-v-E6d0nw1k6aC7QSMk-SOvv4puOjWsQMmA@mail.gmail.com>
<9a65d03b-250f-b086-5a3c-541d8e2de30d@postgrespro.ru>
<CA+HiwqE6M4mwH=CDOKmy5DUHRDP-knLdAfmCh+GW7D59s3QDRg@mail.gmail.com>
<CA+HiwqHtQaZp_t5M3z1+M0T9zd=0s3r_oOmi9TxHDdyW0_cxvg@mail.gmail.com>
<ab62d228-cf9a-e517-8667-7df1184adf2a@postgrespro.ru>
<CA+HiwqG1o5BDi0Hx6Uoxteyra2FbiSvaN4Fzs-i0pfKo1Zp_fg@mail.gmail.com>
<CA+HiwqHT9UFhVHyKsavL5u=iVdW9k92pHHN=FYL0sEysGtvYsw@mail.gmail.com>
<20200907072651.GF4867@paquier.xyz>
<19ba27e7-ee1e-9117-68ad-fd609b5763ca@postgrespro.ru>
<CA+HiwqEoSMpzMhzz1Dv7qFsFtzG71t5Wrgy4F+MoVUVR_5HdDQ@mail.gmail.com>
Hi,
I've started doing a review of v7 yesterday.
On 2020-09-08 10:34, Amit Langote wrote:
> On Mon, Sep 7, 2020 at 7:31 PM Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> >
>> v.7 (in attachment) fixes this problem.
>> I also accepted Amit's suggestion to rename all fdwapi routines such
>> as
>> ForeignCopyIn to *ForeignCopy.
>
It seems that naming is quite inconsistent now:
+ /* COPY a bulk of tuples into a foreign relation */
+ BeginForeignCopyIn_function BeginForeignCopy;
+ EndForeignCopyIn_function EndForeignCopy;
+ ExecForeignCopyIn_function ExecForeignCopy;
You get rid of this 'In' in the function names, but the types are still
with it:
+typedef void (*BeginForeignCopyIn_function) (ModifyTableState *mtstate,
+ ResultRelInfo *rinfo);
+
+typedef void (*EndForeignCopyIn_function) (EState *estate,
+ ResultRelInfo *rinfo);
+
+typedef void (*ExecForeignCopyIn_function) (ResultRelInfo *rinfo,
+ TupleTableSlot **slots,
+ int nslots);
Also docs refer to old function names:
+void
+BeginForeignCopyIn(ModifyTableState *mtstate,
+ ResultRelInfo *rinfo);
I think that it'd be better to choose either of these two naming schemes
and use it everywhere for consistency.
>
> Any thoughts on the taking out the refactoring changes out of the main
> patch as I suggested?
>
+1 for splitting the patch. It was rather difficult for me to
distinguish changes required by COPY via postgres_fdw from this
refactoring.
Another ambiguous part of the refactoring was in changing
InitResultRelInfo() arguments:
@@ -1278,6 +1280,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
Relation partition_root,
+ bool use_multi_insert,
int instrument_options)
Why do we need to pass this use_multi_insert flag here? Would it be
better to set resultRelInfo->ri_usesMultiInsert in the
InitResultRelInfo() unconditionally like it is done for
ri_usesFdwDirectModify? And after that it will be up to the caller
whether to use multi-insert or not based on their own circumstances.
Otherwise now we have a flag to indicate that we want to check for
another flag, while this check doesn't look costly.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, amitlangote09@gmail.com, a.lepikhov@postgrespro.ru, michael@paquier.xyz, ashutosh.bapat@2ndquadrant.com, ashutosh.bapat.oss@gmail.com, etsuro.fujita@gmail.com
Subject: Re: [POC] Fast COPY FROM command for the table with foreign partitions
In-Reply-To: <0c67618c2e43b1dccb260dd6f90eeac2@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox