Re: User defined data types in Logical Replication

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.


---
Thanks and best regards,
Dang Minh Huong