On Fri, Aug 18, 2017 at 2:23 AM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
> 1. Hang in apw_detach_shmem.
> +/*
> + * Clear our PID from autoprewarm shared state.
> + */
> +static void
> +apw_detach_shmem(int code, Datum arg)
> +{
> + LWLockAcquire(&apw_state->lock, LW_EXCLUSIVE);
> + if (apw_state->pid_using_dumpfile == MyProcPid)
> + apw_state->pid_using_dumpfile = InvalidPid;
> + if (apw_state->bgworker_pid == MyProcPid)
> + apw_state->bgworker_pid = InvalidPid;
> + LWLockRelease(&apw_state->lock);
> +}
>
> The reason is that we might already be under the apw_state->lock when
> we error out and jump to apw_detach_shmem. So we should not be trying
> to take the lock again. For example, in autoprewarm_dump_now(),
> apw_dump_now() will error out under the lock if bgworker is already
> using dump file.
Ah, good catch. While I agree that there is probably no great harm
from skipping the lock here, I think it would be better to just avoid
throwing an error while we hold the lock. I think apw_dump_now() is
the only place where that could happen, and in the attached version,
I've fixed it so it doesn't do that any more. Independent of the
correctness issue, I think the code is easier to read this way.
I also realize that it's not formally sufficient to use
PG_TRY()/PG_CATCH() here, because a FATAL would leave us in a bad
state. Changed to PG_ENSURE_ERROR_CLEANUP().
> 2) I also found one issue which was my own mistake in my previous patch 19.
> In "apw_dump_now" I missed calling FreeFile() on first write error,
> whereas on othercases I am already calling the same.
> ret = fprintf(file, "<<" INT64_FORMAT ">>\n", num_blocks);
> + if (ret < 0)
> + {
> + int save_errno = errno;
> +
> + unlink(transient_dump_file_path);
Changed in the attached version.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers