Re: Proposal : For Auto-Prewarm.

On Sun, Jul 2, 2017 at 10:32 PM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
> On Tue, Jun 27, 2017 at 11:41 AM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
>> On Fri, Jun 23, 2017 at 5:45 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>>>
>>> Also, I find it a bit messy that launch_autoprewarm_dump() doesn't
>>> detect an autoprewarm process already running. I'd want this to
>>> return NULL or an error if called for a 2nd time.
>>
>> We log instead of error as we try to check only after launching the
>> worker and inside worker. One solution could be as similar to
>> autoprewam_dump_now(), the autoprewarm_start_worker() can init shared
>> memory and check if we can launch worker in backend itself. I will try
>> to fix same.
>
> I have fixed it now as follows
>
> +Datum
> +autoprewarm_start_worker(PG_FUNCTION_ARGS)
> +{
> + pid_t pid;
> +
> + init_apw_shmem();
> + pid = apw_state->bgworker_pid;
> + if (pid != InvalidPid)
> + ereport(ERROR,
> + (errmsg("autoprewarm worker is already running under PID %d",
> + pid)));
> +
> + autoprewarm_dump_launcher();
> + PG_RETURN_VOID();
> +}
>
> In backend itself, we shall check if an autoprewarm worker is running
> then only start the server. There is a possibility if this function is
> executed concurrently when there is no worker already running (Which I
> think is not a normal usage) then both call will say it has
> successfully launched the worker even though only one could have
> successfully done that (other will log and silently die).

Why can't we close this remaining race condition? Basically, if we
just perform all of the actions in this function under the lock and
autoprewarm_dump_launcher waits till the autoprewarm worker has
initialized the bgworker_pid, then there won't be any remaining race
condition. I think if we don't close this race condition, it will be
unpredictable whether the user will get the error or there will be
only a server log for the same.

> I think that
> is okay as the objective was to get one worker up and running.
>

You are right that the objective will be met, but still, I feel the
behavior of this API will be unpredictable which in my opinion should
be fixed. If it is really not possible or extremely difficult to fix
this behavior, then I think we should update the docs.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com