Re: pg_prewarm

I hope it's not too late for another reviewer to pitch in :) I have let
some time pass between previous reviews so that I can give this patch a
fresh look, and not be tainted by what the other reviews said, so I may be
repeating a few things already covered by other reviewers. I haven't
performed any tests on this (yet) because I have seen a few other posts
which show that other people have already used this utility. When I get
time next, I will try to develop some useful scripts around this function
to help in hibernation-like feature, and also the speeding-up of recovery
when combined with xlogdump as previously suggested in this thread.

This is my first review of a patch, and I just realized after finishing the
review that this does not qualify as proper review as documented in
"Reviewing a patch" wiki page. But this is an ungodly hour for me, so
cannot spend more time on it right now. These are just the notes I took
while doing the code review. Hope it helps in improving the patch.

Applying the patch on master HEAD needs some hunk adjustments, but I didn't
see anything out of place during the review.

<snip>
patching file doc/src/sgml/contrib.sgml
Hunk #1 succeeded at 128 with fuzz 2 (offset 12 lines).
patching file doc/src/sgml/filelist.sgml
Hunk #1 succeeded at 125 (offset 1 line).
</snip>

I think it'll be useful to provide some overloaded functions, or provide
some defaults. Here's what I think are sensible defaults. Note that I have
moved prefetch_type parameter from position 3 to 2; I think prefetch_type
will see more variations in the field than fork (which will be 'main' most
of the times).

pg_prewarm(relation)
defaults: type (prefetch/read), fork (main), first_block(null),
last_block(null)
pg_prewarm(relation, type)
defaults: fork (main), first_block(null), last_block(null)
pg_prewarm(relation, type, fork)
defaults: first_block(null), last_block(null)
pg_prewarm(relation, type, fork, first_block)
defaults: last_block(null)
pg_prewarm(relation, type, fork, first_block, last_block) -- already
provided in the patch.

Should we provide a capability to prewarm all forks of a relation by
allowing a pseudo fork by the name 'all'. I don't see much use for it, but
others might.

We should consider making this error 'fork \"%s\" does not exist for this
relation' into a warning, unless we can guarantee that forks always exist
for a relation; for eg. if Postgres delays creating the 'vm' fork until
first vacuum on a relation, then a script that simply tries to prewarm all
forks of a relation will encounter errors, which may stop the script
processing altogether and lead to not prewarming the rest of the relations
the user might have wanted to.

Does the regclass conversion of first parameter respects the USAGE
privileges on the schema? Does it need to if the user has SELECT privilege
on it?

Do not raise error when the provided last_block number is larger than total
blocks in the relation. This may have been caused by a truncate operation
since the user initiated the function. Just raise a warning and use total
blocks as last_block.

Make this error "prefetch is not supported by this build" into a warning.
This will let the scripts developed on one build at least complete
successfully on a different build.

Check for last_block < 0. Better yet, raise an error if last_block <
first_block.

In PREWARM_BUFFER case, raise a warning and load only (end_buffer -
begin_buffer) number of buffers if ((end_buffer - begin_buffer) >
shared_buffers). This will help prewarming complete quicker if the relation
is too big for the shared_buffers to accommodate, and also let the user
know that she needs to tweak the prewarming method. It may help to perform
PREWARM_READ on the rest of the buffers.

In the docs, where it says
"so it is possible
+ for other system activity may evict the newly prewarmed"

the word 'for' seems out of place. Replace it with 'that'.

Best regards,

On Sat, Jul 14, 2012 at 5:33 AM, Cédric Villemain <cedric(at)2ndquadrant(dot)com>wrote:

> > c) isn't necessarily safe in production (I've crashed Linux with Fincore
> > in the recent past).
>
> fincore is another soft, please provide a bugreport if you hit issue with
> pgfincore, I then be able to fix it and all can benefit.
>
> --
> Cédric Villemain +33 (0)6 20 30 22 52
> http://2ndQuadrant.fr/
> PostgreSQL: Support 24x7 - Développement, Expertise et Formation
>

--
Gurjeet Singh