On Wed, Jan 15, 2020 at 11:33:07PM +0900, Kohei KaiGai wrote:
> 2020年1月15日(水) 17:11 Michael Paquier <michael@paquier.xyz>:
>> 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' ?
It sounds to me that this message is kind of right. This FDW "dummy"
does not have any FDW handler at all, so it complains about it.
Having no support for TRUNCATE is something that may happen after
that. Actually, this error message from your patch used for a FDW
which has a handler but no TRUNCATE support could be reworked:
+ if (!fdwroutine->ExecForeignTruncate)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not a supported foreign table",
+ relname)));
Something like "Foreign-data wrapper \"%s\" does not support
TRUNCATE"?
>> + <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.
Thanks. It would not matter much for relations without inheritance
children, but if truncating a partition tree with many foreign tables
using various FDWs that could matter performance-wise.
--
Michael