Re: logical replication worker accesses catalogs in error context callback

On Tue, Mar 16, 2021 at 2:21 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > 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?

Thanks. In that case, the message can only return the local and remote
columns names and ignore the types (something like below). And the
user will have to figure out what are the types of those columns in
local and remote separately in case of error. Then the function
logicalrep_typmap_gettypname can also be removed. I'm not sure if this
is okay. Thoughts?

TupleDesc localtupdesc = RelationGetDescr(rel->localrel);
Form_pg_attribute localatt = TupleDescAttr(localtupdesc,
errarg->local_attnum - 1);

errcontext("processing remote data for replication target relation
\"%s.%s\" column \"%s\", "
"remote column %s, local column %s",
rel->remoterel.nspname, rel->remoterel.relname,
rel->remoterel.attnames[errarg->remote_attnum],
NameStr(localatt->attname)));

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

I will think more on this point.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com