Re: [HACKERS] Macros bundling RELKIND_* conditions

On 2018-Dec-19, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > I support this idea.  Here's a proof-of-concept patch that corresponds
> > to one of the cases that Ashutosh was on about (specifically, the one
> > that uses the RELKIND_CAN_HAVE_STORAGE macro I just added).  If there
> > are no objections to this approach, I'm going to complete it along these
> > lines.
> 
> +1

Here's a v2 -- completed patching the other places that needed it, fixed
test expected output, and made the other changes you suggested.

It's a bit scary that none of the backend message wording changes affect
any tests.  Only contrib test files had to be patched.  (I didn't run
sepgsql tests.)

One (very small IMO) problem here is that Ashutosh started from the
desire of being able to notice when a new relkind needs to be added to
checks spread in the code.  This patch doesn't really do that; the new
"errdetail_unsuitable_relkind" can be grepped for but that doesn't
really solve it because some places don't throw errors, but just, say,
"continue".  I don't see a solution for this.

But I do see one further problem, which is that each message being
patched was listing the relkinds that are allowed and now they ain't.
It seems user-friendly to preserve that list in some way.  An example:

diff --git a/src/backend/commands/indexcmdsindex bd85099c28..7a255176df 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 437,444 **** DefineIndex(Oid relationId,
          default:
              ereport(ERROR,
                      (errcode(ERRCODE_WRONG_OBJECT_TYPE),
!                      errmsg("\"%s\" is not a table or materialized view",
!                             RelationGetRelationName(rel))));
              break;
      }
  
--- 437,447 ----
          default:
              ereport(ERROR,
                      (errcode(ERRCODE_WRONG_OBJECT_TYPE),
!                      errmsg("cannot create index on \"%s\"",
!                             RelationGetRelationName(rel)),
!                      errdetail_unsuitable_relkind(rel->rd_rel->relkind,
!                                                   RelationGetRelationName(rel)),
!                      errhint("Indexes can only be created on regular tables, materialized views and partitioned
tables.")));
              break;
      }
  

This requires inventing a new message for at least some of the patched
sites.  I'm not happy about using errhint() for that, since that's not a
hint, but I don't see anything else we could use.

(I'm not sure about using the term "regular table" for RELKIND_RELATION,
but I think using unadorned "table" doesn't cut it anymore -- we have
TOAST tables, foreign tables, and partitioned tables, neither of which
are tables for many of these checks.  If you look at DefineIndex you'll
see that we even have a separate message for foreign tables there, which
kinda proves my point.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services