Re: User defined data types in Logical Replication

On Mon, Dec 18, 2017 at 11:25 PM, Huong Dangminh
<huo-dangminh@ys.jp.nec.com> wrote:
>> 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.

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

Regards,

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