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