RE: User defined data types in Logical Replication

Hi Sawada-san,

> Sent: Tuesday, December 12, 2017 9:41 AM
> To: Dang Minh Huong <kakalot49@gmail.com>
> Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>; Yanagisawa
> Hiroshi(柳澤 博) <hir-yanagisawa@ut.jp.nec.com>; Dangminh Huong(ダンＭ
> フーン) <huo-dangminh@ys.jp.nec.com>
> Subject: Re: User defined data types in Logical Replication
> 
> On Sun, Dec 10, 2017 at 12:33 AM, Dang Minh Huong <kakalot49@gmail.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.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.
> 
> 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?

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