RE: NaNs in numeric_power (was Re: Postgres 11 release notes)

[Huong Dangminh]

Hi, 

> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> David Rowley  writes:
> > On 16 May 2018 at 02:01, Tom Lane  wrote:
> >> I'm not particularly fussed about getting credit for that.  However,
> >> looking again at how that patch series turned out --- ie, that we
> >> ensured POSIX behavior for NaNs only in HEAD --- I wonder whether we
> >> shouldn't do what was mentioned in the commit log for 6bdf1303, and
> >> teach numeric_pow() about these same special cases.
> >> It seems like it would be more consistent to change both functions
> >> for v11, rather than letting that other shoe drop in some future
> >> major release.
> 
> > I'm inclined to agree. It's hard to imagine these two functions
> > behaving differently in regards to NaN input is useful to anyone.
> 

Thank you. The patch looks fine to me.
Also, I have done the "make check" in Windows and Linux environment with no 
problem.


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

RE: Postgres 11 release notes

[Huong Dangminh]

Hi,

> From: David Rowley [mailto:david.row...@2ndquadrant.com]
> Sent: Tuesday, May 15, 2018 3:01 PM
> To: Bruce Momjian 
> Cc: Đặng Minh Hướng ; PostgreSQL-development
> 
> Subject: Re: Postgres 11 release notes
> 
> On 15 May 2018 at 08:28, Bruce Momjian  wrote:
> > Consistently return NaN for
> > NaN inputs to power()
> > on older platforms (Dang Minh Huong)
> 
> While I'm not in favour of removing Dang's credit here, technically this
> patch was Tom's. The code added in float.c by Dang's patch
> (61b200e2f) was effectively reverted by 6bdf1303.  Dang's regression tests
> remain, so should also be credited along with Tom.

Thanks David, I agreed.
Also 6bdf1303 should be included like below,


 

 Consistently return NaN for
 NaN inputs to power()
-on older platforms (Dang Minh Huong)
+on older platforms (Tom Lane, Dang Minh Huong)



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

RE: power() function in Windows: "value out of range: underflow"

[Huong Dangminh]

> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> > I updated the patch as David Rowley mentioned.
> 
> Pushed.  I'd mainly note that you need to update all the variant float8
> expected-files, not just the primary one.  (Sometimes this requires a bit

Thank you.
I will be careful next time.

> of guesswork, but here we're expecting all platforms to produce the same
> result.  The buildfarm should tell us if I got it wrong.)

Also thanks a lot of fixing failure in BSD platform as the result from 
buildfarm.


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

RE: power() function in Windows: "value out of range: underflow"

[Huong Dangminh]

> > I updated the patch as David Rowley mentioned.
> 
> Looks fine to me. Please add to the next commitfest.

Thanks. Added.

> --
>  David Rowley   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
> 

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

RE: power() function in Windows: "value out of range: underflow"

[Huong Dangminh]

Thanks for confirming.
 
> 2018-04-11 0:13 GMT-03:00 David Rowley :
> > I can recreate this when building with MSVC 2012. I confirm that I see
> > the same as you. Microsoft are setting errno to EDOM in the above 3
> > cases, where in Linux the result is still NaN, just the errno is not
> > set.
> >
> FWIW, I tested in MSVC 2017 (15.6.4) and it worked like expected.
> Looking at [1], it seems there could be nasty bugs when using math functions
> in MSVC < 2013 (or 2015). I don't have some older MSVC here to try
> /fp:OPTIONHERE [2] to see if it makes any difference.

Thanks.
Anyway currently PostgreSQL installers seem not be compiled with MSVC 2017, 
so it should be fix for current users?
I updated the patch as David Rowley mentioned.


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



dpow_NaN_V2.patch
Description: dpow_NaN_V2.patch

RE: power() function in Windows: "value out of range: underflow"

[Huong Dangminh]

Hi,
Thanks for response

# I will add this thread to current CF soon.

