On Thu, Jan 20, 2011 at 09:48:12PM -0500, Robert Haas wrote:
> On Thu, Jan 20, 2011 at 6:19 PM, Noah Misch <n...@leadboat.com> wrote:
> > On Thu, Jan 20, 2011 at 09:36:11PM +0000, Simon Riggs wrote:
> >> I agree that the DDL behaviour is wrong and should be fixed. Thank you
> >> for championing that alternative view.
> >>
> >> Swapping based upon names only works and is very flexible, much more so
> >> than EXCHANGE could be.
> >>
> >> A separate utility might be worth it, but the feature set of that should
> >> be defined in terms of correctly-working DDL behaviour. It's possible
> >> that no further requirement exists. I remove my own patch from
> >> consideration for this release.
> >>
> >> I'll review your patch and commit it, problems or objections excepted. I
> >> haven't looked at it in any detail.
> >
> > Thanks. ?I wouldn't be very surprised if that patch is even the wrong way to
> > achieve these semantics, but it's great that we're on the same page as to 
> > which
> > semantics they are.
> 
> I think Noah's patch is a not a good idea, because it will result in
> calling RangeVarGetRelid twice even in the overwhelmingly common case
> where no relevant invalidation occurs.  That'll add several syscache
> lookups per table to very common operations.

Valid concern.

[Refresher: this was a patch to improve behavior for this test case:

        psql -c "CREATE TABLE t AS VALUES ('old'); CREATE TABLE new_t AS VALUES 
('new')"
        psql -c 'SELECT pg_sleep(2) FROM t' & # block the ALTER or DROP briefly
        sleep 1                                                           # 
give prev time to take AccessShareLock

        # Do it this way, and the next SELECT gets data from the old table.
        psql -c 'ALTER TABLE t RENAME TO old_t; ALTER TABLE new_t RENAME TO t' &
        # Do it this way, and get: ERROR:  could not open relation with OID 
41380
        #psql -c 'DROP TABLE t; ALTER TABLE new_t RENAME TO t' &

        psql -c 'SELECT * FROM t'               # I get 'old' or an error, 
never 'new'.
        psql -c 'DROP TABLE IF EXISTS t, old_t, new_t'

It did so by rechecking the RangeVar->oid resolution after locking the found
relation, by which time concurrent DDL could no longer be interfering.]

I benchmarked the original patch with this function:

        Datum
        nmtest(PG_FUNCTION_ARGS)
        {
                int32           n = PG_GETARG_INT32(0);
                int                     i;
                RangeVar   *rv = makeRangeVar(NULL, "pg_am", 0);
        
                for (i = 0; i < n; ++i)
                {
                        Relation        r = relation_openrv(rv, 
AccessShareLock);
                        relation_close(r, AccessShareLock);
                }
                PG_RETURN_INT32(4);
        }

(Releasing the lock before transaction end makes for an unrealistic benchmark,
but so would opening the same relation millions of times in a single
transaction.  I'm trying to isolate the cost that would be spread over
millions of transactions opening relations a handful of times.  See attached
shar archive for a complete extension wrapping that test function.)

Indeed, the original patch slowed it by about 50%.  I improved the patch,
adding a global SharedInvalidMessageCounter to increment as we process
messages.  If this counter does not change between the RangeVarGetRelid() call
and the post-lock AcceptInvalidationMessages() call, we can skip the second
RangeVarGetRelid() call.  With the updated patch, I get these timings (in ms)
for runs of "SELECT nmtest(10000000)":

master: 19697.642, 20087.477, 19748.995
patched: 19723.716, 19879.538, 20257.671

In other words, no significant difference.  Since the patch removes the
no-longer-needed pre-lock call to AcceptInvalidationMessages(), changing to
"relation_close(r, NoLock)" in the test case actually reveals a 15%
performance improvement.  Granted, nothing to get excited about in light of
the artificiality.

This semantic improvement would be hard to test with the current pg_regress
suite, so I do not include any test case addition in the patch.  If
sufficiently important, it could be done with isolationtester.

Thanks,
nm
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 01a492e..63537fd 100644
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***************
*** 979,1004 **** relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
  {
        Oid                     relOid;
  
!       /*
!        * Check for shared-cache-inval messages before trying to open the
!        * relation.  This is needed to cover the case where the name 
identifies a
!        * rel that has been dropped and recreated since the start of our
!        * transaction: if we don't flush the old syscache entry then we'll 
latch
!        * onto that entry and suffer an error when we do RelationIdGetRelation.
!        * Note that relation_open does not need to do this, since a relation's
!        * OID never changes.
!        *
!        * We skip this if asked for NoLock, on the assumption that the caller 
has
!        * already ensured some appropriate lock is held.
!        */
!       if (lockmode != NoLock)
!               AcceptInvalidationMessages();
! 
!       /* Look up the appropriate relation using namespace search */
!       relOid = RangeVarGetRelid(relation, false);
  
        /* Let relation_open do the rest */
!       return relation_open(relOid, lockmode);
  }
  
  /* ----------------
--- 979,989 ----
  {
        Oid                     relOid;
  
!       /* Look up and lock the appropriate relation using namespace search */
!       relOid = RangeVarLockRelid(relation, lockmode, false);
  
        /* Let relation_open do the rest */
