Re: TRUNCATE on foreign tables

On Tue, Jan 14, 2020 at 06:16:17PM +0900, Kohei KaiGai wrote:
> The "frels_list" is a list of foreign tables that are connected to a particular
> foreign server, thus, the server-id pulled out by foreign tables id should be
> identical for all the relations in the list.
> Due to the API design, this callback shall be invoked for each foreign server
> involved in the TRUNCATE command, not per table basis.
>
> The 2nd and 3rd arguments also informs FDW driver other options of the
> command. If FDW has a concept of "cascaded truncate" or "restart sequence",
> it can adjust its remote query. In postgres_fdw, it follows the manner of
> usual TRUNCATE command.

I have done a quick read through the patch. You have modified the
patch to pass down to the callback a list of relation OIDs to execute
one command for all, and there are tests for FKs so that coverage
looks fine.

Regression tests are failing with this patch:
-- TRUNCATE doesn't work on foreign tables, either directly or
recursively
TRUNCATE ft2; -- ERROR
-ERROR: "ft2" is not a table
+ERROR: foreign-data wrapper "dummy" has no handler
You visibly just need to update the output because no handlers are
available for truncate in this case.

+void
+deparseTruncateSql(StringInfo buf, Relation rel)
+{
+ deparseRelation(buf, rel);
+}
Don't see much point in having this routine.

+ If FDW does not provide this callback, PostgreSQL considers
+ <command>TRUNCATE</command> is not supported on the foreign table.
+ </para>
This sentence is weird. Perhaps you meant "as not supported"?

+ <literal>frels_list</literal> is a list of foreign tables that are
+ connected to a particular foreign server; thus, these foreign tables
+ should have identical foreign server ID
The list is built by the backend code, so that has to be true.

+ foreach (lc, frels_list)
+ {
+ Relation frel = lfirst(lc);
+ Oid frel_oid = RelationGetRelid(frel);
+
+ if (server_id == GetForeignServerIdByRelId(frel_oid))
+ {
+ frels_list = foreach_delete_current(frels_list, lc);
+ curr_frels = lappend(curr_frels, frel);
+ }
+ }
Wouldn't it be better to fill in a hash table for each server with a
list of relations?

+typedef void (*ExecForeignTruncate_function) (List *frels_list,
+ bool is_cascade,
+ bool restart_seqs);
I would recommend to pass down directly DropBehavior instead of a
boolean to the callback. That's more extensible.
--
Michael