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

On Thu, Jan 5, 2017 at 12:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> ilmari(at)ilmari(dot)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