On Tue, Mar 6, 2018 at 8:35 AM, Alvaro Herrera <alvhe...@alvh.no-ip.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
