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

On Wed, Feb 22, 2023 2:20 PM Michael Paquier <michael@paquier.xyz> wrote:
> 
> 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.
> 

Thanks for your reply. Using two flags makes sense to me.
Attach the updated patch.

Regards,
Shi Yu