On Fri, Dec 2, 2016 at 12:08 PM, Karl O. Pinc <k...@meme.com> wrote: > On Sun, 27 Nov 2016 21:54:46 +0100 > Gilles Darold <gilles.dar...@dalibo.com> wrote: > > > I've attached the v15 of the patch > > > I've not applied patch patch_pg_current_logfile-v14.diff.backoff to > > prevent constant call of logfile_writename() on a busy system (errno = > > ENFILE | EMFILE). > > I don't think it should be applied and included in the basic > functionality patch in any case. I think it needs to be submitted as a > separate patch along with the basic functionality patch. Backing off > the retry of the current_logfiles write could be overly fancy and > simply not needed. > > > I think this can be done quite simply by testing if > > log rotate is still enabled. This is possible because function > > logfile_rotate() is already testing if errno = ENFILE | EMFILE and in > > this case rotation_disabled is set to true. So the following test > > should do the work: > > > > if (log_metainfo_stale && !rotation_disabled) > > logfile_writename(); > > > > This is included in v15 patch. > > I don't see this helping much, if at all. > > First, it's not clear just when rotation_disabled can be false > when log_metainfo_stale is true. The typical execution > path is for logfile_writename() to be called after rotate_logfile() > has already set rotataion_disabled to true. logfile_writename() > is the only place setting log_metainfo_stale to true and > rotate_logfile() the only place settig rotation_disabled to false. > > While it's possible > that log_metainfo_stale might be set to true when logfile_writename() > is called from within open_csvlogfile(), this does not help with the > stderr case. IMO better to just test for log_metaifo_stale at the > code snippet above. > > Second, the whole point of retrying the logfile_writename() call is > to be sure that the current_logfiles file is updated before the logs > rotate. Waiting until logfile rotation is enabled defeats the purpose.
Moved to next CF with "needs review" status. Regards, Hari Babu Fujitsu Australia
