Hello, Thank you for your comments. Please find attached patch addressing following comments ,
>- duplicate_oids error in HEAD. Check. >- a compiler warning: >pgstat.c:2898: warning: no previous prototype for ‘pgstat_reset_activityflag’ Check. >One more change you could do is 's/activityflag/activity_flag/g', Check. >Type of the flag of vacuum activity. The flag variable is an integer to incorporate more commands in future. >Type of st_progress_param and so. st_progress_param is also given a generic name to incorporate different parameters reported from various commands. >st_progress_param_float is currently totally useless. Float parameter has currently been removed from the patch. >Definition of progress_message. >The definition of progress_message in lazy_scan_heap is "char >[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM]" which looks to be >inversed. Corrected. >The current code subtracts the number of blocks when >skipping_all_visible_blocks is set in two places. But I think >it is enough to decrement when skipping. In both the places, the pages are being skipped hence the total count was decremented. >He suggested to keep total_heap_pages fixed while adding number >of skipped pages to that of scanned pages. This has been done in the attached. >snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s", > get_namespace_name(RelationGetNamespace(rel)), > relname); Check. The previous implementation used to add total number of pages across all indexes of a relation to total_index_pages in every scan of indexes to account for total pages scanned. Thus, it was equal to number of scans * total_index_pages. In the attached patch, total_index_pages reflects total number of pages across all indexes of a relation. And the column to report passes through indexes (phase 2) has been added to account for number of passes for index and heap vacuuming. Number of scanned index pages is reset at the end of each pass. This makes the reporting clearer. The percent complete does not account for index pages. It just denotes percentage of heap scanned. >Spotted a potential oversight regarding report of scanned_pages. It >seems pages that are skipped because of not getting a pin, being new, >being empty could be left out of the progress equation. Corrected. >It's better to >report that some other way, like use one of the strings to report a >"phase" of processing that we're currently performing. Has been included in the attached. Some more comments need to be addressed which include name change of activity flag, reporting only changed parameters to shared memory, ACTIVITY_IS_VACUUM flag being set unnecessarily for ANALYZE and FULL commands ,documentation for new view. Also, finer grain reporting from indexes and heap truncate phase is yet to be incorporated into the patch Thank you, Rahila Syed
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index ccc030f..d53833e 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -631,6 +631,19 @@ CREATE VIEW pg_stat_activity AS WHERE S.datid = D.oid AND S.usesysid = U.oid; +CREATE VIEW pg_stat_vacuum_progress AS + SELECT + S.pid, + S.table_name, + S.phase, + S.total_heap_pages, + S.scanned_heap_pages, + S.percent_complete, + S.total_index_pages, + S.scanned_index_pages, + S.index_scan_count + FROM pg_stat_get_vacuum_progress() AS S; + CREATE VIEW pg_stat_replication AS SELECT S.pid, diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7c4ef58..e27a8f3 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -284,6 +284,7 @@ vacuum(int options, RangeVar *relation, Oid relid, VacuumParams *params, VacuumPageMiss = 0; VacuumPageDirty = 0; + pgstat_report_activity_flag(ACTIVITY_IS_VACUUM); /* * Loop to process each selected relation. */ @@ -325,6 +326,7 @@ vacuum(int options, RangeVar *relation, Oid relid, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + pgstat_reset_activity_flag(); PG_RE_THROW(); } PG_END_TRY(); @@ -355,6 +357,7 @@ vacuum(int options, RangeVar *relation, Oid relid, VacuumParams *params, vac_update_datfrozenxid(); } + pgstat_reset_activity_flag(); /* * Clean up working storage --- note we must do this after * StartTransactionCommand, else we might be trying to delete the active diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 2429889..1c74b51 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -439,9 +439,14 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, Relation *Irel, int nindexes, bool scan_all) { BlockNumber nblocks, - blkno; + blkno, + total_heap_pages, + scanned_heap_pages = 0, + total_index_pages = 0, + scanned_index_pages = 0; HeapTupleData tuple; char *relname; + char *schemaname; BlockNumber empty_pages, vacuumed_pages; double num_tuples, @@ -456,14 +461,20 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, bool skipping_all_visible_blocks; xl_heap_freeze_tuple *frozen; StringInfoData buf; + uint32 progress_param[N_PROGRESS_PARAM]; + char progress_message[N_PROGRESS_PARAM][PROGRESS_MESSAGE_LENGTH]; + const char *phase1="Scanning Heap"; + const char *phase2="Vacuuming Index and Heap"; pg_rusage_init(&ru0); relname = RelationGetRelationName(onerel); + schemaname = get_namespace_name(RelationGetNamespace(onerel)); ereport(elevel, (errmsg("vacuuming \"%s.%s\"", get_namespace_name(RelationGetNamespace(onerel)), relname))); + snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s", schemaname,relname); empty_pages = vacuumed_pages = 0; num_tuples = tups_vacuumed = nkeep = nunused = 0; @@ -471,7 +482,11 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, indstats = (IndexBulkDeleteResult **) palloc0(nindexes * sizeof(IndexBulkDeleteResult *)); - nblocks = RelationGetNumberOfBlocks(onerel); + total_heap_pages = nblocks = RelationGetNumberOfBlocks(onerel); + + for (i = 0; i < nindexes; i++) + total_index_pages += RelationGetNumberOfBlocks(Irel[i]); + vacrelstats->rel_pages = nblocks; vacrelstats->scanned_pages = 0; vacrelstats->nonempty_pages = 0; @@ -520,10 +535,15 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, vacuum_delay_point(); } if (next_not_all_visible_block >= SKIP_PAGES_THRESHOLD) + { skipping_all_visible_blocks = true; + if(!scan_all) + scanned_heap_pages = scanned_heap_pages + next_not_all_visible_block; + } else skipping_all_visible_blocks = false; + snprintf(progress_message[1], PROGRESS_MESSAGE_LENGTH, "%s", phase1); for (blkno = 0; blkno < nblocks; blkno++) { Buffer buf; @@ -559,7 +579,11 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, * following blocks. */ if (next_not_all_visible_block - blkno > SKIP_PAGES_THRESHOLD) + { skipping_all_visible_blocks = true; + if(!scan_all) + scanned_heap_pages = scanned_heap_pages + next_not_all_visible_block; + } else skipping_all_visible_blocks = false; all_visible_according_to_vm = false; @@ -596,11 +620,25 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, /* Log cleanup info before we touch indexes */ vacuum_log_cleanup_info(onerel, vacrelstats); + snprintf(progress_message[1], PROGRESS_MESSAGE_LENGTH, "%s", phase2); + /* Remove index entries */ for (i = 0; i < nindexes; i++) + { lazy_vacuum_index(Irel[i], &indstats[i], vacrelstats); + scanned_index_pages += RelationGetNumberOfBlocks(Irel[i]); + + /* Report progress to the statistics collector */ + progress_param[0] = total_heap_pages; + progress_param[1] = scanned_heap_pages; + progress_param[2] = total_index_pages; + progress_param[3] = scanned_index_pages; + progress_param[4] = vacrelstats->num_index_scans + 1; + + pgstat_report_progress(progress_param, 5, progress_message, 2); + } /* Remove tuples from heap */ lazy_vacuum_heap(onerel, vacrelstats); @@ -610,8 +648,12 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, * valid. */ vacrelstats->num_dead_tuples = 0; + scanned_index_pages = 0; vacrelstats->num_index_scans++; } + snprintf(progress_message[1], PROGRESS_MESSAGE_LENGTH, "%s", phase1); + pgstat_report_progress(progress_param, 5, progress_message, 2); + /* * Pin the visibility map page in case we need to mark the page @@ -637,6 +679,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, if (!scan_all) { ReleaseBuffer(buf); + scanned_heap_pages++; vacrelstats->pinskipped_pages++; continue; } @@ -657,6 +700,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, { UnlockReleaseBuffer(buf); vacrelstats->scanned_pages++; + scanned_heap_pages++; vacrelstats->pinskipped_pages++; continue; } @@ -666,6 +710,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, } vacrelstats->scanned_pages++; + scanned_heap_pages++; page = BufferGetPage(buf); @@ -1062,8 +1107,22 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, */ if (vacrelstats->num_dead_tuples == prev_dead_count) RecordPageWithFreeSpace(onerel, blkno, freespace); - } + /* + * Reporting vacuum progress to statistics collector + */ + if (blkno == nblocks - 1 && vacrelstats->num_dead_tuples == 0 && nindexes != 0 + && vacrelstats->num_index_scans == 0) + total_index_pages = 0; + + progress_param[0] = total_heap_pages; + progress_param[1] = scanned_heap_pages; + progress_param[2] = total_index_pages; + progress_param[3] = scanned_index_pages; + progress_param[4] = vacrelstats->num_index_scans; + + pgstat_report_progress(progress_param, 5, progress_message, 2); + } pfree(frozen); /* save stats for use later */ @@ -1093,16 +1152,29 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, /* Log cleanup info before we touch indexes */ vacuum_log_cleanup_info(onerel, vacrelstats); + snprintf(progress_message[1], PROGRESS_MESSAGE_LENGTH, "%s", phase2); + /* Remove index entries */ for (i = 0; i < nindexes; i++) + { lazy_vacuum_index(Irel[i], &indstats[i], vacrelstats); + scanned_index_pages += RelationGetNumberOfBlocks(Irel[i]); + /* Report progress to the statistics collector */ + progress_param[0] = total_heap_pages; + progress_param[1] = scanned_heap_pages; + progress_param[2] = total_index_pages; + progress_param[3] = scanned_index_pages; + progress_param[4] = vacrelstats->num_index_scans + 1; + + pgstat_report_progress(progress_param, 5, progress_message, 2); + } /* Remove tuples from heap */ lazy_vacuum_heap(onerel, vacrelstats); vacrelstats->num_index_scans++; + scanned_index_pages = 0; } - /* Do post-vacuum cleanup and statistics update for each index */ for (i = 0; i < nindexes; i++) lazy_cleanup_index(Irel[i], indstats[i], vacrelstats); diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index ab018c4..ff64959 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -2851,6 +2851,51 @@ pgstat_report_activity(BackendState state, const char *cmd_str) pgstat_increment_changecount_after(beentry); } +/* --------------------------------------------- + * Called from VACUUM after every heap page scan or index scan + * to report progress + * --------------------------------------------- + */ + +void +pgstat_report_progress(uint32 *param1, int num_of_int, char param2[N_PROGRESS_PARAM][PROGRESS_MESSAGE_LENGTH], + int num_of_string) +{ + volatile PgBackendStatus *beentry = MyBEEntry; + int i; + + if (!beentry) + return; + + if (!pgstat_track_activities) + return; + + pgstat_increment_changecount_before(beentry); + + for(i = 0; i < num_of_int; i++) + { + beentry->st_progress_param[i] = param1[i]; + } + + for (i = 0; i < num_of_string; i++) + { + strcpy((char *)beentry->st_progress_message[i], param2[i]); + } + pgstat_increment_changecount_after(beentry); +} + +void +pgstat_report_activity_flag(activity_flag) +{ + PgBackendStatus *beentry = MyBEEntry; + beentry->st_activity_flag = activity_flag; +} +void +pgstat_reset_activity_flag() +{ + PgBackendStatus *beentry = MyBEEntry; + beentry->st_activity_flag = 0; +} /* ---------- * pgstat_report_appname() - * diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index f7c9bf6..d9f1c3a 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -53,6 +53,7 @@ extern Datum pg_stat_get_function_self_time(PG_FUNCTION_ARGS); extern Datum pg_stat_get_backend_idset(PG_FUNCTION_ARGS); extern Datum pg_stat_get_activity(PG_FUNCTION_ARGS); +extern Datum pg_stat_get_vacuum_progress(PG_FUNCTION_ARGS); extern Datum pg_backend_pid(PG_FUNCTION_ARGS); extern Datum pg_stat_get_backend_pid(PG_FUNCTION_ARGS); extern Datum pg_stat_get_backend_dbid(PG_FUNCTION_ARGS); @@ -523,7 +524,106 @@ pg_stat_get_backend_idset(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } } +/* + * Returns VACUUM progress values stored by each backend + * executing VACUUM. + */ +Datum +pg_stat_get_vacuum_progress(PG_FUNCTION_ARGS) +{ +#define PG_STAT_GET_PROGRESS_COLS 30 + int num_backends = pgstat_fetch_stat_numbackends(); + int curr_backend; + TupleDesc tupdesc; + Tuplestorestate *tupstore; + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + MemoryContext per_query_ctx; + MemoryContext oldcontext; + + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("materialize mode required, but it is not " \ + "allowed in this context"))); + + /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); + + per_query_ctx = rsinfo->econtext->ecxt_per_query_memory; + oldcontext = MemoryContextSwitchTo(per_query_ctx); + tupstore = tuplestore_begin_heap(true, false, work_mem); + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; + MemoryContextSwitchTo(oldcontext); + + for (curr_backend = 1; curr_backend <= num_backends; curr_backend++) + { + Datum values[PG_STAT_GET_PROGRESS_COLS]; + bool nulls[PG_STAT_GET_PROGRESS_COLS]; + LocalPgBackendStatus *local_beentry; + PgBackendStatus *beentry; + + MemSet(values, 0, sizeof(values)); + MemSet(nulls, 0, sizeof(nulls)); + + local_beentry = pgstat_fetch_stat_local_beentry(curr_backend); + if (!local_beentry) + continue; + beentry = &local_beentry->backendStatus; + + /* Report values for only those backends which are running VACUUM */ + if(!beentry || beentry->st_activity_flag != ACTIVITY_IS_VACUUM) + continue; + + values[0] = Int32GetDatum(beentry->st_procpid); + if(beentry->st_progress_message[0]) + values[1] = CStringGetTextDatum(beentry->st_progress_message[0]); + else + nulls[1] = true; + + + /* Progress can only be viewed by role member */ + if (has_privs_of_role(GetUserId(), beentry->st_userid)) + { + values[2] = CStringGetTextDatum(beentry->st_progress_message[1]); + values[3] = UInt32GetDatum(beentry->st_progress_param[0]); + values[4] = UInt32GetDatum(beentry->st_progress_param[1]); + if (beentry->st_progress_param[0] != 0) + values[5] = Float8GetDatum(beentry->st_progress_param[1] * 100 / beentry->st_progress_param[0]); + else + nulls[5] = true; + values[6] = UInt32GetDatum(beentry->st_progress_param[2]); + values[7] = UInt32GetDatum(beentry->st_progress_param[3]); + values[8] = UInt32GetDatum(beentry->st_progress_param[4]); + + } + else + { + values[2] = CStringGetTextDatum("<insufficient privilege>"); + nulls[3] = true; + nulls[4] = true; + nulls[5] = true; + nulls[6] = true; + nulls[7] = true; + nulls[8] = true; + } + + tuplestore_putvalues(tupstore, tupdesc, values, nulls); + } + + /* clean up and return the tuplestore */ + tuplestore_donestoring(tupstore); + + return (Datum) 0; +} /* * Returns activity of PG backends. */ diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index d8640db..ae03c15 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -2783,6 +2783,8 @@ DATA(insert OID = 1936 ( pg_stat_get_backend_idset PGNSP PGUID 12 1 100 0 0 f DESCR("statistics: currently active backend IDs"); DATA(insert OID = 2022 ( pg_stat_get_activity PGNSP PGUID 12 1 100 0 0 f f f f f t s r 1 0 2249 "23" "{23,26,23,26,25,25,25,16,1184,1184,1184,1184,869,25,23,28,28,16,25,25,23,16,25}" "{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,datid,pid,usesysid,application_name,state,query,waiting,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,ssl,sslversion,sslcipher,sslbits,sslcompression,sslclientdn}" _null_ _null_ pg_stat_get_activity _null_ _null_ _null_ )); DESCR("statistics: information about currently active backends"); +DATA(insert OID = 3319 ( pg_stat_get_vacuum_progress PGNSP PGUID 12 1 1 0 0 f f f f f t s r 0 0 2249 "" "{23,25,25,23,23,701,23,23,23}" "{o,o,o,o,o,o,o,o,o}" "{pid,table_name,phase,total_heap_pages,scanned_heap_pages,percent_complete,total_index_pages,scanned_index_pages,index_scan_count}" _null_ _null_ pg_stat_get_vacuum_progress _null_ _null_ _null_ )); +DESCR("statistics: information about progress of backends running VACUUM"); DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ )); DESCR("statistics: information about currently active replication"); DATA(insert OID = 2026 ( pg_backend_pid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ )); diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 9ecc163..576ffbd 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -205,6 +205,8 @@ typedef struct PgStat_MsgHdr #define PGSTAT_MAX_MSG_SIZE 1000 #define PGSTAT_MSG_PAYLOAD (PGSTAT_MAX_MSG_SIZE - sizeof(PgStat_MsgHdr)) +#define N_PROGRESS_PARAM 10 +#define PROGRESS_MESSAGE_LENGTH 30 /* ---------- * PgStat_MsgDummy A dummy message, ignored by the collector @@ -776,6 +778,19 @@ typedef struct PgBackendStatus /* current command string; MUST be null-terminated */ char *st_activity; + + /* + * Information about the progress of activity/command being run by the backend. + * The progress parameters indicate progress of a command. Different + * commands can report different number of parameters of each type. + * + * st_activity_flag reports which activity/command is being run by the backend. + * This is used in the SQL callable functions to display progress values + * for respective commands. + */ + uint32 st_activity_flag; + uint32 st_progress_param[N_PROGRESS_PARAM]; + char st_progress_message[N_PROGRESS_PARAM][PROGRESS_MESSAGE_LENGTH]; } PgBackendStatus; /* @@ -815,6 +830,7 @@ typedef struct PgBackendStatus save_changecount = beentry->st_changecount; \ } while (0) +#define ACTIVITY_IS_VACUUM 0x01 /* ---------- * LocalPgBackendStatus * @@ -928,6 +944,10 @@ extern void pgstat_initialize(void); extern void pgstat_bestart(void); extern void pgstat_report_activity(BackendState state, const char *cmd_str); +extern void pgstat_report_activity_flag(int activity_flag); +extern void pgstat_reset_activity_flag(void); +extern void pgstat_report_progress(uint32 *param1, int num_of_int, char param2[N_PROGRESS_PARAM][PROGRESS_MESSAGE_LENGTH], + int num_of_string); extern void pgstat_report_tempfile(size_t filesize); extern void pgstat_report_appname(const char *appname); extern void pgstat_report_xact_timestamp(TimestampTz tstamp); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 80374e4..8404842 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1848,6 +1848,16 @@ pg_stat_user_tables| SELECT pg_stat_all_tables.relid, pg_stat_all_tables.autoanalyze_count FROM pg_stat_all_tables WHERE ((pg_stat_all_tables.schemaname <> ALL (ARRAY['pg_catalog'::name, 'information_schema'::name])) AND (pg_stat_all_tables.schemaname !~ '^pg_toast'::text)); +pg_stat_vacuum_progress| SELECT s.pid, + s.table_name, + s.total_heap_pages, + s.scanned_heap_pages, + s.total_index_pages, + s.scanned_index_pages, + s.total_pages, + s.scanned_pages, + s.percent_complete + FROM pg_stat_get_vacuum_progress() s(pid, table_name, total_heap_pages, scanned_heap_pages, total_index_pages, scanned_index_pages, total_pages, scanned_pages, percent_complete); pg_stat_xact_all_tables| SELECT c.oid AS relid, n.nspname AS schemaname, c.relname,
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers