Re: [POC] Fast COPY FROM command for the table with foreign partitions - Mailing list pgsql-hackers

On 29.12.2020 16:20, Hou, Zhijie wrote:
>> see new version in attachment.
> 
> I took a look into the patch, and have some comments.
> 
> 1.
> +    PG_FINALLY();
> +    {
> +        copy_fmstate = NULL; /* Detect problems */
> I don't quite understand this comment,
> does it means we want to detect something like Null reference ?
> 
> 
> 2.
> +    PG_FINALLY();
> +    {
>     ...
> +        if (!OK)
> +            PG_RE_THROW();
> +    }
> Is this PG_RE_THROW() necessary ?
> IMO, PG_FINALLY will reproduce the PG_RE_THROW action if we get to the code block due to an error being thrown.

This is a debugging stage atavisms. fixed.
> 
> 3.
> +            ereport(ERROR,
> +                    (errmsg("unexpected extra results during COPY of table: %s",
> +                            PQerrorMessage(conn))));
> 
> I found some similar message like the following:
> 
>             pg_log_warning("unexpected extra results during COPY of table \"%s\"",
>                            tocEntryTag);
> How about using existing messages style ?

This style is intended for use in frontend utilities, not for contrib 
extensions, i think.
> 
> 4.
> I noticed some not standard code comment[1].
> I think it's better to comment like:
> /*
>   * line 1
>   * line 2
>   */
> 
> [1]-----------
> +        /* Finish COPY IN protocol. It is needed to do after successful copy or
> +         * after an error.
> +         */
> 
> 
> +/*
> + *
> + * postgresExecForeignCopy
> 
> +/*
> + *
> + * postgresBeginForeignCopy
Thanks, fixed.
The patch in attachment rebased on 107a2d4204.


-- 
regards,
Andrey Lepikhov
Postgres Professional