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(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.

---
Thanks and best regards,
Dang Minh Huong