Re: logical replication worker accesses catalogs in error context callback

On Sat, Jul 3, 2021 at 1:37 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> writes:
> >> << Attaching v5-0001 here again for completion >>
> >> I'm attaching v5-0001 patch that avoids printing the column type names
> >> in the context message thus no cache lookups have to be done in the
> >> error context callback. I think the column name is enough to know on
> >> which column the error occurred and if required it's type can be known
> >> by the user. This patch gets rid of printing local and remote type
> >> names in slot_store_error_callback and also
> >> logicalrep_typmap_gettypname because it's unnecessary. Thoughts?
>
> I've now pushed this patch. I noted that once we drop
> logicalrep_typmap_gettypname, there's really nothing using
> the LogicalRepTypMap table at all, so I nuked that too.
> (We can always recover the code from the git archives if
> some reason to use it emerges.)

Isn't it better to have the below comment before
slot_store_error_callback, similar to what's there before
conversion_error_callback in v7-0002.
* Note that this function mustn't do any catalog lookups, since we are in
* an already-failed transaction.

Maybe it's worth considering
avoid_sys_cache_lookup_in_error_callback_v2.patch from [1] as another
way to enforce the above statement.

[1] - https://www.postgresql.org/message-id/CAD21AoAwxbd-zXXUAeJ2FBRHr%2B%3DbfMUHoN7xJuXcwu1sFi1-sQ%40mail.gmail.com

Regards,
Bharath Rupireddy.