Re: User defined data types in Logical Replication

On Wed, Dec 20, 2017 at 2:28 PM, Huong Dangminh
<huo-dangminh(at)ys(dot)jp(dot)nec(dot)com> wrote:
> Hi Sawada-san,
>
>> >> eventually we'll need to map types to local oid (and possibly more)
>> >> where the local info is cached so that we can interpret binary
>> >> representation of replicated data (which we'll add at some point
>> >> since it's big performance boost).
>>
>> Sounds good.
>>
>> >>
>> >> So I am afraid that if we do the rename of typmap to remotetype in
>> >> this patch it will a) make backports of fixes in the related code
>> >> harder, b) force us to rename it back again in the future.
>> >
>> > Thanks for your comment.
>> >
>> >> I'd keep your general approach but keep using typmap naming.
>> >
>> > I update the patch as Petr Jelineks mention, keep using typmap naming.
>> >
>>
>> Thank you for updating the patch. Here is a review comment.
>
> Thanks for reviewing.
>
>> - if (errarg->attnum < 0)
>> + rel = errarg->rel;
>> + remote_attnum = rel->attrmap[errarg->local_attnum];
>> +
>> + if (remote_attnum < 0)
>> return;
>>
>> I think errarg->local_attnum can be -1 here and access an invalid address
>> if slot_store_error_callback() is called before setting
>> errarg.local_attnum. I cannot see such call path in the current code so
>> far but would need to be fixed.
>
> I updated the patch to fix it.
>

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;

Regards,

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