Re: TRUNCATE on foreign tables

2020年1月15日(水) 17:11 Michael Paquier <michael@paquier.xyz>:
>
> 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.
>
What error message is better in this case? It does not print "ft2" anywhere,
so user may not notice that "ft2" is the source of the error.
How about 'foreign table "ft2" does not support truncate' ?

> +void
> +deparseTruncateSql(StringInfo buf, Relation rel)
> +{
> +   deparseRelation(buf, rel);
> +}
> Don't see much point in having this routine.
>
deparseRelation() is a static function in postgres_fdw/deparse.c
On the other hand, it may be better to move entire logic to construct
remote TRUNCATE command in the deparse.c side like other commands.

> +     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"?
>
Yes.
If FDW does not provide this callback, PostgreSQL performs as if TRUNCATE
is not supported on the foreign table.

> +     <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?
>
It's just a matter of preference. A temporary hash-table with server-id
and list of foreign-tables is an idea. Let me try to revise.

> +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.
>
Ok,

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>