On Sat, Apr 3, 2021 at 7:16 PM Kazutaka Onishi <onishi@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