From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [patch] Concurrent table reindex per-index progress reporting |
Date: | 2020-09-25 06:44:28 |
Message-ID: | 20200925064351.GG3571@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Sep 24, 2020 at 09:19:18PM +0200, Matthias van de Meent wrote:
> While working on a PG12-instance I noticed that the progress reporting of
> concurrent index creation for non-index relations fails to update the
> index/relation OIDs that it's currently working on in the
> pg_stat_progress_create_index view after creating the indexes. Please find
> attached a patch which properly sets these values at the appropriate places.
>
> Any thoughts?
I agree that this is a bug and that we had better report correctly the
heap and index IDs worked on, as these could also be part of a toast
relation from the parent table reindexed. However, your patch is
visibly incorrect in the two places you are updating.
> + * Configure progress reporting to report for this index.
> + * While we're at it, reset the phase as it is cleared by start_command.
> + */
> + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID, newIndexId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
> + PROGRESS_CREATEIDX_PHASE_WAIT_1);
First, updating PROGRESS_CREATEIDX_PHASE here to WAIT_1 makes no
sense, because this is not a wait phase, and index_build() called
within index_concurrently_build() will set this state correctly a bit
after so IMO it is fine to leave that unset here, and the build phase
is by far the bulk of the operation that needs tracking. I think that
you are also missing to update PROGRESS_CREATEIDX_COMMAND to
PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY, and
PROGRESS_CREATEIDX_ACCESS_METHOD_OID to the correct index AM OID,
similarly to reindex_index().
> + /*
> + * Configure progress reporting to report for this index.
> + * While we're at it, reset the phase as it is cleared by start_command.
> + */
> + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID, newIndexId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
> + PROGRESS_CREATEIDX_PHASE_WAIT_2);
> +
> validate_index(heapId, newIndexId, snapshot);
Basically the same three comments here: PROGRESS_CREATEIDX_PHASE set
to WAIT_2 makes no real sense, and validate_index() would update the
phase as it should be. This should set PROGRESS_CREATEIDX_COMMAND to
PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY, and
PROGRESS_CREATEIDX_ACCESS_METHOD_OID to the correct index AM OID.
While reviewing this code, I also noticed that the wait state set just
before dropping the indexes was wrong. The code was using WAIT_4, but
this has to be WAIT_5.
And this gives me the attached. This also means that we still set the
table and relation OID to the last index worked on for WAIT_2, WAIT_4
and WAIT_5, but we cannot set the command start to relationOid either
as given in input of ReindexRelationConcurrently() as this could be a
single index given for REINDEX INDEX CONCURRENTLY. Matthias, does
that look right to you?
--
Michael
Attachment | Content-Type | Size |
---|---|---|
reindex-progress-v2.patch | text/x-diff | 3.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Banck | 2020-09-25 06:53:27 | Re: [patch] Fix checksum verification in base backups for zero page headers |
Previous Message | Fujii Masao | 2020-09-25 06:38:46 | Re: Feature improvement for FETCH tab completion |