Re: Proposal : For Auto-Prewarm.

On Mon, Jun 5, 2017 at 7:58 AM, Rafia Sabih
<rafia(dot)sabih(at)enterprisedb(dot)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