> 2018-04-10 5:30 GMT-03:00 Huong Dangminh <huo-dangm...@ys.jp.nec.com>:
> > There are some cases that power() function does not work correctly
> > with 'NaN' arguments in Windows environment.
> > Something like,
> >
> What is your exact OS version? What is your postgres version? I tested with
> Windows 10 (10.0.16299) x64 and couldn't reproduce the error.

I think it is not depended on OS version but the visual c++ runtime libraries 
version 
(It seem also be included in installers).

My environment:


- PostgreSQL: 9.6.8
  # I am using the EDB PostgreSQL Installer
  # It also can reproduce in PostgreSQL 10.3 or 9.5.12

  >postgres -V
  postgres (PostgreSQL) 9.6.8

  >psql -c "select version()"
 version
  -
   PostgreSQL 9.6.8, compiled by Visual C++ build 1800, 64-bit
  (1 row)

  >psql -c "select 'NaN'^11"
  ERROR:  value out of range: underflow

- OS: Microsoft Windows 10 Pro 10.0.14393 build 14393

=

> > postgres=# select power('NaN',11);
> > ERROR:  value out of range: underflow
> > postgres=# select power('NaN','NaN');
> > ERROR:  value out of range: underflow
> > postgres=# select power(11,'NaN');
> > ERROR:  value out of range: underflow
> >
> > In Linux environment, instead of ERROR it returns 'NaN'.
> >
> Could you show us a simple test case? Print arguments, result and errno.

It just is my confirmation when debug PostgreSQL in Windows environment.
# I built PostgreSQL with VC++ 2012 in debug mode and confirmed.
I don't know how to print those values.


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

power() function in Windows: "value out of range: underflow"

[Huong Dangminh]

Hi,

There are some cases that power() function does not work 
correctly with 'NaN' arguments in Windows environment.
Something like,

postgres=# select power('NaN',11);
ERROR:  value out of range: underflow
postgres=# select power('NaN','NaN');
ERROR:  value out of range: underflow
postgres=# select power(11,'NaN');
ERROR:  value out of range: underflow

In Linux environment, instead of ERROR it returns 'NaN'.

The reason here is,
When pow() in float.c:dpow() is called with 'NaN' arguments,
pow() returns 'NaN' but in Windows environment errno is set to 
EDOM(invalid floating-point exception).
So, PostgreSQL update "result" and cause an ERROR in CHECKFLOATVAL macro.

I think it should be return 'NaN' in all of above cases.
I have tried to create a patch to fix it.
Please confirm the attached file.


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


power_NaN.PATCH
Description: power_NaN.PATCH

RE: PostgreSQL 10: Segmentation fault when using GROUPING SETS with all unsortable columns

[Huong Dangminh]

Hi,

> I have found a case which could get segmentation fault when using GROUPING
> SETS.
> It occurs when columns in GROUPING SETS are all unsortable but hashable.

I have added this thread to current CF.
please let me know if you need more information.


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

RE: User defined data types in Logical Replication

[Huong Dangminh]

> From: Masahiko Sawada [mailto:sawada.m...@gmail.com]
> >> Attached a new version patch incorporated the aboves. Please review it.
> >
> > Thanks for updating the patch.
> > It looks fine to me.

I mean that it passes make check, and subscription TAP tests too.

> >
> 
> Thank you for confirmation.
> 

Also. I have changed status in CF to Ready for Committer.
I would be glad if it can be applied from PostgreSQL 10.


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

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi Sawada-san,

> Thank you for quick response. The changes look good to me. But I wonder
> if the following changes needs some comments to describe what each checks
> does for.
> 
> -if (errarg->attnum < 0)
> +if (errarg->local_attnum <0)
> +return;
> +
> +rel = errarg->rel;
> +remote_attnum = rel->attrmap[errarg->local_attnum];
> +
> +if (remote_attnum < 0)
>  return;

Thanks, I have added some comments as you mentioned.


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


