On Tue, Apr 6, 2021 at 5:36 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> Thank you for checking v13, and here is v14 patch.
>
> > 1) Are we using all of these macros? I see that we are setting them
> > but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> > them?
>
> These may be needed for the foreign data handler other than postgres_fdw.
I'm not sure about this, but if it's discussed upthread and agreed
upon, I'm fine with it.
> > 4) I have a basic question: If I have a truncate statement with a mix
> > of local and foreign tables, IIUC, the patch is dividing up a single
> > truncate statement into two truncate local tables, truncate foreign
> > tables. Is this transaction safe at all?
>
> According to this discussion, we can revert both tables in the local
> and the server.
> https://www.postgresql.org/message-id/CAOP8fzbuJ5GdKa%2B%3DGtizbqFtO2xsQbn4mVjjzunmsNVJMChSMQ%40mail.gmail.com
On giving more thought on this, it looks like we are safe i.e. local
truncation will get reverted. Because if an error occurs on foreign
table truncation, the control in the local server would go to
pgfdw_report_error which generates an error in the local server which
aborts the local transaction and so the local table truncations would
get reverted.
+ /* 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);
I still feel that the above bunch of code is duplicate of what
do_sql_command function already has. I would recommend that we just
make that function non-static(it's easy to do) and keep the
declaration in postgres_fdw.h and use it in the
postgresExecForeignTruncate.
Another minor comment:
We could move + ForeignServer *serv = NULL; within foreach (lc,
frels_list), because it's not being used outside.
The v14 patch mostly looks good to me other than the above comments.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com