Re: logical replication worker accesses catalogs in error context callback

On Wed, Jan 6, 2021 at 11:42 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jan 6, 2021 at 11:02 AM Andres Freund <andres(at)anarazel(dot)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/