Hey Heikki,
On Tue, Nov 3, 2020 at 1:30 AM Heikki Linnakangas <hlinnaka@iki.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)
