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

I have merged Rafia's patch for cosmetic changes. I have also fixed
some of the changes you have recommended over that. But kept few as it
is since Rafia's opinion was needed on that.

On Wed, Jun 7, 2017 at 5:57 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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.

-- Open.

>
> + *        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.

-- Fixed. It has been removed now.

> + * 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.

-- Fixed. I have changed from if to whether.

>
> - * 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.

-- It is a half-open interval so rewrote it within the notation [,)

>
> -         * 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

-- Open

> + * $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).

-- Fixed.

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

-- Fixed.

>
> -                 * 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.

-- Fixed.

>
> +         * 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, ...

--Fixed.

>
> + *        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.

-- Fixed. We call it a loop now.

>
> + * Call dum_now() at regular intervals defined by GUC variable dump_interval.
>
> This change introduces an obvious typographical error.

-- Fixed.

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

-- Open

>
> +    /* Perform autoprewarm's task. */
>
> Uninformative.

-- Fixed. I have removed same now.

>
> + *        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.

-- Fixed to common.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers