On Mon, Jan 20, 2020 at 11:30:34AM +0900, Kohei KaiGai wrote:
> Sorry, it was a midnight job on Friday.
Should I be, err, worried about that? ;)
> Please check the attached patch.
+ switch (behavior)
+ {
+ case DROP_RESTRICT:
+ appendStringInfoString(buf, " RESTRICT");
+ break;
+ case DROP_CASCADE:
+ appendStringInfoString(buf, " CASCADE");
+ break;
+ default:
+ elog(ERROR, "Bug? unexpected DropBehavior (%d)",
(int)behavior);
+ break;
+ }
Here, you can actually remove the default clause. By doing so,
compilation would generate a warning if a new value is added to
DropBehavior if it is not listed. So anybody adding a new value to
the enum will need to think about this code path.
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("foreign data wrapper \"%s\" on behalf of the
foreign table \"%s\" does not support TRUNCATE",
+ fdw->fdwname, relname)));
I see two problems here:
- The error message is too complicated. I would just use "cannot
truncate foreign table \"%s\"".
- The error code should be ERRCODE_FEATURE_NOT_SUPPORTED.
The docs for the FDW description can be improved. I found that a
large portion of it used rather unclear English, and that things were
not clear enough regarding the use of a list of relations, when an
error is raised because ExecForeignTruncate is NULL, etc. I have also
cut the last paragraph which is actually implementation-specific
(think for example about callbacks at xact commit/abort time).
Documentation needs to be added to postgres_fdw about the truncation
support. Particularly, providing details about the possibility to do
truncates in our shot for a set of relations so as dependencies are
automatically handled is an advantage to mention.
There is no need to include the truncate routine in
ForeignTruncateInfo, as the server OID can be used to find it.
Another thing is that I would prefer splitting the patch into two
separate commits, so attached are two patches:
- 0001 for the addition of the in-core API
- 0002 for the addition of support in postgres_fdw.
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?
--
Michael