Re: User defined data types in Logical Replication

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

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.
Attached a new version patch incorporated the aboves. Please review it.

Regards,

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