Re: [HACKERS] [PATCH] Add GUCs for predicate lock promotion thresholds

Kevin Grittner <kgrittn@gmail.com> writes:

> (1)  The new GUCs are named max_pred_locks_per_*, but the
> implementation passes them unmodified from a function specifying at
> what count the lock should be promoted.  We either need to change
> the names or (probably better, only because I can't think of a good
> name for the other way) return the GUC value + 1 from the function.
> Or maybe change the function name and how the returned number is
> used, if we care about eliminating the overhead of the increment
> instruction.  That actually seems least confusing.

I've done the latter, and renamed the function MaxPredicateChildLocks.
I also changed the default for max_pred_locks_per_page from 3 to 4 and
the minimum to 0, to keep the effective thresholds the same.

> (2)  The new GUCs are declared and documented to be set only on
> startup.  This was probably just copied from
> max_pred_locks_per_transaction, but that sets an allocation size
> while these don't.  I think these new GUCs should be PGC_SIGHUP,
> and documented to change on reload.

Fixed.

> (3)  The documentation for max_pred_locks_per_relation needs a fix.
> Both page locks and tuple locks for the relation are counted toward
> the limit.

Fixed.

> In releases prior to this patch, max_pred_locks_per_relation was
> calculated as "max_pred_locks_per_transaction / 2".  I know that
> people have sometimes increased max_pred_locks_per_transaction
> specifically to raise the limit on locks within a relation before
> the promotion to relation granularity occurs.  It seems kinda
> anti-social not to support a special value for continuing that
> behavior or, if we don't want to do that, at least modifying
> pg_upgrade to set max_pred_locks_per_relation to the value that was
> in effect in the old version.

This is exactly what we've been doing at my workplace, and I mentioned
this in my original email:

>> ilmari@ilmari.org (Dagfinn Ilmari Manns�ker) writes:
>>> One thing I don't like about this patch is that if a user has
>>> increased max_pred_locks_per_transaction, they need to set
>>> max_pred_locks_per_relation to half of that to retain the current
>>> behaviour, or they'll suddenly find themselves with a lot more
>>> relation locks.  If it's possible to make a GUCs default value
>>> dependent on the value of another, that could be a solution.
>>> Otherwise, the page lock threshold GUC could be changed to be
>>> expressed as a fraction of max_pred_locks_per_transaction, to keep
>>> the current behaviour.
>
>> Another option would be to have a special sentinel (e.g. -1) which is
>> the default, and keeps the current behaviour.

The attached updated patch combines the two behaviours.  Positive values
mean an absolute number of locks, while negative values mean
max_predicate_locks_per_transaction / -max_predicate_locks_per_relation.
Making the default value -2 keeps backwards compatibility.

Alternatively we could have a separate setting for the fraction (which
could then be a floating-point number, for finer control), and use the
absolute value if that is zero.

Regards,

Ilmari

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

From 12c55660e9235e100b8802645eb8aaef7683888f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 12 Dec 2016 17:57:33 +0000
Subject: [PATCH] Add GUCs for predicate lock promotion thresholds

This addresses part of the TODO comment for predicate lock promotion
threshold, by making them configurable.  The default values are the same
as what used to be hardcoded.
---
 doc/src/sgml/config.sgml                      | 39 +++++++++++++++++++++++++++
 doc/src/sgml/mvcc.sgml                        |  4 ++-
 src/backend/storage/lmgr/predicate.c          | 37 +++++++++++++++----------
 src/backend/utils/misc/guc.c                  | 22 +++++++++++++++
 src/backend/utils/misc/postgresql.conf.sample |  2 ++
 src/include/storage/predicate.h               |  2 ++
 6 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index fb5d6473ef..1f944aab1c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7255,6 +7255,45 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-max-pred-locks-per-relation" xreflabel="max_pred_locks_per_relation">
+      <term><varname>max_pred_locks_per_relation</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>max_pred_locks_per_relation</> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        This controls how many pages or tuples of a single relation can be
+        predicate-locked before the lock is promoted to covering the whole
+        relation.  Values greater than or equal to zero mean an absolute
+        limit, while negative values
+        mean <xref linkend="guc-max-pred-locks-per-transaction"> divided by
+        the absolute value of this setting.  The default is -2, which keeps
+        the behaviour from previous versions of <productname>PostgreSQL</>.
+        This parameter can only be set in the <filename>postgresql.conf</>
+        file or on the server command line.
+       </para>
+
+      </listitem>
+     </varlistentry>
+
+     <varlistentry id="guc-max-pred-locks-per-page" xreflabel="max_pred_locks_per_page">
+      <term><varname>max_pred_locks_per_page</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>max_pred_locks_per_page</> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        This controls how many rows on a single page can be predicate-locked
+        before the lock is promoted to covering the whole page.  The default
+        is 4.  This parameter can only be set in
+        the <filename>postgresql.conf</> file or on the server command line.
+       </para>
+
+      </listitem>
+     </varlistentry>
+
      </variablelist>
    </sect1>
 
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 306def4a15..4652cdf094 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -765,7 +765,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
        locks into a single relation-level predicate lock because the predicate
        lock table is short of memory, an increase in the rate of serialization
        failures may occur.  You can avoid this by increasing
-       <xref linkend="guc-max-pred-locks-per-transaction">.
+       <xref linkend="guc-max-pred-locks-per-transaction">,
+       <xref linkend="guc-max-pred-locks-per-relation"> and/or
+       <xref linkend="guc-max-pred-locks-per-page">.
       </para>
      </listitem>
      <listitem>
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 9183764ca7..ff50e5f9fb 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -355,6 +355,12 @@ static SERIALIZABLEXACT *OldCommittedSxact;
 /* This configuration variable is used to set the predicate lock table size */
 int            max_predicate_locks_per_xact;        /* set by guc.c */
 
