On Wed, Jan 6, 2021 at 11:42 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Wed, Jan 6, 2021 at 11:02 AM Andres Freund <and...@anarazel.de> wrote: > > > > Hi, > > > > Due to a debug ereport I just noticed that worker.c's > > slot_store_error_callback is doing something quite dangerous: > > > > static void > > slot_store_error_callback(void *arg) > > { > > SlotErrCallbackArg *errarg = (SlotErrCallbackArg *) arg; > > LogicalRepRelMapEntry *rel; > > char *remotetypname; > > Oid remotetypoid, > > localtypoid; > > > > /* Nothing to do if remote attribute number is not set */ > > if (errarg->remote_attnum < 0) > > return; > > > > rel = errarg->rel; > > remotetypoid = rel->remoterel.atttyps[errarg->remote_attnum]; > > > > /* Fetch remote type name from the LogicalRepTypMap cache */ > > remotetypname = logicalrep_typmap_gettypname(remotetypoid); > > > > /* Fetch local type OID from the local sys cache */ > > localtypoid = get_atttype(rel->localreloid, errarg->local_attnum + > > 1); > > > > errcontext("processing remote data for replication target relation > > \"%s.%s\" column \"%s\", " > > "remote type %s, local type %s", > > rel->remoterel.nspname, rel->remoterel.relname, > > rel->remoterel.attnames[errarg->remote_attnum], > > remotetypname, > > format_type_be(localtypoid)); > > } > > > > > > that's not code that can run in an error context callback. It's > > theoretically possible (but unlikely) that > > logicalrep_typmap_gettypname() is safe to run in an error context > > callback. But get_atttype() definitely isn't. > > > > get_attype() may do catalog accesses. That definitely can't happen > > inside an error context callback - the entire transaction might be > > borked at this point! > > You're right. Perhaps calling to format_type_be() is also dangerous > since it does catalog access. We should have added the local type > names to SlotErrCallbackArg so we avoid catalog access in the error > context. > > I'll try to fix this.
Attached the patch that fixes this issue. Since logicalrep_typmap_gettypname() could search the sys cache by calling to format_type_be(), I stored both local and remote type names to SlotErrCallbackArg so that we can just set the names in the error callback without sys cache lookup. Please review it. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
fix_slot_store_error_callback.patch
Description: Binary data