Re: BUG #15552: Unexpected error in COPY to a foreign table in a transaction

Hi,

On 2018/12/18 22:41, Luis Carril wrote:
> Hi everyone,
>
> thanks for the input and the discussion, attached you can find the patch along a TAP test as Michael suggested.
>
> Would you like something else there?

Thanks for updating the patch. Some comments:

diff --git a/contrib/postgres_fdw/t/001_copy_same_txn_rem.pl
b/contrib/postgres_fdw/t/001_copy_same_txn_rem.pl

Not sure that's a good name to give to the test file, although since I'm
not greatly enthusiastic about adding a TAP test for this, I don't have a
good suggestion. Maybe, choose a more generic name? Michael, any
suggestions?

+ CREATE SERVER testserver FOREIGN DATA WRAPPER postgres_fdw;
...
+ CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;

These two lines are not really necessary.

@@ -2401,6 +2401,7 @@ CopyFrom(CopyState cstate)
^I */
^I/* createSubid is creation check, newRelfilenodeSubid is truncation
check */
^Iif (cstate->rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE &&
+^I cstate->rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
^I^I(cstate->rel->rd_createSubid != InvalidSubTransactionId ||
^I^I cstate->rel->rd_newRelfilenodeSubid != InvalidSubTransactionId))
^I{

It seems you've used spaces to indent the newly added line. Please,
change that to a tab like neighboring lines.

> Another question about the procedure: should I submit the patch directly to the commitfest or should it be published first in psql-hacking?

Generally, it makes sense to create a CF entry *after* you've sent an
email containing the patch, so that you can link to it immediately from
the entry. Since you've posted the patch here, you can create the CF
entry under the topic Bug Fixes. You'll need a PostgreSQL community
account to do that.

Thanks,
Amit