Re: User defined data types in Logical Replication

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.
>
> 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.
>

Attached the patch incorporated what I have on mind. Please review it.

Regards,

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