[PATCH] Add GUCs for predicate lock promotion thresholds
Hi hackers,
I have a workload using SSI that causes a lot of tuple predicate to be
promoted to page locks. This causes a lot of spurious serialisability
errors, since the promotion happens at once three tuples on a page are
locked, and the affected tables have 30-90 tuples per page.
PredicateLockPromotionThreshold() has the following comment:
* 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.
Attached is a patch that does the "at least" part of this.
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.
Cheers,
Ilmari
Attachments:
0001-Add-GUCs-for-predicate-lock-promotion-thresholds.patchtext/x-diffDownload+78-8
On Wed, Dec 14, 2016 at 8:16 AM, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
Attached is a patch
Please add this to the open CommitFest to ensure that it is reviewed.
http://commitfest.postgresql.org/action/commitfest_view/open
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@gmail.com> writes:
On Wed, Dec 14, 2016 at 8:16 AM, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:Attached is a patch
Please add this to the open CommitFest to ensure that it is reviewed.
Done.
https://commitfest.postgresql.org/12/910/
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
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.
FWIW, interdependent GUC defaults are generally unworkable.
See commit a16d421ca and the sorry history that led up to it.
I think you could make it work as a fraction, though.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jan 5, 2017 at 12:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
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.FWIW, interdependent GUC defaults are generally unworkable.
See commit a16d421ca and the sorry history that led up to it.
I think you could make it work as a fraction, though.
I think that, ultimately, both an fraction of actual (the number of
tuples on a page or the number of pages in a relation) *and* an
absolute number (as this patch implements) could be useful. The
former would be more "adaptable" -- providing reasonable behavior
for different sized tuples and different sized tables, while the
latter would prevent a single table with very small tuples or a lot
of pages from starving all other uses. This patch implements the
easier part, and is likely to be very useful to many users without
the other part, so I think it is worth accepting as a step in the
right direction, and consistent with not letting the good be the
enemy of the perfect.
There are a couple minor formatting issues I can clean up as
committer, but there are a couple more substantive things to note.
(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.
(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.
Other than that, I think it is in good shape and I am would feel
good about committing it. Of course, if there are any objections
to it, I will not go ahead without reaching consensus. I am on
vacation next week, so I would not do so before then; in fact I may
not have a chance to respond to any comments before then. If the
author wishes to make these changes, great; otherwise I can do it
before commit.
I will mark this Ready for Committer.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
A couple things occurred to me after hitting "Send".
In addition to the prior 2 points:
(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.
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. In any event, it seems more like
autovacuum_work_mem or autovacuum_vacuum_cost_limit than like
effective_cache_size.
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
0001-Add-GUCs-for-predicate-lock-promotion-thresholds-v2.patchtext/x-diffDownload+91-16
On Mon, Jan 23, 2017 at 9:50 AM, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
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
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 23, 2017 at 9:50 AM, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
[new patch addressing issues raised]
Thanks!
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
It occurred to me that it would make sense to allow these settings
to be attached to a database or table (though *not* a role). I'll
look at that when I get back if you don't get to it first.
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.
I will need some time to consider that....
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jan 24, 2017 at 3:32 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
I will need some time to consider that....
Moved to CF 2017-03 for now. The last patch sent still applies.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@gmail.com> writes:
It occurred to me that it would make sense to allow these settings
to be attached to a database or table (though *not* a role). I'll
look at that when I get back if you don't get to it first.
Attached is a draft patch (no docs or tests) on top of the previous one,
adding reloptions for the per-relation and per-page thresholds. That
made me realise that the corresponding GUCs don't need to be PGC_SIGHUP,
but could be PGC_SUSET or PGC_USERSET. I'll adjust the original patch
if you agree. I have not got around around to adding per-database
versions of the setting, but could have a stab at that.
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl
Attachments:
0002-Add-max_pred_locks_per_-relation-page-reloptions.patchtext/x-diffDownload+71-14
On Mon, Feb 27, 2017 at 7:35 AM, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
Kevin Grittner <kgrittn@gmail.com> writes:
It occurred to me that it would make sense to allow these settings
to be attached to a database or table (though *not* a role). I'll
look at that when I get back if you don't get to it first.Attached is a draft patch (no docs or tests) on top of the previous one,
adding reloptions for the per-relation and per-page thresholds. That
made me realise that the corresponding GUCs don't need to be PGC_SIGHUP,
but could be PGC_SUSET or PGC_USERSET. I'll adjust the original patch
if you agree. I have not got around around to adding per-database
versions of the setting, but could have a stab at that.
Unfortunately, I was unable to get the follow-on patch to allow
setting by relation into a shape I liked. Let's see what we can do
for the next release. The first patch was applied with only very
minor tweaks. I realize that nothing would break if individual
users could set their granularity thresholds on individual
connections, but as someone who has filled the role of DBA, the
thought kinda made my skin crawl. I left it as PGC_SIGHUP for now;
we can talk about loosening that up later, but I think we should
have one or more use-cases that outweigh the opportunities for
confusion and bad choices by individual programmers to justify that.
--
Kevin Grittner
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@gmail.com> writes:
Unfortunately, I was unable to get the follow-on patch to allow
setting by relation into a shape I liked. Let's see what we can do
for the next release.
Okay, I'll try and crete a more comprehensive version of it for the next
commitfest.
The first patch was applied with only very minor tweaks.
Thanks!
I realize that nothing would break if individual users could set their
granularity thresholds on individual connections, but as someone who
has filled the role of DBA, the thought kinda made my skin crawl. I
left it as PGC_SIGHUP for now; we can talk about loosening that up
later, but I think we should have one or more use-cases that outweigh
the opportunities for confusion and bad choices by individual
programmers to justify that.
I agree. The committed version is fine for my current use case.
- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers