Re: Parallel copy

Thanks Ashutosh for your comments.

On Wed, Sep 16, 2020 at 6:36 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Vignesh,
>
> I've spent some time today looking at your new set of patches and I've
> some thoughts and queries which I would like to put here:
>
> Why are these not part of the shared cstate structure?
>
>     SerializeString(pcxt, PARALLEL_COPY_KEY_NULL_PRINT, cstate->null_print);
>     SerializeString(pcxt, PARALLEL_COPY_KEY_DELIM, cstate->delim);
>     SerializeString(pcxt, PARALLEL_COPY_KEY_QUOTE, cstate->quote);
>     SerializeString(pcxt, PARALLEL_COPY_KEY_ESCAPE, cstate->escape);
>

I have used shared_cstate mainly to share the integer & bool data
types from the leader to worker process. The above data types are of
char* data type, I will not be able to use it like how I could do it
for integer type. So I preferred to send these as separate keys to the
worker. Thoughts?

> I think in the refactoring patch we could replace all the cstate
> variables that would be shared between the leader and workers with a
> common structure which would be used even for a serial copy. Thoughts?
>

Currently we are using shared_cstate only to share integer & bool data
types from leader to worker. Once worker retrieves the shared data for
integer & bool data types, worker will copy it to cstate. I preferred
this way because only for integer & bool we retrieve to shared_cstate
& copy it to cstate and for rest of the members any way we are
directly copying back to cstate. Thoughts?

> Have you tested your patch when encoding conversion is needed? If so,
> could you please point out the email that has the test results.
>

We have not yet done encoding testing, we will do and post the results
separately in the coming days.

> Apart from above, I've noticed some cosmetic errors which I am sharing here:
>
> +#define    IsParallelCopy()        (cstate->is_parallel)
> +#define IsLeader()             (cstate->pcdata->is_leader)
>
> This doesn't look to be properly aligned.
>

Fixed.

> +   shared_info_ptr = (ParallelCopyShmInfo *)
> shm_toc_allocate(pcxt->toc, sizeof(ParallelCopyShmInfo));
> +   PopulateParallelCopyShmInfo(shared_info_ptr, full_transaction_id);
>
> ..
>
> +   /* Store shared build state, for which we reserved space. */
> +   shared_cstate = (SerializedParallelCopyState
> *)shm_toc_allocate(pcxt->toc, est_cstateshared);
>
> In the first case, while typecasting you've added a space between the
> typename and the function but that is missing in the second case. I
> think it would be good if you could make it consistent.
>

Fixed

> Same comment applies here as well:
>
> +   pg_atomic_uint32    line_state;     /* line state */
> +   uint64              cur_lineno;     /* line number for error messages */
> +}ParallelCopyLineBoundary;
>
> ...
>
> +   CommandId                   mycid;  /* command id */
> +   ParallelCopyLineBoundaries  line_boundaries; /* line array */
> +} ParallelCopyShmInfo;
>
> There is no space between the closing brace and the structure name in
> the first case but it is in the second one. So, again this doesn't
> look consistent.
>

Fixed

> I could also find this type of inconsistency in comments. See below:
>
> +/* It can hold upto 10000 record information for worker to process. RINGSIZE
> + * should be a multiple of WORKER_CHUNK_COUNT, as wrap around cases
> is currently
> + * not handled while selecting the WORKER_CHUNK_COUNT by the worker. */
> +#define RINGSIZE (10 * 1000)
>
> ...
>
> +/*
> + * Each worker will be allocated WORKER_CHUNK_COUNT of records from DSM data
> + * block to process to avoid lock contention. Read RINGSIZE comments before
> + * changing this value.
> + */
> +#define WORKER_CHUNK_COUNT 50
>
> You may see these kinds of errors at other places as well if you scan
> through your patch.

Fixed.

Please find the attached v5 patch which has the fixes for the same.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com