Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Hm, strategically sprinkled CheckTableNotInUse() might do the trick?

> +1.  We can't reasonably make it work: the outer query already has its
> list of indexes that need to be inserted into.  Also, if you try to
> make the index via ALTER TABLE ADD CONSTRAINT in the trigger, that will
> give you "cannot ALTER TABLE "mytable" because it is being used by active
> queries in this session" because of the check in AlterTable().

Attached is a proposed patch that closes off this problem.  I've tested
it to the extent that it blocks Albe's example and passes check-world.

I'm unsure whether to back-patch or not; the main argument for not doing
so is that if any extensions are calling DefineIndex() directly, this
would be an API break for them.  Given what a weird case this is, I'm not
sure it's worth that.

A possible end-run around the API objection would be to not add an extra
argument to DefineIndex() in the back branches, but to use !is_alter_table
as the control condition.  That would work for the core callers, although
we might need a special case for bootstrap mode.  On the other hand,
thinking again about hypothetical third-party callers, it's possible that
that's not the right answer for them, in which case they'd be really in
trouble.  So I'm not that much in love with that answer.

> It doesn't seem terribly hard to fix the CREATE INDEX case to behave
> likewise, but I wonder how many other cases we've missed?

That remains an open question :-(

            regards, tom lane

diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index de3695c..2e1fef0 100644
*** a/src/backend/bootstrap/bootparse.y
--- b/src/backend/bootstrap/bootparse.y
*************** Boot_DeclareIndexStmt:
*** 323,328 ****
--- 323,329 ----
                                  $4,
                                  false,
                                  false,
+                                 false,
                                  true, /* skip_build */
                                  false);
                      do_end();
*************** Boot_DeclareUniqueIndexStmt:
*** 366,371 ****
--- 367,373 ----
                                  $5,
                                  false,
                                  false,
+                                 false,
                                  true, /* skip_build */
                                  false);
                      do_end();
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 4861799..c090279 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
*************** CheckIndexCompatible(Oid oldId,
*** 295,300 ****
--- 295,303 ----
   * 'is_alter_table': this is due to an ALTER rather than a CREATE operation.
   * 'check_rights': check for CREATE rights in namespace and tablespace.  (This
   *        should be true except when ALTER is deleting/recreating an index.)
+  * 'check_not_in_use': check for table not already in use in current session.
+  *        This should be true unless caller is holding the table open, in which
+  *        case the caller had better have checked it earlier.
   * 'skip_build': make the catalog entries but leave the index file empty;
   *        it will be filled later.
   * 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
*************** DefineIndex(Oid relationId,
*** 307,312 ****
--- 310,316 ----
              Oid indexRelationId,
              bool is_alter_table,
              bool check_rights,
+             bool check_not_in_use,
              bool skip_build,
              bool quiet)
  {
*************** DefineIndex(Oid relationId,
*** 405,410 ****
--- 409,423 ----
                   errmsg("cannot create indexes on temporary tables of other sessions")));

      /*
+      * Unless our caller vouches for having checked this already, insist that
+      * the table not be in use by our own session, either.  Otherwise we might
+      * fail to make entries in the new index (for instance, if an INSERT or
+      * UPDATE is in progress and has already made its list of target indexes).
+      */
+     if (check_not_in_use)
+         CheckTableNotInUse(rel, "CREATE INDEX");
+
+     /*
       * Verify we (still) have CREATE rights in the rel's namespace.
       * (Presumably we did when the rel was created, but maybe not anymore.)
       * Skip check if caller doesn't want it.  Also skip check if
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7959120..b61fda9 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATExecAddIndex(AlteredTableInfo *tab, Re
*** 6679,6684 ****
--- 6679,6685 ----
                            InvalidOid,    /* no predefined OID */
                            true, /* is_alter_table */
                            check_rights,
+                           false,    /* check_not_in_use - we did it already */
                            skip_build,
                            quiet);

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 1e941fb..a22fd53 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** ProcessUtilitySlow(ParseState *pstate,
*** 1329,1334 ****
--- 1329,1335 ----
                                      InvalidOid, /* no predefined OID */
                                      false,        /* is_alter_table */
                                      true,        /* check_rights */
+                                     true,        /* check_not_in_use */
                                      false,        /* skip_build */
                                      false);        /* quiet */

diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 79f3be3..5dd14b4 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
*************** extern ObjectAddress DefineIndex(Oid rel
*** 27,32 ****
--- 27,33 ----
              Oid indexRelationId,
              bool is_alter_table,
              bool check_rights,
+             bool check_not_in_use,
              bool skip_build,
              bool quiet);
  extern Oid    ReindexIndex(RangeVar *indexRelation, int options);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers