Re: pg_prewarm - Mailing list pgsql-hackers
| From | Gurjeet Singh |
|---|---|
| Subject | Re: pg_prewarm |
| Date | |
| Msg-id | CABwTF4V4vFaGrxQ0kAPGpCzbBhZ-v3-HmF5=GPMhysiyESbuGg@mail.gmail.com Whole thread Raw |
| In response to | Re: pg_prewarm (Cédric Villemain <cedric@2ndquadrant.com>) |
| List | pgsql-hackers |
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.
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@2ndquadrant.com> wrote:
> c) isn't necessarily safe in production (I've crashed Linux with Fincorefincore is another soft, please provide a bugreport if you hit issue with
> in the recent past).
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
pgsql-hackers by date: