Hi

> 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.

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 ?

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

-----------
Best regards,
Houzj



Reply via email to