On Mon, Jan 11, 2021 at 4:46 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Mon, Jan 11, 2021 at 10:56 AM Masahiko Sawada <sawada.m...@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/
avoid_sys_cache_lookup_in_error_callback_v2.patch
Description: Binary data