Hi,
Thank you for your comments. I updated patch to v0003.
> Do we still need the cancel_flags? I cannot find other reasons to terminate
> workers. Also the things I don't like is that
> BGWORKER_CANCEL_ADMIN_COMMANDS must
> have the same value as BGWORKER_EXIT_AT_DATABASE_DROP. Only one
> flag exists but
> it has 0x0004. Can we remove the argument and flags from the patch?
One reason for adding these flags was that I considered a case where
we might not want to allow all worker terminations during database deletion,
even when the BGWORKER_EXIT_AT_DATABASE_DROP flag is set.
However, This might be a rare case. Therefore, I removed these flags.
> Here are some more minor review comments:
>
> ======
> doc/src/sgml/bgworker.sgml
>
> 1. Typo?
>
> s/damon/daemon/
Thank you. Yes, it is a typo. I fixed this.
> ======
> src/backend/postmaster/bgworker.c
>
> 2.
> +void
> +CancelBackgroundWorkers(Oid databaseId, int cancel_flags)
> +{
> + int slotno;
> + bool signal_postmaster = false;
> +
> + LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE);
> +
> + for (slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)
> + {
> + BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];
> +
> + /* Check worker slot. */
> + if (!slot->in_use)
> + continue;
> +
> + /* 1st, check cancel flags. */
> + if ((slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP) &
> cancel_flags)
> + {
> + PGPROC *proc = BackendPidGetProc(slot->pid);
> +
> + if (!proc)
> + continue;
> +
> + /* 2nd, compare databaseId. */
> + if (proc->databaseId == databaseId)
> + {
> + /*
> + * Set terminate flag in shared memory, unless slot has
> + * been reused.
> + */
> + slot->terminate = true;
> + signal_postmaster = true;
> + }
> + }
> + }
>
> 2a.
> Declare slotno as a 'for' loop variable.
Thank you. I fixed this.
> ~
>
> 2b.
> There seem to be excessive conditions in the code. Is it better to
> restructure with less, like:
>
> for (int slotno = 0; ...)
> {
> ...
>
> if (!slot->in_use)
> continue;
>
> if (slot flags are not set to drop)
> continue;
> proc = BackendPidGetProc(slot->pid);
> if (proc && proc->databaseId == databaseId)
> {
> slot->terminate = true;
> signal_postmaster = true;
> }
> }
Thank you. I fixed this, too.
Regards,
Aya Iwata
Fujitsu Limited
