Thread: Re: [HACKERS] Proposal : For Auto-Prewarm.
Hi,
--
Regards,
Neha Sharma
I have been testing this feature for a while and below are the observations for few scenarios.
Observation:
scenario 1: If we set pg_prewarm.dump_interval = -1.0,we get an additional warning message in logfile and instead of ending the task of auto-dump it executes successfully.
[centos@test-machine bin]$ more logfile
2017-06-06 08:39:53.127 GMT [21905] WARNING: invalid value for parameter "pg_prewarm.dump_interval": "-1.0"
2017-06-06 08:39:53.127 GMT [21905] HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".
2017-06-06 08:39:53.127 GMT [21905] LOG: listening on IPv6 address "::1", port 5432
2017-06-06 08:39:53.127 GMT [21905] LOG: listening on IPv4 address "127.0.0.1", port 5432
2017-06-06 08:39:53.130 GMT [21905] LOG: listening on Unix socket "/tmp/.s.PGSQL.5432"
2017-06-06 08:39:53.143 GMT [21906] LOG: database system was shut down at 2017-06-06 08:38:20 GMT
2017-06-06 08:39:53.155 GMT [21905] LOG: database system is ready to accept connections
2017-06-06 08:39:53.155 GMT [21912] LOG: autoprewarm has started
[centos@test-machine bin]$ ps -ef | grep prewarm
centos 21912 21905 0 08:39 ? 00:00:00 postgres: bgworker: autoprewarm
[centos@test-machine bin]$ ./psql postgres
psql (10beta1)
Type "help" for help.
postgres=# show pg_prewarm.dump_interval;
pg_prewarm.dump_interval
--------------------------
5min
(1 row)
scenario 2: If we set pg_prewarm.dump_interval = 0.0,we get an additional warning message in logfile and the message states that the task was started and the worker thread it is also active,but the dump_interval duration is set to default 5 min (300 sec) instead of 0.
[centos@test-machine bin]$ ps -ef | grep prewarm
centos 21980 21973 0 08:54 ? 00:00:00 postgres: bgworker: autoprewarm
[centos@test-machine bin]$ more logfile
2017-06-06 09:20:52.436 GMT [22223] WARNING: invalid value for parameter "pg_prewarm.dump_interval": "0.0"
2017-06-06 09:20:52.436 GMT [22223] HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".
2017-06-06 09:20:52.436 GMT [22223] LOG: listening on IPv6 address "::1", port 5432
2017-06-06 09:20:52.437 GMT [22223] LOG: listening on IPv4 address "127.0.0.1", port 5432
2017-06-06 09:20:52.439 GMT [22223] LOG: listening on Unix socket "/tmp/.s.PGSQL.5432"
2017-06-06 09:20:52.452 GMT [22224] LOG: database system was shut down at 2017-06-06 09:19:49 GMT
2017-06-06 09:20:52.455 GMT [22223] LOG: database system is ready to accept connections
2017-06-06 09:20:52.455 GMT [22230] LOG: autoprewarm has started
[centos@test-machine bin]$ ./psql postgres
psql (10beta1)
Type "help" for help.
postgres=# show pg_prewarm.dump_interval;
pg_prewarm.dump_interval
--------------------------
5min
(1 row)
On Mon, Jun 5, 2017 at 8:06 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jun 5, 2017 at 7:58 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
> I had a look at the patch from stylistic/formatting point of view,
> please find the attached patch for the suggested modifications.
Many of these seem worse, like these ones:
- * Quit if we've reached records for another database. Unless the
+ * Quit if we've reached records of another database. Unless the
- * When we reach a new relation, close the old one. Note, however,
- * that the previous try_relation_open may have failed, in which case
- * rel will be NULL.
+ * On reaching a new relation, close the old one. Note, that the
+ * previous try_relation_open may have failed, in which case rel will
+ * be NULL.
- * Try to open each new relation, but only once, when we first
- * encounter it. If it's been dropped, skip the associated blocks.
+ * Each relation is open only once at it's first encounter. If it's
+ * been dropped, skip the associated blocks.
Others are better, like these:
- (errmsg("could not continue autoprewarm worker is
already running under PID %d",
+ (errmsg("autoprewarm worker is already running under PID %d",
- * Start of prewarm per-database worker. This will try to load blocks of one
+ * Start prewarm per-database worker, which will load blocks of one
Others don't really seem better or worse, like:
- * Register a per-database worker to load new database's block. And
- * wait until they finish their job to launch next one.
+ * Register a per-database worker to load new database's block. Wait
+ * until they finish their job to launch next one.
IMHO, there's still a good bit of work needed here to make this sound
like American English. For example:
- * It is a bgworker which automatically records information about blocks
- * which were present in buffer pool before server shutdown and then
- * prewarm the buffer pool upon server restart with those blocks.
+ * It is a bgworker process that automatically records information about
+ * blocks which were present in buffer pool before server
shutdown and then
+ * prewarms the buffer pool upon server restart with those blocks.
This construction "It is a..." without a clear referent seems to be
standard in Indian English, but it looks wrong to English speakers
from other parts of the world, or at least to me.
+ * Since there could be at max one worker who could do a prewarm, hence,
+ * acquiring locks is not required before setting skip_prewarm_on_restart.
To me, adding a comma before hence looks like a significant
improvement, but the word hence itself seems out-of-place. Also, I'd
change "at max" to "at most" and maybe reword the sentence a little.
There's a lot of little things like this which I have tended be quite
strict about changing before commit; I occasionally wonder whether
it's really worth the effort. It's not really wrong, it just sounds
weird to me as an American.
--
Robert Haas
EnterpriseDB: 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
Regards,
Neha Sharma
On Tue, Jun 6, 2017 at 3:48 PM, Neha Sharma <neha.sharma@enterprisedb.com> wrote: > Hi, > > I have been testing this feature for a while and below are the observations > for few scenarios. > > Observation: > scenario 1: If we set pg_prewarm.dump_interval = -1.0,we get an additional > warning message in logfile and instead of ending the task of auto-dump it > executes successfully. > [centos@test-machine bin]$ more logfile > 2017-06-06 08:39:53.127 GMT [21905] WARNING: invalid value for parameter > "pg_prewarm.dump_interval": "-1.0" > 2017-06-06 08:39:53.127 GMT [21905] HINT: Valid units for this parameter > are "ms", "s", "min", "h", and "d". > > postgres=# show pg_prewarm.dump_interval; > pg_prewarm.dump_interval > -------------------------- > 5min > (1 row) > > scenario 2: If we set pg_prewarm.dump_interval = 0.0,we get an additional > warning message in logfile and the message states that the task was started > and the worker thread it is also active,but the dump_interval duration is > set to default 5 min (300 sec) instead of 0. > > [centos@test-machine bin]$ ps -ef | grep prewarm > centos 21980 21973 0 08:54 ? 00:00:00 postgres: bgworker: > autoprewarm > > [centos@test-machine bin]$ more logfile > 2017-06-06 09:20:52.436 GMT [22223] WARNING: invalid value for parameter > "pg_prewarm.dump_interval": "0.0" > 2017-06-06 09:20:52.436 GMT [22223] HINT: Valid units for this parameter > are "ms", "s", "min", "h", and "d". > postgres=# show pg_prewarm.dump_interval; > pg_prewarm.dump_interval > -------------------------- > 5min > (1 row) dump_interval is int type custom GUC variable so passing floating value is illegal here, but the reason we are only throwing a warning and setting it to default 5 mins is that of existing behavior @define_custom_variable /* * Assign the string value(s) stored in the placeholder to the real * variable. Essentially, we need to duplicateall the active and stacked * values, but with appropriate validation and datatype adjustment. * * If anassignment fails, we report a WARNING and keep going. We don't * want to throw ERROR for bad values, because it'd bollixthe add-on * module that's presumably halfway through getting loaded. In such cases * the default or previousstate will become active instead. */ /* First, apply the reset value if any */ if (pHolder->reset_val) (void) set_config_option(name, pHolder->reset_val, pHolder->gen.reset_scontext, pHolder->gen.reset_source, GUC_ACTION_SET, true, WARNING, false); I think this should be fine as it is defined behavior for all of the add-on modules. Please let me know if you have any suggestions.