Re: [HACKERS] [PATCH] Addition of some trivial auto vacuum logging
Alvaro Herrera writes: > Excerpts from Royce Ausburn's message of mar sep 27 21:28:26 -0300 2011: >> Tom's suggestion looks like it's less trivial that I can do just yet, but >> I'll take a look and ask for help if I need it. > It's not that difficult. Just do a search on "git log > src/backend/postmaster/pgstat.c" for patches that add a new column > somewhere. Yeah, I was just about to say the same thing. Find a previous patch that adds a feature similar to what you have in mind, and crib like mad. We've added enough stats counters over time that you should have several models to work from. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Addition of some trivial auto vacuum logging
Excerpts from Royce Ausburn's message of mar sep 27 21:28:26 -0300 2011: > Thanks for the tips guys. > > Just a question: is the utility great enough to warrant me working further > on this? I have no real desire to implement this particular feature -- I > simply saw an opportunity to cut my teeth on something easy. I'd be happy to > find something on the TODO list instead if this feature isn't really > worthwhile. First patches are always going to be small things. If you try to tackle something too large, chances are you'll never finish, despair utterly and throw yourself off a nearby bridge. Surely it's better to set realistic goals, start small and build slowly from there. > Tom's suggestion looks like it's less trivial that I can do just yet, but > I'll take a look and ask for help if I need it. It's not that difficult. Just do a search on "git log src/backend/postmaster/pgstat.c" for patches that add a new column somewhere. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Addition of some trivial auto vacuum logging
* Royce Ausburn (royce...@inomial.com) wrote: > Just a question: is the utility great enough to warrant me working further > on this? I have no real desire to implement this particular feature -- I > simply saw an opportunity to cut my teeth on something easy. I'd be happy to > find something on the TODO list instead if this feature isn't really > worthwhile. Seeing as how it's already got one committer willing to consider it (and that one tends to go the other direction on new features..), I'd definitely say it's worthwhile. That doesn't mean it's guaranteed to get in, but I'd put the probability above 75% given that feedback. That's pretty good overall. :) > Tom's suggestion looks like it's less trivial that I can do just yet, but > I'll take a look and ask for help if I need it. Don't let the notion of fiddling with the catalogs (system tables) discourage you.. It's really not all *that* bad. What you will need to figure out (and which I don't recall offhand..) is if you can just update those catalogs directly from VACUUM or if you need to go through the statistics collecter (which involves a bit of UDP communication, but hopefully we've abstracted that out enough that you won't have to deal with it directly really..). Looking at an existing example case where VACUUM is doing something that updates the stat tables (such as under the 'ANALYZE' option) will help out a lot, I'm sure. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Addition of some trivial auto vacuum logging
Thanks for the tips guys. Just a question: is the utility great enough to warrant me working further on this? I have no real desire to implement this particular feature -- I simply saw an opportunity to cut my teeth on something easy. I'd be happy to find something on the TODO list instead if this feature isn't really worthwhile. Tom's suggestion looks like it's less trivial that I can do just yet, but I'll take a look and ask for help if I need it. Cheers! --Royce On 28/09/2011, at 4:42 AM, Kevin Grittner wrote: > Royce Ausburn wrote: > >> As this is my first patch to postgresql, I'm expecting I've done > < something wrong. Please if you want me to fix something up, or >> just go away please say so ;) I appreciate that this is a trivial >> patch, and perhaps doesn't add value except for my very specific >> use case* feel free to ignore it =) > > Thanks for offering this to the community. I see you've already > gotten feedback on the patch, with a suggestion for a different > approach. Don't let that discourage you -- very few patches get in > without needing to be modified based on review and feedback. > > If you haven't already done so, please review this page and its > links: > > http://www.postgresql.org/developer/ > > Of particular interest is the Developer FAQ: > > http://wiki.postgresql.org/wiki/Developer_FAQ > > You should also be aware of the development cycle, which (when not > in feature freeze for beta testing) involves alternating periods of > focused development and code review (the latter called CommitFests): > > http://wiki.postgresql.org/wiki/CommitFest > > We are now in the midst of a CF, so it would be great if you could > join in that as a reviewer. Newly submitted patches should be > submitted to the "open" CF: > > http://commitfest.postgresql.org/action/commitfest_view/open > > You might want to consider what Tom said and submit a modified patch > for the next review cycle. > > Welcome! > > -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Addition of some trivial auto vacuum logging
Royce Ausburn wrote: > As this is my first patch to postgresql, I'm expecting I've done < something wrong. Please if you want me to fix something up, or > just go away please say so ;) I appreciate that this is a trivial > patch, and perhaps doesn't add value except for my very specific > use case* feel free to ignore it =) Thanks for offering this to the community. I see you've already gotten feedback on the patch, with a suggestion for a different approach. Don't let that discourage you -- very few patches get in without needing to be modified based on review and feedback. If you haven't already done so, please review this page and its links: http://www.postgresql.org/developer/ Of particular interest is the Developer FAQ: http://wiki.postgresql.org/wiki/Developer_FAQ You should also be aware of the development cycle, which (when not in feature freeze for beta testing) involves alternating periods of focused development and code review (the latter called CommitFests): http://wiki.postgresql.org/wiki/CommitFest We are now in the midst of a CF, so it would be great if you could join in that as a reviewer. Newly submitted patches should be submitted to the "open" CF: http://commitfest.postgresql.org/action/commitfest_view/open You might want to consider what Tom said and submit a modified patch for the next review cycle. Welcome! -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Addition of some trivial auto vacuum logging
Royce Ausburn writes: > The attached patch adds extra detail the the existing autovacuum log message > that is emitted when the log_autovacuum_min_duration threshold is met, > exposing the unremovable dead tuple count similar to what you get from VACUUM > VERBOSE. > Sample log output (my addition in bold): > LOG: automatic vacuum of table "test.public.test": index scans: 0 > pages: 0 removed, 5 remain > tuples: 0 removed, 1000 remain, 999 dead but not removable > system usage: CPU 0.00s/0.00u sec elapsed 0.00 sec This proposal seems rather ill-designed. In the first place, these numbers are quite unrelated to vacuum duration, and in the second place, most people who might need the info don't have that setting turned on anyway. I wonder whether it wouldn't be more helpful to have a pg_stat_all_tables column that reports the number of unremovable tuples as of the last vacuum. I've been known to object to more per-table stats counters in the past on the basis of space required, but perhaps this one would be worth its keep. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Addition of some trivial auto vacuum logging
Hi all,
I spent a bit of today diagnosing a problem where long held transactions were
preventing auto vacuum from doing its job. Eventually I had set
log_autovacuum_min_duration to 0 to see what was going on. Unfortunately it
wasn't until I tried a VACUUM VERBOSE that I found there were dead tuples not
being removed. Had the unremoveable tuple count been included in the
autovacuum log message life would have been a tiny bit easier.
I've been meaning for a while to dabble in postgres, so I thought this might be
a good trivial thing for me to improve. The attached patch adds extra detail
the the existing autovacuum log message that is emitted when the
log_autovacuum_min_duration threshold is met, exposing the unremovable dead
tuple count similar to what you get from VACUUM VERBOSE.
Sample log output (my addition in bold):
LOG: automatic vacuum of table "test.public.test": index scans: 0
pages: 0 removed, 5 remain
tuples: 0 removed, 1000 remain, 999 dead but not removable
system usage: CPU 0.00s/0.00u sec elapsed 0.00 sec
My patch adds another member to the LVRelStats struct named
undeleteable_dead_tuples. lazy_scan_heap() sets undeleteable_dead_tuples to
the value of lazy_scan_heap()'s local variable "nkeep", which is the same
variable that is used to emit the VACUUM VERBOSE unremoveable dead row count.
As this is my first patch to postgresql, I'm expecting I've done something
wrong. Please if you want me to fix something up, or just go away please say
so ;) I appreciate that this is a trivial patch, and perhaps doesn't add value
except for my very specific use case… feel free to ignore it =)
--Royce
diff --git a/src/backend/commands/vacuumlazy.c
b/src/backend/commands/vacuumlazy.c
index cf8337b..12f03d7 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -91,6 +91,7 @@ typedef struct LVRelStats
double scanned_tuples; /* counts only tuples on scanned pages
*/
double old_rel_tuples; /* previous value of pg_class.reltuples
*/
double new_rel_tuples; /* new estimated total # of tuples */
+ double undeleteable_dead_tuples; /* count of dead tuples not
yet removeable */
BlockNumber pages_removed;
double tuples_deleted;
BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */
@@ -256,7 +257,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
ereport(LOG,
(errmsg("automatic vacuum of table
\"%s.%s.%s\": index scans: %d\n"
"pages: %d removed, %d
remain\n"
- "tuples: %.0f removed,
%.0f remain\n"
+ "tuples: %.0f removed,
%.0f remain, %.0f dead but not removable\n"
"system usage: %s",
get_database_name(MyDatabaseId),
get_namespace_name(RelationGetNamespace(onerel)),
@@ -266,6 +267,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
vacrelstats->rel_pages,
vacrelstats->tuples_deleted,
new_rel_tuples,
+
vacrelstats->undeleteable_dead_tuples,
pg_rusage_show(&ru0;
}
}
@@ -829,6 +831,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
/* save stats for use later */
vacrelstats->scanned_tuples = num_tuples;
vacrelstats->tuples_deleted = tups_vacuumed;
+ vacrelstats->undeleteable_dead_tuples = nkeep;
/* now we can compute the new value for pg_class.reltuples */
vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false,
