Re: User defined data types in Logical Replication

On Sun, Dec 10, 2017 at 12:33 AM, Dang Minh Huong <kakalot49(at)gmail(dot)com> wrote:
> On 2017/12/08 13:18, Huong Dangminh wrote:
>
>> Hi Sawada-san,
>>
>>> On Thu, Dec 7, 2017 at 11:07 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
>>> wrote:
>>>>
>>>> On Thu, Dec 7, 2017 at 12:23 AM, Dang Minh Huong <kakalot49(at)gmail(dot)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.

Yeah, actually we don't use LogicalRepTyp.typoid as well. If we allow
to replicate data to an another data type of column, what is the data
type mapping on subscriber for? It seems enough to have remote data
type oid, namespace and name.

Regards,

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