Re: [Tarantool-patches] [PATCH v2 1/3] gc/xlog: delay xlog cleanup until relays are subscribed

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Tue, Mar 23, 2021 at 10:28:46AM +0300, Cyrill Gorcunov wrote:
> 
> > 
> > Also you should use 'replication_anon' global variable instead
> > of the config, which might be not installed at this moment yet.
> 
> What would happen if one setup both 'wal_cleanup_delay' and
> 'replication_anon' in config at once. Which C's replication_anon
> value I will be reading? The C's replication_anon is set after
> the cfg procedure complete, so since I operate on values obrained
> from Lua level I need to use cfg_geti("replication_anon") because
> at this moment only Lua level is consistent and replication_anon
> may have a value from previous box.cfg call.

Vlad, here are some moments I don't understand.

1) gc_init is called from box_cfg_xc, and here is a call chain

box.cfg{}
  box.cfg = locked(load_cfg)
    pcall(private.cfg_check)
      -- goes into C layer --
      lbox_cfg_check
        box_check_config
          ...
          box_check_replication
          if (box_check_wal_cleanup_delay() < 0)
	    diag_raise();

          here either all values are sane and
          passes the tests, or configuration
          stage aborts

      -- back to Lua level --
    lbox_cfg_load
      -- jusmp to C again --
      box_cfg
        box_cfg_xc
          ...
          gc_init(box_check_wal_cleanup_delay());

here the box_check_wal_cleanup_delay won't fail because
it is already verified but we can't be sure that any
dependent variable (as global replication_anon) been
already set or not. And we should not bound to order
of box_check_wal_cleanup_delay() invocation relatively
to some other helpers.

Thus I think we should use cfg_geti("replication_anon") inside
box_check_wal_cleanup_delay code instead because only the
level cfg_x() is consistent here.

On reconfiguration stage the scheme is similar we must
not access C's global replication_anon variable which
is rather a cached value from cfg_geti("replication_anon").

Am I convinced you? I mean current code with access to
cfg_geti("replication_anon") looks more preferred.