From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> |
Cc: | andres(at)anarazel(dot)de, gkokolatos(at)protonmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: shared-memory based stats collector |
Date: | 2021-03-12 14:33:05 |
Message-ID: | 25dc6a37-5d9a-85c6-1f13-db68ac125f44@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2021/03/12 17:24, Kyotaro Horiguchi wrote:
> At Fri, 12 Mar 2021 15:13:15 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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.
>
> # 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...
>
> 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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachment | Content-Type | Size |
---|---|---|
v55-0003-Make-archiver-process-an-auxiliary-process_fujii.patch | text/plain | 23.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Laurenz Albe | 2021-03-12 14:35:28 | Re: documentation fix for SET ROLE |
Previous Message | Robert Haas | 2021-03-12 13:33:14 | Re: pg_amcheck contrib application |