Re: TRUNCATE on foreign tables

--1bhFRg6vL9BT9osV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 part=
icular
> foreign server, thus, the server-id pulled out by foreign tables id shoul=
d be
> identical for all the relations in the list.
> Due to the API design, this callback shall be invoked for each foreign se=
rver
> involved in the TRUNCATE command, not per table basis.
>=20
> 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.=20

+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 =3D lfirst(lc);
+           Oid         frel_oid =3D RelationGetRelid(frel);
+
+           if (server_id =3D=3D GetForeignServerIdByRelId(frel_oid))
+           {
+               frels_list =3D foreach_delete_current(frels_list, lc);
+               curr_frels =3D 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

--1bhFRg6vL9BT9osV
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4eyS4ACgkQnvQgOdby
QH0V9hAAnmqa/HQL7cfD1xSa6TODooQhN9PY4f+V8UiH7fTAGdOrLGgoYgw2iLOg
3T4wtZQYm3MJFHOsPlCezVpJOs/oG5gPPOfKeR5Yxngroqqs8RDEUoO+3n1cMEC6
2jCiblslBWsGocF5j/EwNvnO1eQclP928OngRjGGIyU+E5Zmz+f2qDDS7SFsObwI
6r3DEV3yJOkPyTqSXf4NLCRSpqPxLr/xlwIR4ivBOvQYdojATvOEbqc1lv2JX5JB
KLklz5fBhrDw0f4RE8UYSLzzc6csyO5W/4nm6ZZMGVJNv1XGJTqX1zBfWMxDeF/m
H64UFqIAgyxYQAm0dhn61vEsY3Os2ShkrobKKgRM8sYNlHr+sZPo2nll1mhzdyBO
kmiKWi3YRlKPlIkBpjLklLiWjHIbSeDy8B8oy4Bffp8iyS1DTbFNDhiQb9E1u3oP
Nc3fnYs4k/8A4ft1KHmpIQvIEUVQ0l9ExYY1pW6WEn1WhtkDmcasReJhQo+XxVH3
04IkywEpMIlqbXA0h4OdQX1KuOhfq7psrKCqU4bDJE3p4tasJ6BAUfYLPkW++LBm
yjf5Ve6V5Qw50T0ZI0tKz9viCk7Gni4GU9YOdkREx1L882ACTGFDx4KWBBwkAYMK
vBcGMw3HPpjpJGc9XO2TxPyzdnwPSJmnL5yD0/D+YqYl8W3ELAE=
=RyUl
-----END PGP SIGNATURE-----

--1bhFRg6vL9BT9osV--