Re: User defined data types in Logical Replication

On Thu, Mar 15, 2018 at 9:41 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Masahiko Sawada wrote:
>
>> Regarding to regression test, I added a new test module
>> test_subscription that creates a new user-defined data type. In a
>> subscription regression test, using test_subscription we make
>> subscriber call slot_store_error_callback and check if the subscriber
>> can correctly look up both remote and local data type strings. One
>> downside of this regression test is that in a failure case, the
>> duration of the test will be long (up to 180sec) because it has to
>> wait for the polling timeout.
>> Attached an updated patch with a regression test.
>
> Pushed the fix to pg10 and master. Thanks to all involved for the
> report, patches and review.

Thank you for committing!

>
> Here's the regression test patch. The problem with it is that the TAP
> test is not verifying much -- I tried applying it before the fix commit,
> and it succeeds! The only funny is that the errcontext messages are
> wrong, they look like this:
>
> 2018-03-14 20:31:03.564 -03 [763018] LOG: input int: 1
> 2018-03-14 20:31:03.564 -03 [763018] CONTEXT: processing remote data for replication target relation "public.test" column "b", remote type dummytext, local type dummyint
> 2018-03-14 20:31:03.564 -03 [763018] LOG: input text: "one"
> 2018-03-14 20:31:03.564 -03 [763018] CONTEXT: processing remote data for replication target relation "public.test" column "a", remote type dummyint, local type dummytext
>
> but I think it would be better to verify them. (With your version I
> think you were trusting that the OID would not match anything, giving
> raise to the "cache lookup failed" error before the patch. I'm not sure
> that that's very trustworthy either.)

Agreed. Also, since my patch attempted to lead the error we need a
long time to check if it failed, which is not good.

>
> I think this is a worthwhile test, but IMO it should be improved a bit
> before we include it. Also, we can come up with a better name for the
> test surely, not just refer to this particular bug. Maybe "typemap".
>

It might be useful if we have the facility of TAP test to check the
log message and regexp-match the message to the expected string.

Regards,

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