Re: User defined data types in Logical Replication

On Tue, Mar 6, 2018 at 8:35 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> I noticed that logicalrep_typmap_gettypname's only caller is
> slot_store_error_callback, which is an error callback. So by raising an
> error from the former, we would effectively just hide the actual reason
> for the error behind the error that the cache entry cannot be found.
>
> Therefore, I'm inclined to make this function raise a warning, then
> return a substitute value (something like "unrecognized type XYZ"). Do
> the same for the case with the mismatched major versions. Lastly, if
> LogicalRepTypMap is not defined, also throw a warning and return a
> substitute value rather than try to initialize the hash table:
> initializing the table is not going to make the entry show up in it,
> anyway, so it's pretty pointless; and there's plenty chance for things
> to go wrong, which is not a good idea in an error callback.

I agree with you about not hiding the actual reason for the error but
if we raise a warning at logicalrep_typmap_gettypname don't we call
slot_store_error_callback recursively?

>
> Do we need a new TAP test for this? Judging by
> https://coverage.postgresql.org/src/backend/replication/logical/relation.c.gcov.html
> showing all zeroes for the last function, we do. Same with
> slot_store_error_callback in
> https://coverage.postgresql.org/src/backend/replication/logical/worker.c.gcov.html
>

Agreed. Will add a TAP test.

Regards,

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