Thread: Proposed patch for bug #3921

Attached is a proposed patch for bug #3921, which complained that CREATE
TABLE LIKE INCLUDING INDEXES fails inappropriately for non-superusers.

There are two parts to the patch: the first follows Greg Starks' opinion
that explicitly specifying the current database's default tablespace
shouldn't be an error at all, and so the permissions checks during
table/index creation are suppressed when tablespaceId ==
MyDatabaseTableSpace.  (Note that the assign hooks for
default_tablespace and temp_tablespaces already behaved this way, so we
were not exactly being consistent anyhow.)  I couldn't find anyplace in
the documentation that specifies the old behavior, so no docs changes.

The second part fixes the LIKE INCLUDING INDEXES code to default the
index tablespace specification when the source index is in the
database's default tablespace.  This would provide an alternative
way of fixing the bug if anyone doesn't like the first part.  With the
first part it's redundant, but seems worth doing anyway to avoid the
overhead of looking up the tablespace name and then converting it back
to OID in the typical case.

Also there is a correction of an obsolete comment in parsenodes.h, which
should be applied in any case.

An issue that this patch doesn't address is what happens if the source
index is in a non-default tablespace that the current user does not have
CREATE permission for.  With both current CVS HEAD and this patch, that
will result in an error.  Is that okay?  We could imagine making
parse_utilcmd.c check the permissions and default the tablespace
specification if no good, but that behavior seems a bit peculiar to me.
Command semantics don't normally change based on whether you have
permissions or not.

An entirely different tack we could take is to adopt the position
that LIKE INCLUDING INDEXES shouldn't copy index tablespaces at all,
but I didn't hear anyone favoring that approach in the bug discussion.

            regards, tom lane

Index: src/backend/commands/indexcmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v
retrieving revision 1.170
diff -c -r1.170 indexcmds.c
*** src/backend/commands/indexcmds.c    9 Jan 2008 21:52:36 -0000    1.170
--- src/backend/commands/indexcmds.c    3 Feb 2008 02:32:14 -0000
***************
*** 215,221 ****
      }

      /* Check permissions except when using database's default */
!     if (OidIsValid(tablespaceId))
      {
          AclResult    aclresult;

--- 215,221 ----
      }

      /* Check permissions except when using database's default */
!     if (OidIsValid(tablespaceId) && tablespaceId != MyDatabaseTableSpace)
      {
          AclResult    aclresult;

Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.241
diff -c -r1.241 tablecmds.c
*** src/backend/commands/tablecmds.c    30 Jan 2008 19:46:48 -0000    1.241
--- src/backend/commands/tablecmds.c    3 Feb 2008 02:32:15 -0000
***************
*** 340,346 ****
      }

      /* Check permissions except when using database's default */
!     if (OidIsValid(tablespaceId))
      {
          AclResult    aclresult;

--- 340,346 ----
      }

      /* Check permissions except when using database's default */
!     if (OidIsValid(tablespaceId) && tablespaceId != MyDatabaseTableSpace)
      {
          AclResult    aclresult;

Index: src/backend/executor/execMain.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/execMain.c,v
retrieving revision 1.302
diff -c -r1.302 execMain.c
*** src/backend/executor/execMain.c    1 Jan 2008 19:45:49 -0000    1.302
--- src/backend/executor/execMain.c    3 Feb 2008 02:32:15 -0000
***************
*** 2594,2600 ****
      }

      /* Check permissions except when using the database's default space */
!     if (OidIsValid(tablespaceId))
      {
          AclResult    aclresult;

--- 2594,2600 ----
      }

      /* Check permissions except when using the database's default space */
