At Fri, 12 Mar 2021 23:33:05 +0900, Fujii Masao <masao.fu...@oss.nttdata.com> wrote in > > > On 2021/03/12 17:24, Kyotaro Horiguchi wrote: > > At Fri, 12 Mar 2021 15:13:15 +0900, Fujii Masao > > <masao.fu...@oss.nttdata.com> wrote in > >> On 2021/03/12 13:49, Kyotaro Horiguchi wrote: > >>> I noticed that I accidentally removed the launch-suppression feature > >>> that is to avoid frequent relaunching. That mechanism is needed on > >>> the postmaster side. I added PgArchIsSuppressed() to do the same check > >>> with the old pgarch_start() and make it called as a part of > >>> PgArchStartupAllowed(). > >> > >> You're right! At least for me the function name PgArchIsSuppressed() > >> sounds not good to me. What about something like PgArchCanRestart()? > > The reason for the name was some positive-meaning names came up with > > me are confusing with PgArchStartupAllowed(). The name > > PgArchCanRestart suggests that it's usable only when > > restarting. However, the function requires to be called also when the > > first launch since last_pgarch_start_time needs to be updated every > > time archiver is launched. > > Anyway in the attached the name is changed wihtout changing its usage. > > Thanks! If we come up with better name, let's rename the function > later.
Ok. > > # I don't like it uses "can" as "allowed" so much. The archiver > > # actually can restart but is just inhibited to restart. > > > >> This is not fault of this patch. But last_pgarch_start_time should be > >> initialized with zero? > > Right. I noticed that but forgot to fix it. > > > >> + if ((curtime - last_pgarch_start_time) < PGARCH_RESTART_INTERVAL) > >> + return true; > >> > >> Why did you remove the cast to unsigned int there? > > The cast converts small negative values to large numbers, the code > > looks like intending to allow archiver launched when curtime goes > > behind than last_pgarch_start_time. That is the case the on-memory > > data is corrupt. I'm not sure it's worth worrying and in the first > > place if we want to care of the case we should explicitly compare the > > operands of the subtraction. I did that in the attached. > > That's an idea. But the similar calculation using that cast is used in > other places (e.g., in pgarch_MainLoop()), so I'm thinking that it's > better not to change that... Mmm. I'm fine with it:( (:p) > > And the last_pgarch_start_time is accessed only in the function. I > > moved it to inside the function. > > OK. > > > > > >> + /* > >> + * Advertise our latch that backends can use to wake us up while > >> we're > >> + * sleeping. > >> + */ > >> + PgArch->pgprocno = MyProc->pgprocno; > >> > >> The comment should be updated? > > Hmm. What is advertised is our pgprocno.. Fixed. > > OK. > > Thanks for updating the patch! I applied some minor changes into your > patch. > Attached is the updated version of the patch. I'm thinking to commit > this version. Thanks for committing this! I'm very happy to see this reduces the size of this patchset. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
