Re: Proposal : For Auto-Prewarm.

On Tue, Jun 6, 2017 at 6:29 AM, Rafia Sabih
<rafia(dot)sabih(at)enterprisedb(dot)com> wrote:
> On Mon, Jun 5, 2017 at 8:06 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> 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
>>
> Why is it worse? I've always encountered using "records of database"
> and not "records for database". Anyhow, I tried reframing the sentence
> altogether.

My experience is the opposite. If I Google "from another database",
including the quotes, I get 516,000 hits; if I do the same using "of
another database", I get 110,000 hits. I think that means the former
usage is more popular, although obviously to some degree it's a matter
of taste.

+ * database, tablespace, filenode, forknum, blocknum in

The file doesn't contain the spaces you added here, which is probably
why Mithun did it as he did, but actually we don't really need this
level of detail in the file header comment anyway. I think you could
drop the specific mention of how blocks are identified.

+ * GUC variable to decide if autoprewarm worker has to be started when

if->whether? has to be->should be?

Actually this patch uses "if" in various places where I would use
"whether", but that's probably a matter of taste to some extent.

- * Start of prewarm per-database worker. This will try to load blocks of one
- * database starting from block info position state->prewarm_start_idx to
- * state->prewarm_stop_idx.
+ * Connect to the database and load the blocks of that database starting from
+ * the position state->prewarm_start_idx to state->prewarm_stop_idx.

That's a really good rephrasing. It's more compact and more
imperative. The only thing that seems a little off is that you say
"starting from" and then mention both the start and stop indexes.
Maybe say "between" instead.

- * Quit if we've reached records for another database. Unless the
- * previous blocks were of global objects which were combined with
- * next database's block infos.
+ * Quit if we've reached records for another database. If previous
+ * blocks are of some global objects, then continue pre-warming.

Good.

- * 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.
+ * As soon as we encounter a block of a new relation, close the old
+ * relation. Note, that rel will be NULL if try_relation_open failed
+ * previously, in that case there is nothing to close.

I wrote the original comment here, so you may not be too surprised to
here that I liked it as it was. Actually, your rewrite of the first
sentence seems like it might be better, but I think the second one is
not. By deleting "however", you've made the comma after "Note"
redundant, and by changing "in which case" to "in that case", you've
made a dependent clause into a comma splice. I hate comma splices.

https://en.wikipedia.org/wiki/Comma_splice

+ * $PGDATA/AUTOPREWARM_FILE to a dsm. Further, these BlockInfoRecords are

I would probably capitalize DSM here. There's no data structure
called dsm (lower-case) and we have other examples where it is
capitalized in documentation and comment text (see, e.g.
custom-scan.sgml).

+ * Since there could be at most one worker for prewarm, locking is not

could -> can?

- * For block info of a global object whose database will be 0
- * try to combine them with next non-zero database's block
- * infos to load.
+ * For the BlockRecordInfos of a global object, combine them
+ * with BlockRecordInfos of the next non-global object.

Good. Or even "Combine BlockRecordInfos for a global object with the
next non-global object", which I think is even more compact.

+ * If we are here with database having InvalidOid, then only
+ * BlockRecordInfos belonging to global objects exist. Since, we can
+ * not connect with InvalidOid skip prewarming for these objects.

If we reach this point with current_db == InvalidOid, ...

+ * Sub-routine to periodically call dump_now().

Every existing use of the word subroutine in our code based spells it
with no hyphen.

[rhaas pgsql]$ git grep -- Subroutine | wc -l
47
[rhaas pgsql]$ git grep -- Sub-routine | wc -l
0

Personally, I find this change worse, because calling something a
subroutine is totally generic, like saying that you met a "person" or
that something was "nice". Calling it a loop gives at least a little
bit of specific information.

+ * Call dum_now() at regular intervals defined by GUC variable dump_interval.

This change introduces an obvious typographical error.

- /* One last block info dump while postmaster shutdown. */
+ /* It's time for postmaster shutdown, let's dump for one last time. */

Comma splice.

+ /* Perform autoprewarm's task. */

Uninformative.

+ * A Common function to initialize BackgroundWorker structure.

Adding a period to the end is a good idea, but how about also fixing
the capitalization of "Common"? I think this is a common usage in
India, but not elsewhere.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company