Re: [HACKERS] Proposal : For Auto-Prewarm.

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