RE: User defined data types in Logical Replication

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

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.

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