RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE

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