From: | Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com> |
---|---|
To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
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.c |
Date: | 2020-11-11 19:49:57 |
Message-ID: | CAE-ML+-K2LqbQWqgJ8tbsS9win2Oxt9HwNRv3xd9hM7Jy-ZCmA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hey Heikki,
On Tue, Nov 3, 2020 at 1:30 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
Thanks for working on this refactor. LGTM! I had a few minor comments:
1. We should rename the CopyFormatOptions *cstate param in
ProcessCopyOptions() to CopyFormatOptions *options and List *options to
List *raw_options IMO, to make it more readable.
2. We need to update the header comment for Copy{From,To}StateData. It is
currently the old comment from CopyStateData.
3. Can we add docs for the missing fields in the header comment for
BeginCopyFrom()?
4.
> /*
> * Working state for COPY TO/FROM
> */
> MemoryContext copycontext; /* per-copy execution context */
Comment needs to be updated for the COPY operation.
5.
> 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..
}
6.
> /* create workspace for CopyReadAttributes results */
> if (!cstate->opts.binary)
Can we replace this if with an else?
Regards,
Soumyadeep (VMware)
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2020-11-11 19:52:56 | Re: Parallel bitmap index scan |
Previous Message | Bruce Momjian | 2020-11-11 19:44:47 | Re: Add docs stub for recovery.conf |