!       return relation_open(relOid, NoLock);
  }
  
  /* ----------------
***************
*** 1014,1043 **** try_relation_openrv(const RangeVar *relation, LOCKMODE 
lockmode)
  {
        Oid                     relOid;
  
!       /*
!        * Check for shared-cache-inval messages before trying to open the
!        * relation.  This is needed to cover the case where the name 
identifies a
!        * rel that has been dropped and recreated since the start of our
!        * transaction: if we don't flush the old syscache entry then we'll 
latch
!        * onto that entry and suffer an error when we do RelationIdGetRelation.
!        * Note that relation_open does not need to do this, since a relation's
!        * OID never changes.
!        *
!        * We skip this if asked for NoLock, on the assumption that the caller 
has
!        * already ensured some appropriate lock is held.
!        */
!       if (lockmode != NoLock)
!               AcceptInvalidationMessages();
! 
!       /* Look up the appropriate relation using namespace search */
!       relOid = RangeVarGetRelid(relation, true);
  
        /* Return NULL on not-found */
        if (!OidIsValid(relOid))
                return NULL;
  
        /* Let relation_open do the rest */
!       return relation_open(relOid, lockmode);
  }
  
  /* ----------------
--- 999,1013 ----
  {
        Oid                     relOid;
  
!       /* Look up and lock the appropriate relation using namespace search */
!       relOid = RangeVarLockRelid(relation, lockmode, true);
  
        /* Return NULL on not-found */
        if (!OidIsValid(relOid))
                return NULL;
  
        /* Let relation_open do the rest */
!       return relation_open(relOid, NoLock);
  }
  
  /* ----------------
diff --git a/src/backend/catalog/namespindex 41e9299..771976b 100644
*** a/src/backend/catalog/namespace.c
--- b/src/backend/catalog/namespace.c
***************
*** 44,49 ****
--- 44,51 ----
  #include "parser/parse_func.h"
  #include "storage/backendid.h"
  #include "storage/ipc.h"
+ #include "storage/lmgr.h"
+ #include "storage/sinval.h"
  #include "utils/acl.h"
  #include "utils/builtins.h"
  #include "utils/guc.h"
***************
*** 285,290 **** RangeVarGetRelid(const RangeVar *relation, bool failOK)
--- 287,358 ----
  }
  
  /*
+  * RangeVarLockRelid
+  *            Like RangeVarGetRelid, but simulatenously acquire the specified 
lock on
+  *            the relation.  This works properly in the face of concurrent 
DDL that
+  *            may drop, create or rename relations.
+  *
+  * If the relation is not found and failOK = true, take no lock and return
+  * InvalidOid.  Otherwise, raise an error.
+  */
+ Oid
+ RangeVarLockRelid(const RangeVar *relation, LOCKMODE lockmode,
+                                 bool failOK)
+ {
+       int                     lastCounter;
+       Oid                     relOid1,
+                               relOid2;
+ 
+       /*
+        * First attempt.  If the caller requested NoLock, it already acquired 
an
+        * appropriate lock and has called AcceptInvalidationMessages() since
+        * doing so.  In this case, our first search is always correct, and we
+        * degenerate to behave exactly like RangeVarGetRelid().
+        */
+       lastCounter = SharedInvalidMessageCounter;
+       relOid1 = RangeVarGetRelid(relation, failOK);
+       if (lockmode == NoLock)
+               return relOid1;
+ 
+       /*
+        * By the time we acquire the lock, our RangeVar may denote a different
+        * relation or no relation at all.  In particular, this can happen when
+        * the lock acquisition blocks on a transaction performing DROP or ALTER
+        * TABLE RENAME.  However, once and while we do hold a lock of any 
level,
+        * we can count on the name of the found relation remaining stable.
+        *
+        * Even so, DDL activity could cause an object in a schema earlier in 
the
+        * search path to mask our original selection undetected.  No current 
lock
+        * would prevent that.  We let the user worry about it, such as by 
taking
+        * additional explicit locks.
+        */
+       do
+       {
+               /* Not-found is always final. */
+               if (!OidIsValid(relOid1))
+                       return relOid1;
+ 
+               /*
+                * LockRelationOid also calls AcceptInvalidationMessages() to 
make
+                * recent DDL effects visible, if needed.  Finding none, we're 
done.
+                */
+               LockRelationOid(relOid1, lockmode);
+               if (lastCounter == SharedInvalidMessageCounter)
+                       break;
+               else
+                       lastCounter = SharedInvalidMessageCounter;
+ 
+               /* Some invalidation messages arrived; search again. */
+               relOid2 = relOid1;
+               relOid1 = RangeVarGetRelid(relation, failOK);
+ 
+               /* Done when our RangeVar denotes the same relation we locked. 
*/
+       } while (relOid1 != relOid2);
+ 
+       return relOid1;
+ }
+ 
+ /*
   * RangeVarGetCreationNamespace
   *            Given a RangeVar describing a to-be-created relation,
   *            choose which namespace to create it in.
diff --git a/src/backend/storage/ipc/sindex 9ab16b1..9b1ec82 100644
*** a/src/backend/storage/ipc/sinval.c
--- b/src/backend/storage/ipc/sinval.c
***************
*** 22,27 ****
--- 22,30 ----
  #include "utils/inval.h"
  
  
+ unsigned SharedInvalidMessageCounter;
+ 
+ 
  /*
   * Because backends sitting idle will not be reading sinval events, we
   * need a way to give an idle backend a swift kick in the rear and make
***************
*** 90,95 **** ReceiveSharedInvalidMessages(
--- 93,99 ----
        {
                SharedInvalidationMessage *msg = &messages[nextmsg++];
  
+               SharedInvalidMessageCounter++;
                invalFunction(msg);
        }
  
***************
*** 106,111 **** ReceiveSharedInvalidMessages(
--- 110,116 ----
                {
                        /* got a reset message */
                        elog(DEBUG4, "cache state reset");
