Re: User defined data types in Logical Replication

On Sat, Dec 23, 2017 at 4:08 PM, Dang Minh Huong <kakalot49@gmail.com> wrote:
> On 2017/12/21 10:05, Masahiko Sawada wrote:
>
>> On Wed, Dec 20, 2017 at 5:39 PM, Huong Dangminh
>> <huo-dangminh@ys.jp.nec.com> wrote:
>>>
>>> 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.
>>>
>> Thank you for updating the patch.
>>
>> -       if (errarg->attnum < 0)
>> +       /* Check case of slot_store_error_callback() is called before
>> +        * errarg.local_attnum is set. */
>> +       if (errarg->local_attnum <0)
>>
>> This comment style isn't preferred by PostgreSQL code. Please refer to
>> https://www.postgresql.org/docs/current/static/source-format.html
>> --
>> $ git diff --check
>> src/backend/replication/logical/worker.c:291: trailing whitespace.
>> +       /* Check case of slot_store_error_callback() is called before
>
> Thanks, I will be careful in the next time.
>>
>> There is an extra white space in the patch.
>>
>> I'm inclined to think SlotErrCallbackArg can have remote_attnum and
>> pass it to the callback function. That way, we don't need to case the
>> case where local_attnum is not set yet.
>
> Nice.
>>
>> Attached a new version patch incorporated the aboves. Please review it.
>
> Thanks for updating the patch.
> It looks fine to me.
>

Thank you for confirmation.

Regards,

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