Re: User defined data types in Logical Replication

On Sat, Dec 23, 2017 at 4:08 PM, Dang Minh Huong <kakalot49(at)gmail(dot)com> wrote:
> On 2017/12/21 10:05, Masahiko Sawada wrote:
>
>> On Wed, Dec 20, 2017 at 5:39 PM, Huong Dangminh
>> <huo-dangminh(at)ys(dot)jp(dot)nec(dot)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