On Mon, May 27, 2019 at 12:20:58AM -0400, Alvaro Herrera wrote:
> I wonder if we really want to abolish all distinction between "cannot do
> X" and "Y is not supported". I take the former to mean that the
> operation is impossible to do for some reason, while the latter means we
> just haven't implemented it yet and it seems likely to get implemented
> in a reasonable timeframe. See some excellent commentary about about
> the "can not" wording at
> https://postgr.es/m/CA+TgmoYS8jKhETyhGYTYMcbvGPwYY=qA6yYp9B47MX7MweE25w@mail.gmail.com
Incorrect URL?
> I notice your patch changes "catalog relations" to "system catalogs".
> I think we predominantly prefer the latter, so that part of your change
> seems OK. (In passing, I noticed we have a couple of places using
> "system catalog tables", which is weird.)
Good point. These are not new though, so I would prefer not touch
those parts for this patch.
src/backend/catalog/index.c: errmsg("user-defined
indexes on system catalog tables are not supported")));
src/backend/catalog/index.c: errmsg("concurrent index
creation on system catalog tables is not supported")));
src/backend/catalog/index.c: errmsg("user-defined
indexes on system catalog tables are not supported")));
src/backend/parser/parse_clause.c: errmsg("ON CONFLICT
is not supported with system catalog tables"),
> I think reindexing system catalogs concurrently is a complex enough
> undertaking that implementing it is far enough in the future that the
> "cannot" wording is okay; but reindexing partitioned tables is not so
> obviously out of the question.
I am not sure that we actually can without much complication, as
technically locks on catalogs may get released before commit if I
recall correctly.
> We do have "is not yet implemented" in a
> couple of other places, so all things considered I'm not so sure about
> changing that one to "cannot".
Okay. I can live with this difference. Not changing the string in
ReindexRelationConcurrently() has the merit to be consistent with the
existing ones in reindex_relation() and ReindexPartitionedIndex().
Please find attached an updated version. What do you think?
--
Michael