2020年1月21日(火) 15:38 Michael Paquier <mich...@paquier.xyz>:
>
> On Mon, Jan 20, 2020 at 10:50:21PM +0900, Michael Paquier wrote:
> > I have spent a good amount of time polishing 0001, tweaking the docs,
> > comments, error messages and a bit its logic. I am getting
> > comfortable with it, but it still needs an extra lookup, an indent run
> > which has some noise and I lacked of time today. 0002 has some of its
> > issues fixed and I have not reviewed it fully yet. There are still
> > some places not adapted in it (why do you use "Bug?" in all your
> > elog() messages by the way?), so the postgres_fdw part needs more
> > attention. Could you think about some docs for it by the way?
>
> I have more comments about the postgres_fdw that need to be
> addressed.
>
> + if (!OidIsValid(server_id))
> + {
> + server_id = GetForeignServerIdByRelId(frel_oid);
> + user = GetUserMapping(GetUserId(), server_id);
> + conn = GetConnection(user, false);
> + }
> + else if (server_id != GetForeignServerIdByRelId(frel_oid))
> + elog(ERROR, "Bug? inconsistent Server-IDs were supplied");
> I agree here that an elog() looks more adapted than an assert.
> However I would change the error message to be more like "incorrect
> server OID supplied by the TRUNCATE callback" or something similar.
> The server OID has to be valid anyway, so don't you just bypass any
> errors if it is not set?
>
> +-- truncate two tables at a command
> What does this sentence mean? Isn't that "truncate two tables in one
> single command"?
>
> The table names in the tests are rather hard to parse. I think that
> it would be better to avoid underscores at the beginning of the
> relation names.
>
> It would be nice to have some coverage with inheritance, and also
> track down in the tests what we expect when ONLY is specified in that
> case (with and without ONLY, both parent and child relations).
>
> Anyway, attached is the polished version for 0001 that I would be fine
> to commit, except for one point: are there objections if we do not
> have extra handling for ONLY when it comes to foreign tables with
> inheritance? As the patch stands, the list of relations is first
> built, with an inheritance recursive lookup done depending on if ONLY
> is used or not. Hence, if using "TRUNCATE ONLY foreign_tab, ONLY
> foreign_tab2", then only those two tables would be passed down to the
> FDW. If ONLY is removed, both tables as well as their children are
> added to the lists of relations split by server OID. One problem is
> that this could be confusing for some users I guess? For example,
> with a 1:1 mapping in the schema of the local and remote servers, a
> user asking for TRUNCATE ONLY foreign_tab would pass down to the
> remote just the equivalent of "TRUNCATE foreign_tab" using
> postgres_fdw, causing the full inheritance tree to be truncated on the
> remote side. The concept of ONLY mixed with inherited foreign tables
> is rather blurry (point raised by Stephen upthread).
>
Hmm. Do we need to deliver another list to inform why these relations are
trundated?
If callback is invoked with a foreign-relation that is specified by TRUNCATE
command with ONLY, it seems to me reasonable that remote TRUNCATE
command specifies the relation on behalf of the foreign table with ONLY.
Foreign-tables can be truncated because ...
1. it is specified by user with ONLY-clause.
2. it is specified by user without ONLY-clause.
3. it is inherited child of the relations specified at 2.
4. it depends on the relations picked up at 1-3.
So, if ExecForeignTruncate() has another list to inform the context for each
relation, postgres_fdw can build proper remote query that may specify the
remote tables with ONLY-clause.
Regarding to the other comments, it's all Ok for me. I'll update the patch.
And, I forgot "updatable" option at postgres_fdw. It should be checked on
the truncate also, right?
Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kai...@heterodb.com>
