Re: logical replication worker accesses catalogs in error context callback

On Mon, Jan 11, 2021 at 4:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Jan 11, 2021 at 10:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > Agreed. Attached the updated patch.
>
> Thanks for the updated patch. Looks like the comment crosses the 80
> char limit, I have corrected it. And also changed the variable name
> from remotetypeid to remotetypid, so that logicalrep_typmap_gettypname
> will not cross the 80 char limit. And also added a commit message.
> Attaching v3 patch, please have a look. Both make check and make
> check-world passes.

Thanks! The change looks good to me.

>
> > > I quickly searched in places where error callbacks are being used, I
> > > think we need a similar kind of fix for conversion_error_callback in
> > > postgres_fdw.c, because get_attname and get_rel_name are being used
> > > which do catalogue lookups. ISTM that all the other error callbacks
> > > are good in the sense that they are not doing sys catalogue lookups.
> >
> > Indeed. If we need to disallow the catalog lookup during executing
> > error callbacks we might want to have an assertion checking that in
> > SearchCatCacheInternal(), in addition to Assert(IsTransactionState()).
>
> I tried to add(as attached in
> v1-0001-Avoid-Sys-Cache-Lookups-in-Error-Callbacks.patch) the
> Assert(!error_context_stack); in SearchCatCacheInternal, initdb itself
> fails [1]. That means, we are doing a bunch of catalogue access from
> error context callbacks. Given this, I'm not quite sure whether we can
> have such an assertion in SearchCatCacheInternal.

I think checking !error_context_stack is not a correct check if we're
executing an error context callback since it's a stack to store
callbacks. It can be non-NULL by setting an error callback, see
setup_parser_errposition_callback() for instance. Probably we need to
check if (recursion_depth > 0) and elevel. Attached a patch for that
as an example.

>
> Although unrelated to what we are discussing here -  when I looked at
> SearchCatCacheInternal, I found that the function SearchCatCache has
> SearchCatCacheInternal in the function comment, I think we should
> correct it. Thoughts? If okay, I will post a separate patch for this
> minor comment fix.

Perhaps you mean this?

/*
 *  SearchCatCacheInternal
 *
 *      This call searches a system cache for a tuple, opening the relation
 *      if necessary (on the first access to a particular cache).
 *
 *      The result is NULL if not found, or a pointer to a HeapTuple in
 *      the cache.  The caller must not modify the tuple, and must call
 *      ReleaseCatCache() when done with it.
 *
 * The search key values should be expressed as Datums of the key columns'
 * datatype(s).  (Pass zeroes for any unused parameters.)  As a special
 * exception, the passed-in key for a NAME column can be just a C string;
 * the caller need not go to the trouble of converting it to a fully
 * null-padded NAME.
 */
HeapTuple
SearchCatCache(CatCache *cache,

Looking at commit 141fd1b66 it intentionally changed to
SearchCatCacheInternal from SearchCatCache but it seems to me that
it's a typo.

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/