Re: "out of relcache_callback_list slots" after multiple calls to pg_logical_slot_get_binary_changes

On Wed, Feb 22, 2023 at 12:07:06PM +0900, Kyotaro Horiguchi wrote:
> At Wed, 22 Feb 2023 12:29:59 +1100, Peter Smith <smithpb2250@gmail.com> wrote in 
>> If you are going to do that, then won't just copying the
>> CacheRegisterSyscacheCallback(PUBLICATIONOID...  into function
>> init_rel_sync_cache() be effectively the same as doing that?
> 
> I'm not sure if it has anything to do with the relation sync cache.
> On the other hand, moving all the content of init_rel_sync_cache() up
> to pgoutput_startup() doesn't seem like a good idea.. Another option,
> as you see, was to separate callback registration code.

Both are kept separate in the code, so keeping this separation makes
sense to me.

+       /* Register callbacks if we didn't do that. */
+       if (!callback_registered)
+           CacheRegisterSyscacheCallback(PUBLICATIONOID,
+                                         publication_invalidation_cb,
+                                         (Datum) 0);
 
        /* Initialize relation schema cache. */
        init_rel_sync_cache(CacheMemoryContext);
+       callback_registered = true;
[...]
+   /* Register callbacks if we didn't do that. */
+   if (!callback_registered)

I am a bit confused by the use of one single flag called
callback_registered to track both the publication callback and the
relation callbacks.  Wouldn't it be cleaner to use two flags?  I don't
think that we'll have soon a second code path calling
init_rel_sync_cache(), but if we do then the callback load could again
be messed up.

(FYI, we use this method of callback registration for everything
that's not a one-time code path, like hash tables for RI triggers,
base backup callbacks, etc.)
--
Michael