Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

[Amit Langote]

On 2019/01/08 11:10, Michael Paquier wrote:
> On Mon, Jan 07, 2019 at 05:28:27PM +0900, Amit Langote wrote:
>> On 2019/01/07 16:35, Michael Paquier wrote:
>>> It seems to me that we may want something more like:
>>> Primary: "could not use \"%s.%s\" as logical replication target".
>>> Detail: "Relation %s.%s is a foreign table", "not a table", etc.
>>
>> I've thought about that before and I tend to agree with you.  Maybe:
>>
>> ERROR: cannot use "%s.%s" as logical replication target
>> DETAIL: Using partitioned tables as logical replication target is not
>> supported.
>>
>> Sounds a bit repetitive, but perhaps it's better to use the words "not
>> supported" in the DETAIL message.
> 
> Or the detailed message could just say "\"%s.%s\" is a foreign table"
> and such flavor for other relkinds?  It is redundant to repeat
> "logical replication target" for both message parts.
Yeah, I think so too.  I also noticed that the patch uses
ERRCODE_WRONG_OBJECT_TYPE as the error code,  whereas we may want to use
ERRCODE_FEATURE_NOT_SUPPORTED.  Thoughts on that?

Attached updated patch, which changes the detail message text as you
suggest and updates the error code.

Thanks,
Amit

diff --git a/src/backend/executor/execReplication.c 
b/src/backend/executor/execReplication.c
index 5bd3bbc35e..c95153b463 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -609,8 +609,24 @@ CheckSubscriptionRelkind(char relkind, const char *nspname,
                                                 const char *relname)
 {
        /*
-        * We currently only support writing to regular tables.
+        * We currently only support writing to regular tables.  However, give
+        * a more specific error for partitioned and foreign tables.
         */
+       if (relkind == RELKIND_PARTITIONED_TABLE)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot use \"%s.%s\" as logical 
replication target",
+                                               nspname, relname),
+                                errdetail("\"%s.%s\" is a partitioned table.",
+                                               nspname, relname)));
+       else if (relkind == RELKIND_FOREIGN_TABLE)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot use \"%s.%s\" as logical 
replication target",
+                                               nspname, relname),
+                                errdetail("\"%s.%s\" is a foreign table.",
+                                               nspname, relname)));
+
        if (relkind != RELKIND_RELATION)
                ereport(ERROR,
                                (errcode(ERRCODE_WRONG_OBJECT_TYPE),