+/* This configuration variable is used to decide when to upgrade a page lock to a relation lock */
+int            max_predicate_locks_per_relation;    /* set by guc.c */
+
+/* This configuration variable is used to decide when to upgrade a row lock to a page lock */
+int            max_predicate_locks_per_page;        /* set by guc.c */
+
 /*
  * This provides a list of objects in order to track transactions
  * participating in predicate locking.  Entries in the list are fixed size,
@@ -437,7 +443,7 @@ static void RestoreScratchTarget(bool lockheld);
 static void RemoveTargetIfNoLongerUsed(PREDICATELOCKTARGET *target,
                            uint32 targettaghash);
 static void DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag);
-static int    PredicateLockPromotionThreshold(const PREDICATELOCKTARGETTAG *tag);
+static int    MaxPredicateChildLocks(const PREDICATELOCKTARGETTAG *tag);
 static bool CheckAndPromotePredicateLockRequest(const PREDICATELOCKTARGETTAG *reqtag);
 static void DecrementParentLocks(const PREDICATELOCKTARGETTAG *targettag);
 static void CreatePredicateLock(const PREDICATELOCKTARGETTAG *targettag,
@@ -2118,28 +2124,31 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
 }
 
 /*
- * Returns the promotion threshold for a given predicate lock
- * target. This is the number of descendant locks required to promote
- * to the specified tag. Note that the threshold includes non-direct
- * descendants, e.g. both tuples and pages for a relation lock.
+ * Returns the promotion limit for a given predicate lock target.
+ * This is the max number of descendant locks allowed before
+ * promoting to the specified tag. Note that the limit includes
+ * non-direct descendants, e.g. both tuples and pages for a relation
+ * lock.
  *
  * TODO SSI: We should do something more intelligent about what the
- * thresholds are, either making it proportional to the number of
- * tuples in a page & pages in a relation, or at least making it a
- * GUC. Currently the threshold is 3 for a page lock, and
- * max_pred_locks_per_transaction/2 for a relation lock, chosen
+ * limits are, e.g. making it proportional to the number of tuples
+ * in a page & pages in a relation. Currently the default limit is
+ * 4 for a page lock, and 32 (half of the default value for
+ * max_pred_locks_per_transaction) for a relation lock, chosen
  * entirely arbitrarily (and without benchmarking).
  */
 static int
-PredicateLockPromotionThreshold(const PREDICATELOCKTARGETTAG *tag)
+MaxPredicateChildLocks(const PREDICATELOCKTARGETTAG *tag)
 {
     switch (GET_PREDICATELOCKTARGETTAG_TYPE(*tag))
     {
         case PREDLOCKTAG_RELATION:
-            return max_predicate_locks_per_xact / 2;
+            return max_predicate_locks_per_relation < 0
+                ? max_predicate_locks_per_xact / -max_predicate_locks_per_relation
+                : max_predicate_locks_per_relation;
 
         case PREDLOCKTAG_PAGE:
-            return 3;
+            return max_predicate_locks_per_page;
 
         case PREDLOCKTAG_TUPLE:
 
@@ -2194,8 +2203,8 @@ CheckAndPromotePredicateLockRequest(const PREDICATELOCKTARGETTAG *reqtag)
         else
             parentlock->childLocks++;
 
-        if (parentlock->childLocks >=
-            PredicateLockPromotionThreshold(&targettag))
+        if (parentlock->childLocks >
+            MaxPredicateChildLocks(&targettag))
         {
             /*
              * We should promote to this parent lock. Continue to check its
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5f43b1ec92..af7929ae7f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2186,6 +2186,28 @@ static struct config_int ConfigureNamesInt[] =
         NULL, NULL, NULL
     },
 
+    {
+        {"max_pred_locks_per_relation", PGC_SIGHUP, LOCK_MANAGEMENT,
+            gettext_noop("Sets the maximum number of predicate-locked pages per relation."),
+            gettext_noop("If more than this number of pages or tuples in the same relation are locked "
+                         "the lock is promoted to a relation level lock.")
+        },
+        &max_predicate_locks_per_relation,
+        -2, INT_MIN, INT_MAX,
+        NULL, NULL, NULL
+    },
+
+    {
+        {"max_pred_locks_per_page", PGC_SIGHUP, LOCK_MANAGEMENT,
+            gettext_noop("Sets the maximum number of predicate-locked rows per page."),
+            gettext_noop("If more than this number of rows on the same page are locked "
+                         "the lock is promoted to a page level lock.")
+        },
+        &max_predicate_locks_per_page,
+        4, 0, INT_MAX,
+        NULL, NULL, NULL
+    },
+
     {
         {"authentication_timeout", PGC_SIGHUP, CONN_AUTH_SECURITY,
             gettext_noop("Sets the maximum allowed time to complete client authentication."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 661b0fa9b6..fe2c7096dd 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -591,6 +591,8 @@
                     # (change requires restart)
 #max_pred_locks_per_transaction = 64    # min 10
                     # (change requires restart)
+#max_pred_locks_per_relation = -2    # negative values mean max_pred_locks_per_transaction / -N
+#max_pred_locks_per_page = 3            # min 0
 
 
 #------------------------------------------------------------------------------
diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h
index f996d8e818..8f9ea29917 100644
--- a/src/include/storage/predicate.h
+++ b/src/include/storage/predicate.h
@@ -22,6 +22,8 @@
  * GUC variables
  */
 extern int    max_predicate_locks_per_xact;
+extern int    max_predicate_locks_per_relation;
+extern int    max_predicate_locks_per_page;
 
 
 /* Number of SLRU buffers to use for predicate locking */
-- 
2.11.0


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