!     if (OidIsValid(tablespaceId) && tablespaceId != MyDatabaseTableSpace)
      {
          AclResult    aclresult;

Index: src/backend/parser/parse_utilcmd.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_utilcmd.c,v
retrieving revision 2.8
diff -c -r2.8 parse_utilcmd.c
*** src/backend/parser/parse_utilcmd.c    1 Jan 2008 19:45:51 -0000    2.8
--- src/backend/parser/parse_utilcmd.c    3 Feb 2008 02:32:15 -0000
***************
*** 767,773 ****
      index = makeNode(IndexStmt);
      index->relation = cxt->relation;
      index->accessMethod = pstrdup(NameStr(amrec->amname));
!     index->tableSpace = get_tablespace_name(source_idx->rd_node.spcNode);
      index->unique = idxrec->indisunique;
      index->primary = idxrec->indisprimary;
      index->concurrent = false;
--- 767,776 ----
      index = makeNode(IndexStmt);
      index->relation = cxt->relation;
      index->accessMethod = pstrdup(NameStr(amrec->amname));
!     if (OidIsValid(idxrelrec->reltablespace))
!         index->tableSpace = get_tablespace_name(idxrelrec->reltablespace);
!     else
!         index->tableSpace = NULL;
      index->unique = idxrec->indisunique;
      index->primary = idxrec->indisprimary;
      index->concurrent = false;
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.358
diff -c -r1.358 parsenodes.h
*** src/include/nodes/parsenodes.h    1 Jan 2008 19:45:58 -0000    1.358
--- src/include/nodes/parsenodes.h    3 Feb 2008 02:32:15 -0000
***************
*** 1537,1543 ****
      char       *idxname;        /* name of new index, or NULL for default */
      RangeVar   *relation;        /* relation to build index on */
      char       *accessMethod;    /* name of access method (eg. btree) */
!     char       *tableSpace;        /* tablespace, or NULL to use parent's */
      List       *indexParams;    /* a list of IndexElem */
      List       *options;        /* options from WITH clause */
      Node       *whereClause;    /* qualification (partial-index predicate) */
--- 1537,1543 ----
      char       *idxname;        /* name of new index, or NULL for default */
      RangeVar   *relation;        /* relation to build index on */
      char       *accessMethod;    /* name of access method (eg. btree) */
!     char       *tableSpace;        /* tablespace, or NULL for default */
      List       *indexParams;    /* a list of IndexElem */
      List       *options;        /* options from WITH clause */
      Node       *whereClause;    /* qualification (partial-index predicate) */

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

> An issue that this patch doesn't address is what happens if the source
> index is in a non-default tablespace that the current user does not have
> CREATE permission for.  With both current CVS HEAD and this patch, that
> will result in an error.  Is that okay?

I was going to say "we could add a hint suggesting how to specify the
tablespace" but as you pointed out earlier there really isn't a way to specify
the tablespace.

Hm, looking at the grammar we already have something like this for
constraints. We could add an OptConsTableSpace after INCLUDING INDEXES. I just
tested it and it didn't cause any conflicts which makes sense since like
options appear in the column list.

So the syntax would be

CREATE TABLE foo (..., LIKE bar INCLUDING INDEXES USING INDEX TABLESPACE foo_ts, ...)

Kind of verbose but nice that it's the same syntax as constraints.

Not sure how easy it would be to shoehorn into t he like processing, I could
look at that if you want.

--
  Gregory Stark
  EnterpriseDB          http://www.enterprisedb.com
  Ask me about EnterpriseDB's RemoteDBA services!

Gregory Stark <stark@enterprisedb.com> writes:
> So the syntax would be

> CREATE TABLE foo (..., LIKE bar INCLUDING INDEXES USING INDEX TABLESPACE foo_ts, ...)

This (presumably) forces all the indexes into the same tablespace,
so I don't find it to be a complete solution, just a wart.

We could get the same behavior with much less code if we redefined
LIKE to not try to copy the source indexes' tablespace(s).  Then,
the user would set default_tablespace to get the effect of the
USING clause.

That would also make LIKE entirely free of tablespace permissions
hazards, so I'm starting to think more and more that that's really the
right definition.

            regards, tom lane