Re: User defined data types in Logical Replication

On 2017/12/08 13:18, Huong Dangminh wrote:

> Hi Sawada-san,
>
>> On Thu, Dec 7, 2017 at 11:07 AM, Masahiko Sawada <sawada.mshk@gmail.com>
>> wrote:
>>> 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.
Yeah, so we do not need to check the existing of publisher's type name 
in subscriber.
>>> 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?
I totally agree. It will make logical replication more flexible with 
data type.
>>> --- 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.
>>>
> Sorry for the late reply.
>
>> Attached the patch incorporated what I have on mind. Please review it.
> Thanks for the patch, I will do it at this weekend.
Your patch is fine for me.
But logicalrep_typmap_getid will be unused.
I attached the patch with removing of it.


---
Thanks and best regards,
Dang Minh Huong