Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [tarantool-patches] Re: [PATCH 01/10] box: zap atfork callback
Date: Thu, 6 Jun 2019 13:04:01 +0300	[thread overview]
Message-ID: <20190606100401.xwgtkfw5rh5c5odu@esperanza> (raw)
In-Reply-To: <20190601081608.GB29429@atlas>

On Sat, Jun 01, 2019 at 11:16:08AM +0300, Konstantin Osipov wrote:
> * Konstantin Osipov <kostja@tarantool.org> [19/05/18 21:41]:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/17 17:54]:
> > > box_atfork calls wal_atfork which in turn calls xlog_atfork for the wal
> > > and vylog files. A comment to xlog_atfork says that it's necessary to
> > > prevent atexit handlers in a child from closing xlog files again, but we
> > > don't use atexit for that anymore. A comment to box_atfork says that
> > > box.coredump forks to write a core, but there's no box.coredump anymore.
> > > There's also a comment mentioning box.cfg.background, but when we fork
> > > that early there's no xlog file open.
> > > 
> > > To sum it up, atfork looks like a piece of legacy code. Let's get rid of
> > > it now so as not to bother patching it later.
> > 
> > If anyone adds a fork any time in the future, xlogs will break
> > silently, and it would be hard to catch in a test. 
> > 
> > If you wish to remove the dead code, please keep the atfork, but panic in it.
> 
> As discussed in the channel, you can't panic in atfork handler
> since there is an upcoming popen() patch. This all still looks
> very fragile.

Note, I didn't remove atfork handlers completely. I only removed the box
part, which closes xlog file descriptors. This doesn't make much sense
anyway, because there are lots of other file descriptors that need to be
closed (think of vinyl) so closing xlog and vylog only is just a half
measure. We should probably use O_CLOEXEC instead.

That's why I don't understand why you're so opposed to this particular
patch.

> I read from vfork() man page that NPTL doesn't call
> pthread_atfork() handlers in vfork,
> while older implementation may. What is the behaviour on freebsd? 
> If both systems don't invoke the handlers, I would keep the
> panic() call, otherwise remove.

Hmm, turns out we use fork() to spawn a pipe logger thread. And it may
be called after box.cfg from a C module to create a custom logger. So we
definitely can't panic() there.

  reply	other threads:[~2019-06-06 10:04 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-17 14:52 [PATCH 00/10] vinyl: don't yield in DDL on_commit triggers Vladimir Davydov
2019-05-17 14:52 ` [PATCH 01/10] box: zap atfork callback Vladimir Davydov
2019-05-18 18:37   ` [tarantool-patches] " Konstantin Osipov
2019-05-20  8:13     ` Vladimir Davydov
2019-06-01  8:16     ` Konstantin Osipov
2019-06-06 10:04       ` Vladimir Davydov [this message]
2019-05-17 14:52 ` [PATCH 02/10] vinyl: add a separate thread for vylog Vladimir Davydov
2019-05-18 18:39   ` [tarantool-patches] " Konstantin Osipov
2019-05-20  8:17     ` Vladimir Davydov
2019-06-01  8:26   ` Konstantin Osipov
2019-06-06 10:20     ` Vladimir Davydov
2019-05-17 14:52 ` [PATCH 03/10] vinyl: move vylog recovery to vylog thread Vladimir Davydov
2019-06-01  8:36   ` [tarantool-patches] " Konstantin Osipov
2019-06-06 10:23     ` Vladimir Davydov
2019-06-07 13:39       ` Konstantin Osipov
2019-06-10 15:24         ` Vladimir Davydov
2019-06-07 13:40       ` Konstantin Osipov
2019-05-17 14:52 ` [PATCH 04/10] vinyl: rework vylog transaction backlog implementation Vladimir Davydov
2019-06-01  8:38   ` [tarantool-patches] " Konstantin Osipov
2019-06-06 11:58     ` Vladimir Davydov
2019-05-17 14:52 ` [PATCH 05/10] vinyl: don't purge deleted runs from vylog on compaction Vladimir Davydov
2019-05-18 18:47   ` [tarantool-patches] " Konstantin Osipov
2019-05-20  8:27     ` Vladimir Davydov
2019-06-01  8:39   ` Konstantin Osipov
2019-06-06 12:40     ` Vladimir Davydov
2019-05-17 14:52 ` [PATCH 06/10] vinyl: lock out compaction while checkpointing is in progress Vladimir Davydov
2019-05-17 14:52 ` [PATCH 07/10] vinyl: don't access last vylog signature outside vylog implementation Vladimir Davydov
2019-05-17 14:52 ` [PATCH 08/10] vinyl: zap ERRINJ_VY_LOG_FLUSH_DELAY Vladimir Davydov
2019-05-17 14:52 ` [PATCH 09/10] key_def: pass alloc callback to key_def_dump_parts Vladimir Davydov
2019-05-18 18:52   ` [tarantool-patches] " Konstantin Osipov
2019-05-20  8:34     ` Vladimir Davydov
2019-06-01  8:41     ` Konstantin Osipov
2019-06-10 15:28       ` Vladimir Davydov
2019-06-16 14:57         ` Konstantin Osipov
2019-05-17 14:52 ` [PATCH 10/10] vinyl: get rid of the latch protecting vylog buffer Vladimir Davydov
2019-06-01  8:44   ` [tarantool-patches] " Konstantin Osipov
2019-06-06 13:15     ` Vladimir Davydov
2019-05-18 18:35 ` [tarantool-patches] Re: [PATCH 00/10] vinyl: don't yield in DDL on_commit triggers Konstantin Osipov
2019-05-20  8:09   ` Vladimir Davydov
2019-06-01  8:09 ` Konstantin Osipov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190606100401.xwgtkfw5rh5c5odu@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kostja@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] Re: [PATCH 01/10] box: zap atfork callback' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox