On 6/15/20 10:26 AM, Ashutosh Bapat wrote: > Thanks Andrey for the patch. I am glad that the patch has taken care > of some corner cases already but there exist still more. > > COPY command constructed doesn't take care of dropped columns. There > is code in deparseAnalyzeSql which constructs list of columns for a > given foreign relation. 0002 patch attached here, moves that code to a > separate function and reuses it for COPY. If you find that code change > useful please include it in the main patch.
Thanks, i included it.
> 2. In the same case, if the foreign table declared locally didn't have > any non-dropped columns but the relation that it referred to on the > foreign server had some non-dropped columns, COPY command fails. I > added a test case for this in 0002 but haven't fixed it.
I fixed it. This is very special corner case. The problem was that COPY FROM does not support semantics like the "INSERT INTO .. DEFAULT VALUES". To simplify the solution, i switched off bulk copying for this case.
> I think this work is useful. Please add it to the next commitfest so > that it's tracked. Ok.
It looks like we call BeginForeignInsert and EndForeignInsert even though actual copy is performed using BeginForeignCopy, ExecForeignCopy and EndForeignCopy. BeginForeignInsert constructs the INSERT query which looks unnecessary. Also some of the other PgFdwModifyState members are initialized unnecessarily. It also gives an impression that we are using INSERT underneath the copy. Instead a better way would be to call BeginForeignCopy instead of BeginForeignInsert and EndForeignCopy instead of EndForeignInsert, if we are going to use COPY protocol to copy data to the foreign server. Corresponding postgres_fdw implementations need to change in order to do that.
This isn't a full review. I will continue reviewing this patch further.