+                       SharedInvalidMessageCounter++;
                        resetFunction();
                        break;                          /* nothing more to do */
                }
***************
*** 118,123 **** ReceiveSharedInvalidMessages(
--- 123,129 ----
                {
                        SharedInvalidationMessage *msg = &messages[nextmsg++];
  
+                       SharedInvalidMessageCounter++;
                        invalFunction(msg);
                }
  
diff --git a/src/backend/storage/lmgr/lindex 859b385..90b2ecc 100644
*** a/src/backend/storage/lmgr/lmgr.c
--- b/src/backend/storage/lmgr/lmgr.c
***************
*** 81,90 **** LockRelationOid(Oid relid, LOCKMODE lockmode)
        /*
         * Now that we have the lock, check for invalidation messages, so that 
we
         * will update or flush any stale relcache entry before we try to use 
it.
!        * We can skip this in the not-uncommon case that we already had the 
same
!        * type of lock being requested, since then no one else could have
!        * modified the relcache entry in an undesirable way.  (In the case 
where
!        * our own xact modifies the rel, the relcache update happens via
         * CommandCounterIncrement, not here.)
         */
        if (res != LOCKACQUIRE_ALREADY_HELD)
--- 81,91 ----
        /*
         * Now that we have the lock, check for invalidation messages, so that 
we
         * will update or flush any stale relcache entry before we try to use 
it.
!        * RangeVarLockRelid() specifically relies on us for this.  We can skip
!        * this in the not-uncommon case that we already had the same type of 
lock
!        * being requested, since then no one else could have modified the
!        * relcache entry in an undesirable way.  (In the case where our own 
xact
!        * modifies the rel, the relcache update happens via
         * CommandCounterIncrement, not here.)
         */
        if (res != LOCKACQUIRE_ALREADY_HELD)
diff --git a/src/include/catalog/namesindex 5360096..7e64952 100644
*** a/src/include/catalog/namespace.h
--- b/src/include/catalog/namespace.h
***************
*** 15,20 ****
--- 15,21 ----
  #define NAMESPACE_H
  
  #include "nodes/primnodes.h"
+ #include "storage/lock.h"
  
  
  /*
***************
*** 48,53 **** typedef struct OverrideSearchPath
--- 49,56 ----
  
  
  extern Oid    RangeVarGetRelid(const RangeVar *relation, bool failOK);
+ extern Oid    RangeVarLockRelid(const RangeVar *relation, LOCKMODE lockmode,
+                                 bool failOK);
  extern Oid    RangeVarGetCreationNamespace(const RangeVar *newRelation);
  extern Oid    RangeVarGetAndCheckCreationNamespace(const RangeVar 
*newRelation);
  extern Oid    RelnameGetRelid(const char *relname);
diff --git a/src/include/storage/sinvaindex e9ce025..a90aa2d 100644
*** a/src/include/storage/sinval.h
--- b/src/include/storage/sinval.h
***************
*** 116,121 **** typedef union
--- 116,125 ----
  } SharedInvalidationMessage;
  
  
+ /* Counter of messages processed; may overflow. */
+ extern unsigned SharedInvalidMessageCounter;
+ 
+ 
  extern void SendSharedInvalidMessages(const SharedInvalidationMessage *msgs,
                                                  int n);
  extern void ReceiveSharedInvalidMessages(

Attachment: nmtest-relation_openrv.shar
Description: Unix shell archive

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

Reply via email to