On Fri, Aug 18, 2017 at 2:23 AM, Mithun Cy <[email protected]> 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
autoprewarm-rmh-v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