fix_slot_store_error_callback_v10.patch
Description: fix_slot_store_error_callback_v10.patch

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi Sawada-san,

> >> eventually we'll need to map types to local oid (and possibly more)
> >> where the local info is cached so that we can interpret binary
> >> representation of replicated data (which we'll add at some point
> >> since it's big performance boost).
> 
> Sounds good.
> 
> >>
> >> So I am afraid that if we do the rename of typmap to remotetype in
> >> this patch it will a) make backports of fixes in the related code
> >> harder, b) force us to rename it back again in the future.
> >
> > Thanks for your comment.
> >
> >> I'd keep your general approach but keep using typmap naming.
> >
> > I update the patch as Petr Jelineks mention, keep using typmap naming.
> >
> 
> Thank you for updating the patch. Here is a review comment.

Thanks for reviewing.

> -   if (errarg->attnum < 0)
> +   rel = errarg->rel;
> +   remote_attnum = rel->attrmap[errarg->local_attnum];
> +
> +   if (remote_attnum < 0)
> return;
> 
> I think errarg->local_attnum can be -1 here and access an invalid address
> if slot_store_error_callback() is called before setting
> errarg.local_attnum. I cannot see such call path in the current code so
> far but would need to be fixed.

I updated the patch to fix it.


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



fix_slot_store_error_callback_v9.patch
Description: fix_slot_store_error_callback_v9.patch

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi Petr Jelineks, Sawada-san

> I think the changes make sense in terms of how it all works now.
> 
> That said I don't think the renaming idea is a good one, the naming was
> chosen to be future proof because eventually we'll need to map types to
> local oid (and possibly more) where the local info is cached so that we
> can interpret binary representation of replicated data (which we'll add
> at some point since it's big performance boost).
> 
> So I am afraid that if we do the rename of typmap to remotetype in this
> patch it will a) make backports of fixes in the related code harder, b)
> force us to rename it back again in the future.

Thanks for your comment.

> I'd keep your general approach but keep using typmap naming.

I update the patch as Petr Jelineks mention, keep using typmap naming.


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


fix_slot_store_error_callback_v8.patch
Description: fix_slot_store_error_callback_v8.patch

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi Sawada-san,

> > Sent: Tuesday, December 12, 2017 9:41 AM
> > To: Dang Minh Huong <kakalo...@gmail.com>
> > Cc: PostgreSQL Hackers <pgsql-hack...@postgresql.org>; Yanagisawa
> > Hiroshi(柳澤 博) <hir-yanagis...@ut.jp.nec.com>; Dangminh Huong(ダン
> Ｍ
> > フーン) <huo-dangm...@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
> > <kakalo...@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.m...@gmail.com>
> > >>> wrote:
> > >>>>
> > >>>> On Thu, Dec 7, 2017 at 12:23 AM, Dang Minh Huong
> > >>>> <kakalo...@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/re

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi Sawada-san,

> Sent: Tuesday, December 12, 2017 9:41 AM
> To: Dang Minh Huong <kakalo...@gmail.com>
> Cc: PostgreSQL Hackers <pgsql-hack...@postgresql.org>; Yanagisawa
> Hiroshi(柳澤 博) <hir-yanagis...@ut.jp.nec.com>; Dangminh Huong(ダンＭ
> フーン) <huo-dangm...@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 <kakalo...@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.m...@gmail.com>
> >>> wrote:
> >>>>
> >>>> On Thu, Dec 7, 2017 at 12:23 AM, Dang Minh Huong
> >>>> <kakalo...@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;
> >>>> -intattnum;
> >>>> +LogicalRepRelMapEntry *rel;
> >>>> +intremote_attnum;
> >>>> +intlocal_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/





fix_slot_store_error_callback_v6.patch
Description: fix_slot_store_error_callback_v6.patch

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi Sawada-san,

> On Thu, Dec 7, 2017 at 11:07 AM, Masahiko Sawada 
> wrote:
> > On Thu, Dec 7, 2017 at 12:23 AM, Dang Minh Huong 
> 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;
> > -intattnum;
> > +LogicalRepRelMapEntry *rel;
> > +intremote_attnum;
> > +intlocal_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/

RE: Re: User defined data types in Logical Replication

[Huong Dangminh]

Hi,

> From: Huong Dangminh [mailto:huo-dangm...@ys.jp.nec.com]
> I attached a patch based on Sawada-san's patch with a bit of messages
> modified and remove the above check.
> Could somebody check it for me or should I add it into CF?

Sorry, I have added this thread to the next CF.


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

RE: Re: User defined data types in Logical Replication

[Huong Dangminh]

Sawada-san,

Thanks for your response.
# And sorry again because I could not reply to your gmail 
# address from my environment due to security restriction.

> >> We are getting the bellow error while trying use Logical Replication
> >> with user defined data types in a C program (when call elog function).
> >>
> >>  ERROR:  XX000: cache lookup failed for type X
> >>
> >
> > Sorry for continuously disturbing in this topic, but am I missing something
> here?
> 
> No, but I'd suggest to provide a procedure for reproducing if possible,
> which will be helpful for investigation.

Sorry, I will be careful next time.

> > I mean that in case of type's OID in PUBLICATION host does not exists
> > in SUBSCRIPTION host's pg_type, it could returns unintended error (the
> XX000 above) when elog or ereport is executed.
> >
> > For more details, it happen in slot_store_error_callback when it try to
> call format_type_be(localtypoid) for errcontext.
> > slot_store_error_callback is set in slot_store_cstrings,
> slot_modify_cstrings function and it also be unset here, so the effect here
> is small but it happens.
> >
> 
> I think I found out the cause of this issue, and this is a bug. This can
> be reproduced, for example, if the input function of the data type calls
> elog() during applying on the environment where OIDs of the data type on
> publisher and subscriber are different. The cause of this issue is that
> we call format_type_be() with remotetypoid. If the OIDs of data type on
> publisher and subscriber are different we search it from syscache by the
> OID that doesn't exist on subscriber.

Yes, I also think that.

> On detail of your patch, I don't think this direction is good. Since the
> subscriber already has a LogicalRepTyp cache entry for the type we can report
> the error message using the data type name. So I think this issue can be
> fixed by using the remote type name got from the cache.

Thanks, 
I did not realize the LogicalRepRelMapEntry, remote type name is already here.

> Also I'm confused about the message of errcontext; currently we store the
> local data type OID corresponding to the remote data type name into the
> cache, and then we search the local data type name by the local data type
> OID stored in the cache. So  it means the both the local data type OID and
> the remote data type OID always imply the same data type. We use the both
> data type OIDs for log message in slot_store_error_callback, but I think
> what the function want to do is to show the different type names if the
> table definitions on both server are different (e.g. sending jsonb column
> data to text column data). I think we should use the type of the local 
> relation
> attribute rather than remote's one.
> 
> Attached draft patch fixed this issue, at least on my environment.

It works good for me.

> Please review it.

I will review it soon.


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

RE: User defined data types in Logical Replication

[Huong Dangminh]

Hi,

> We are getting the bellow error while trying use Logical Replication with
> user defined data types in a C program (when call elog function).
> 
>  ERROR:  XX000: cache lookup failed for type X
> 

Sorry for continuously disturbing in this topic, but am I missing something 
here?

I mean that in case of type's OID in PUBLICATION host does not exists in 
SUBSCRIPTION host's pg_type, 
it could returns unintended error (the XX000 above) when elog or ereport is 
executed.

For more details, it happen in slot_store_error_callback when it try to call 
format_type_be(localtypoid) for errcontext.
slot_store_error_callback is set in slot_store_cstrings, slot_modify_cstrings 
function and it also be unset here, so the effect here is small but it happens.


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