Thread: 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
Attachment
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