From: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
---|---|
To: | Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com> |
Cc: | Erikjan Rijkers <er(at)xs4all(dot)nl>, David Rowley <dgrowleyml(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, vignesh C <vignesh21(at)gmail(dot)com> |
Subject: | Re: Split copy. |
Date: | 2020-11-17 10:38:44 |
Message-ID: | b406934b-6081-c6a7-56e9-034130fb8946@iki.fi |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for feedback, attached is a new patch version.
On 11/11/2020 21:49, Soumyadeep Chakraborty wrote:
> On Tue, Nov 3, 2020 at 1:30 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> I also split/duplicated the CopyStateData struct into CopyFromStateData
>> and CopyToStateData. Many of the fields were common, but many were not,
>> and I think some duplication is nicer than a struct where you use some
>> fields and others are unused. I put the common formatting options into a
>> new CopyFormatOptions struct.
>
> Would we be better off if we sub-struct CopyState <- Copy{From,To}State?
> Like this:
> typedef struct Copy{From|To}StateData
> {
> CopyState cs;
> // Fields specific to COPY FROM/TO follow..
> }
Hmm. I don't think that would be better. There isn't actually that much
in common between CopyFromStateData and CopyToStateData, and a little
bit of duplication seems better.
> 6.
>
>> /* create workspace for CopyReadAttributes results */
>> if (!cstate->opts.binary)
>
> Can we replace this if with an else?
Seems better as it is IMO, the if- and else-branches are not really
related to each other, even though they both happen to be conditioned on
cstate->opts.binary.
Fixed all the other things you listed, fixed a bug in setting
'file_encoding', and trimmed down the #includes.
- Heikki
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Split-copy.c-into-three-files.patch | text/x-patch | 293.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2020-11-17 10:58:42 | Re: Heads-up: macOS Big Sur upgrade breaks EDB PostgreSQL installations |
Previous Message | Etsuro Fujita | 2020-11-17 09:56:02 | Re: Asynchronous Append on postgres_fdw nodes. |