Re: Add index scan progress to pg_stat_progress_vacuum

    +     
    +      
    +       indexes_total bigint
    +      
    +      
    +       The number of indexes to be processed in the
    +       vacuuming indexes or cleaning up indexes phase
    +       of the vacuum.
    +      
    +     
    +
    +     
    +      
    +       indexes_processed bigint
    +      
    +      
    +       The number of indexes processed in the
    +       vacuuming indexes or cleaning up indexes phase.
    +       At the start of an index vacuum cycle, this value is set to 0.
    +      
    +     

    > Will these be set to 0 for failsafe vacuums and vacuums with INDEX_CLEANUP
    > turned off?

If the failsafe kicks in midway through a vacuum, the number indexes_total will not be reset to 0. If INDEX_CLEANUP is turned off, then the value will be 0 at the start of the vacuum.

    +typedef struct VacWorkerProgressInfo
    +{
    +    int                     num_vacuums;    /* number of active VACUUMS with parallel workers */
    +    int                     max_vacuums;    /* max number of VACUUMS with parallel workers */
    +    slock_t         mutex;
    +    VacOneWorkerProgressInfo vacuums[FLEXIBLE_ARRAY_MEMBER];
    +} VacWorkerProgressInfo;

    > max_vacuums appears to just be a local copy of MaxBackends.  Does this
    > information really need to be stored here?  Also, is there a strong reason
    > for using a spinlock instead of an LWLock?

First, The BTVacInfo code in backend/access/nbtree/nbtutils.c inspired this, so I wanted to follow this pattern. With that said, I do see max_vacuums being redundant here, and I am inclined to replace it with a MaxBackends() call. 

Second, There is no strong reason to use spinlock here except I incorrectly assumed it will be better for this case. After reading more about this and reading up src/backend/storage/lmgr/README, an LWLock will be better.

    +void
    +vacuum_worker_end(int leader_pid)
    +{
    +    SpinLockAcquire(&vacworkerprogress->mutex);
    +    for (int i = 0; i < vacworkerprogress->num_vacuums; i++)
    +    {
    +        VacOneWorkerProgressInfo *vac = &vacworkerprogress->vacuums[i];
    +
    +        if (vac->leader_pid == leader_pid)
    +        {
    +            *vac = vacworkerprogress->vacuums[vacworkerprogress->num_vacuums - 1];
    +            vacworkerprogress->num_vacuums--;
    +            SpinLockRelease(&vacworkerprogress->mutex);
    +            break;
    +        }
    +    }
    +    SpinLockRelease(&vacworkerprogress->mutex);
    +}

  >  I see this loop pattern in a couple of places, and it makes me wonder if
  >  this information would fit more naturally in a hash table.

Followed the pattern in backend/access/nbtree/nbtutils.c for this as well. Using dynahash may make sense here if it simplifies the code. Will look.

    +        if (callback)
    +            callback(values, 3);

  >  Why does this need to be set up as a callback function?  Could we just call
  >  the function if cmdtype == PROGRESS_COMMAND_VACUUM?  ISTM that is pretty
  >  much all this is doing.

The intention will be for the caller to set the callback early on in the function using the existing " if (pg_strcasecmp(cmd, "VACUUM") == 0), etc." statement. This way we avoid having to add another if/else block before tuplestore_putvalues is called.

--
Sami Imseih
Amazon Web Services