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).
--
Michael