Re: User defined data types in Logical Replication

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

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