On Sat, Apr 3, 2021 at 7:16 PM Kazutaka Onishi <oni...@heterodb.com> wrote: > > Sorry but I found the v7 patch has typo and it can't be built... > I attached fixed one(v8).
Thanks for the patch. Here are some comments on v8 patch: 1) We usually have the struct name after "+typedef struct ForeignTruncateInfo", please refer to other struct defs in the code base. 2) We should add ORDER BY clause(probably ORDER BY id?) for data generating select queries in added tests, otherwise tests might become unstable. 3) How about dropping the tables, foreign tables that got created for testing in postgres_fdw.sql? 4) I think it's not "foreign-tables"/"foreign-table", it can be "foreign tables"/"foreign table", other places in the docs use this convention. + the context where the foreign-tables are truncated. It is a list of integers and same length with 5) Can't we use do_sql_command function after making it non static? We could go extra mile, that is we could make do_sql_command little more generic by passing some enum for each of PQsendQuery, PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace the respective code chunks with do_sql_command in postgres_fdw.c. + /* run remote query */ + if (!PQsendQuery(conn, sql.data)) + pgfdw_report_error(ERROR, NULL, conn, false, sql.data); + res = pgfdw_get_result(conn, sql.data); + if (PQresultStatus(res) != PGRES_COMMAND_OK) + pgfdw_report_error(ERROR, res, conn, true, sql.data); + /* clean-up */ + PQclear(res); 6) A white space error when the patch is applied. contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace. + 7) I may be missing something here. Why do we need a hash table at all? We could just do it with a linked list right? Is there a specific reason to use a hash table? IIUC, the hash table entries will be lying around until the local session exists since we are not doing hash_destroy. 8) How about having something like this? + <command>TRUNCATE</command> can be used for foreign tables if the foreign data wrapper supports, for instance, see <xref linkend="postgres-fdw"/>. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
