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