Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE - Mailing list pgsql-hackers

Hi,

Here are some more minor review comments:

======
doc/src/sgml/bgworker.sgml

1. Typo?

s/damon/daemon/

======
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.

~

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;
  }
}

======
Kind Regards,
Peter Smith.
Fujitsu Australia