Re: logical replication worker accesses catalogs in error context callback

Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> writes:
> Thanks for pointing to the changes in the commit message. I corrected
> them. Attaching v4 patch set, consider it for further review.

I took a quick look at this. I'm quite worried about the potential
performance cost of the v4-0001 patch (the one for fixing
slot_store_error_callback). Previously, we didn't pay any noticeable
cost for having the callback unless there actually was an error.
As patched, we perform several catalog lookups per column per row,
even in the non-error code path. That seems like it'd be a noticeable
performance hit. Just to add insult to injury, it leaks memory.

I propose a more radical but simpler solution: let's just not bother
with including the type names in the context message. How much are
they really buying?

v4-0002 (for postgres_fdw's conversion_error_callback) has the same
problems, although mitigated a bit by not needing to do any catalog
lookups in the non-join case. For the join case, I wonder whether
we could push the lookups back to plan setup time, so they don't
need to be done over again for each row. (Not doing lookups at all
doesn't seem attractive there; it'd render the context message nearly
useless.) A different idea is to try to get the column names out
of the query's rangetable instead of going to the catalogs.

regards, tom lane