Re: User defined data types in Logical Replication - Mailing list pgsql-hackers

On Thu, Dec 7, 2017 at 12:23 AM, Dang Minh Huong <kakalot49@gmail.com> wrote:
> Hi Sawada-san,
>
> Sorry for my late response.
>
> On 2017/12/05 0:11, Masahiko Sawada wrote:
>
> There is one more case that user-defined data type is not supported in
> Logical Replication.
> That is when remote data type's name does not exist in SUBSCRIBE.
>
> In relation.c:logicalrep_typmap_gettypname
> We search OID in syscache by remote's data type name and mapping it, if it
> does not exist in syscache
> We will be faced with the bellow error.
>
>         if (!OidIsValid(entry->typoid))
>                 ereport(ERROR,
>
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>                                  errmsg("data type \"%s.%s\" required for
> logical replication does not exist",
>                                                 entry->nspname,
> entry->typname)));
>
> I think, it is not necessary to check typoid here in order to avoid above
> case, is that right?
>
> I think it's not right. We should end up with an error in the case where the
> same type name doesn't exist on subscriber. With your proposed patch,
> logicalrep_typmap_gettypname() can return an invalid string (entry->typname)
> in that case, which can be a cause of SEGV.
>
> Thanks, I think you are right.
> # I thought that entry->typname was received from walsender, and it is
> already be qualified in logicalrep_write_typ.
> # But we also need check it in subscriber, because it could be lost info in
> transmission.

Oops, the last sentence of my previous mail was wrong.
logicalrep_typmap_gettypname() doesn't return an invalid string since
entry->typname is initialized with a type name got from wal sender.

After more thought, we might not need to raise an error even if there
is not the same data type on both publisher and subscriber. Because
data is sent after converted to the text representation and is
converted to a data type according to the local table definition
subscribers don't always need to have the same data type. If it's
right, slot_store_error_callback() doesn't need to find a
corresponding local data type OID but just finds the corresponding
type name by remote data type OID. What do you think?

--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -100,8 +100,9 @@ static dlist_head lsn_mapping =
DLIST_STATIC_INIT(lsn_mapping);

 typedef struct SlotErrCallbackArg
 {
-    LogicalRepRelation *rel;
-    int            attnum;
+    LogicalRepRelMapEntry *rel;
+    int            remote_attnum;
+    int            local_attnum;
 } SlotErrCallbackArg;

Since LogicalRepRelMapEntry has a map of local attributes to remote
ones we don't need to have two attribute numbers.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center