Обсуждение: BUG #16818: progress reporting ALTER TABLE ADD UNIQUE

The following bug has been logged on the website:

Bug reference:      16818
Logged by:          Matthias van de  Meent
Email address:      boekewurm+postgres@gmail.com
PostgreSQL version: 12.5
Operating system:   Debian Stretch (9.13)
Description:

This may be considered a nitpick, but:

The progress reprorting for `ALTER TABLE test ADD UNIQUE (col)` is in
`pg_stat_progress_create_index`. As it indeed creates an index, that is not
too unexpected, but the `command` column of that view reports `CREATE
INDEX`, and _that_ is somewhat unexpected. A reasonable expectation would be
`ALTER TABLE ADD CONSTRAINT` or comparable. 

The only discussion regarding `ALTER TABLE` in index progress reporting
seems to have been in the original thread[0], but that was about potentially
thrashing callers' progress reporting status/values, and less about the
command name of this backend state.

[0]
https://www.postgresql.org/message-id/flat/20190329150828.s2bu4zckuxnceo6u%40alap3.anarazel.de

On 2021-Jan-11, PG Bug reporting form wrote:

> The progress reprorting for `ALTER TABLE test ADD UNIQUE (col)` is in
> `pg_stat_progress_create_index`. As it indeed creates an index, that is not
> too unexpected, but the `command` column of that view reports `CREATE
> INDEX`, and _that_ is somewhat unexpected. A reasonable expectation would be
> `ALTER TABLE ADD CONSTRAINT` or comparable. 

Hmm, seems a reasonable complaint.  Are there other command wordings
that would need to be handled?  I can't think of any (but I already
overlooked this one, evidently ...)

This seems fixed easily, in a way -- we'd need to set a distinct value
to the PROGRESS_CREATEIDX_COMMAND param when ALTER TABLE ADD; currently
possible values are in progress.h:

/* Commands of PROGRESS_CREATEIDX */
#define PROGRESS_CREATEIDX_COMMAND_CREATE        1
#define PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY    2
#define PROGRESS_CREATEIDX_COMMAND_REINDEX        3
#define PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY    4

The problem is that we'd need to change system_view.sql to recognize the
new value, and we can't change that on existing systems.  If we fail to
adjust that view definition, the column will show NULL when the command
is ALTER TABLE ADD.

Something like the attached patch, but I haven't tried to compile it
yet.  Probably need docs adjustments also.


(Also: I don't see we set
PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY anywhere ... an
oversight?)

-- 
Álvaro Herrera

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jan-11, PG Bug reporting form wrote:
>> The progress reprorting for `ALTER TABLE test ADD UNIQUE (col)` is in
>> `pg_stat_progress_create_index`. As it indeed creates an index, that is not
>> too unexpected, but the `command` column of that view reports `CREATE
>> INDEX`, and _that_ is somewhat unexpected. A reasonable expectation would be
>> `ALTER TABLE ADD CONSTRAINT` or comparable.

> Hmm, seems a reasonable complaint.  Are there other command wordings
> that would need to be handled?  I can't think of any (but I already
> overlooked this one, evidently ...)

TBH, I think that reporting it as "CREATE INDEX" is good, and what
the OP is asking for is less good.  Creating an index is what is
actually the time-consuming step here --- making the catalog entries
for the constraint is negligible.  Also, just how picky would we be
about replicating the command spelling -- e.g., consider "ALTER TABLE t
ADD PRIMARY KEY(p)" vs "ALTER TABLE t ADD CONSTRAINT c PRIMARY KEY(p)"
vs "ALTER TABLE t ADD COLUMN c int PRIMARY KEY" vs all the same options
for UNIQUE vs all the same options for EXCLUSION vs yadda yadda.
That is not going to be helpful to anybody, IMO, especially not
automated tools that might be watching the progress view.

It's reasonable for the view to distinguish REINDEX and CONCURRENTLY
options, as those are relevant to performance, but I don't think we
should add purely-cosmetic variations.

            regards, tom lane