RE: User defined data types in Logical Replication

Hi Sawada-san,

> > Sent: Tuesday, December 12, 2017 9:41 AM
> > To: Dang Minh Huong <kakalot49(at)gmail(dot)com>
> > Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>; Yanagisawa
> > Hiroshi(柳澤 博) <hir-yanagisawa(at)ut(dot)jp(dot)nec(dot)com>; Dangminh Huong(ダン
> Ｍ
> > フーン) <huo-dangminh(at)ys(dot)jp(dot)nec(dot)com>
> > Subject: 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.
>
> Yeah, so the meaning of the type map will be disappeared, aren't you?
> I updated the patch with removing of LogicalRepTyp.typoid and changing
> concept "typmap" to "remote type".
> How do you think?

Sorry, Please confirm V7 of patch (attached in this mail).

---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/