[PROPOSAL] VACUUM Progress Checker.

Hi

2015-06-30 9:37 GMT+02:00 Rahila Syed <rahilasyed90@gmail.com>:

Hello Hackers,

Following is a proposal for feature to calculate VACUUM progress.

interesting idea - I like to see it integrated to core.

Use Case : Measuring progress of long running VACUUMs to help DBAs make
informed decision
whether to continue running VACUUM or abort it.

Design:

A shared preload library to store progress information from different
backends running VACUUM, calculate remaining time for each and display
progress in the
in the form a view.

probably similar idea can be used for REINDEX, CREATE INDEX, COPY TO
statements

I though about the possibilities of progress visualization - and one
possibility is one or two special column in pg_stat_activity table - this
info can be interesting for VACUUM started by autovacuum too.

Regards

Pavel

On Tue, Jun 30, 2015 at 7:37 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello Hackers,

Following is a proposal for feature to calculate VACUUM progress.

Use Case : Measuring progress of long running VACUUMs to help DBAs make
informed decision
whether to continue running VACUUM or abort it.

+1

I was thinking recently that it would be very cool to see some
estimation of the progress of VACUUM and CLUSTER in a view similar to
pg_stat_activity, or the ps title.

--
Thomas Munro
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

On Tue, Jun 30, 2015 at 1:07 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello Hackers,

Following is a proposal for feature to calculate VACUUM progress.

Use Case : Measuring progress of long running VACUUMs to help DBAs make
informed decision
whether to continue running VACUUM or abort it.

+1

I am excited to know how the progress works in when any of the statement
got blocked during locks. Rather displaying the stats in the LOG, shall we
have this in a pg_stat_vacuum_activity[ New catalog for all auto-vacuum
stats].

Best Regards,
Dinesh
manojadinesh.blogspot.com

Design:

On 30 June 2015 at 08:52, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I though about the possibilities of progress visualization - and one
possibility is one or two special column in pg_stat_activity table - this
info can be interesting for VACUUM started by autovacuum too.

Yes, I suggest just a single column on pg_stat_activity called pct_complete

trace_completion_interval = 5s (default)

Every interval, we report the current % complete for any operation that
supports it. We just show NULL if the current operation has not reported
anything or never will.

We do this for VACUUM first, then we can begin adding other operations as
we work out how (for that operation).

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2015-06-30 PM 04:37, Rahila Syed wrote:

Design:

A shared preload library to store progress information from different
backends running VACUUM, calculate remaining time for each and display
progress in the
in the form a view.

[...]

Reporting :

A view named 'pg_maintenance_progress' can be created using the values in
the struct above.

pg_stat_maintenance can be called from any other backend and will display
progress of

+1

Just to clarify, the attached patch does not implement the view or the shared
memory initialization part yet, right? I understand your intention to get
comments on proposed hooks and shared memory structure(s) at this point. By
the way, how does a regular send stats to background stats collector approach
compares to the proposed hooks+shmem approach?

Thanks,
Amit

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

On 30 June 2015 at 08:37, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello Hackers,

Following is a proposal for feature to calculate VACUUM progress.

Use Case : Measuring progress of long running VACUUMs to help DBAs make
informed decision
whether to continue running VACUUM or abort it.

Design:

A shared preload library to store progress information from different
backends running VACUUM, calculate remaining time for each and display
progress in the
in the form a view.

VACUUM needs to be instrumented with a hook to collect progress
information (pages vacuumed/scanned) periodically.

The patch attached implements a new hook to store vacuumed_pages and
scanned_pages count at the end of each page scanned by VACUUM.

This information is stored in a shared memory structure.

In addition to measuring progress this function using hook also calculates
remaining time for VACUUM.

The frequency of collecting progress information can be reduced by
appending delays in between hook function calls.

Also, a GUC parameter

log_vacuum_min_duration can be used.

This will cause VACUUM progress to be calculated only if VACUUM runs more
than specified milliseconds.

A value of zero calculates VACUUM progress for each page processed. -1
disables logging.

Progress calculation :

percent_complete = scanned_pages * 100 / total_pages_to_be_scanned;

remaining_time = elapsed_time * (total_pages_to_be_scanned -
scanned_pages) / scanned_pages;

Shared memory struct:

typedef struct PgStat_VacuumStats

{

Oid databaseoid;

Oid tableoid;

Int32 vacuumed_pages;

Int32 total_pages;

Int32 scanned_pages;

double elapsed_time;

double remaining_time;

} PgStat_VacuumStats[max_connections];

Reporting :

A view named 'pg_maintenance_progress' can be created using the values in
the struct above.

pg_stat_maintenance can be called from any other backend and will display
progress of

each running VACUUM.

Other uses of hook in VACUUM:

Cost of VACUUM in terms of pages hit , missed and dirtied same as
autovacuum can be collected using this hook.

Autovacuum does it at the end of VACUUM for each table. It can be done
while VACUUM on a table is in progress.
This can be helpful to track manual VACUUMs also not just autovacuum.

Read/Write(I/O) rates can be computed on the lines of autovacuum.
Read rate patterns can be used to help tuning future vacuum on the
table(like shared buffers tuning)
Other resource usages can also be collected using progress checker hook.

Attached patch is POC patch of progress calculation for a single backend.

Also attached is a brief snapshot of the output log.

@@ -559,7 +567,9 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
* following blocks.
*/
if (next_not_all_visible_block - blkno > SKIP_PAGES_THRESHOLD)
+ {
skipping_all_visible_blocks = true;
+ }

There's no need to add those curly braces, or to subsequent if blocks.

Also, is this patch taking the visibility map into account for its
calculations?

--
Thom

Hello,

There's no need to add those curly braces, or to subsequent if blocks

Yes, those are added by mistake.

Also, is this patch taking the visibility map into account for its calculations?

Yes, it subtracts skippable/all-visible pages from total pages to be scanned.
For each page processed by lazy_scan_heap, if number of all visible pages ahead exceeds the threshold, it is subtracted from
the ‘total pages to be scanned’ count.

The all visible pages are accounted for incrementally during the execution of VACUUM and not before starting the process.

Thank you,
Rahila Syed

From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Thom Brown
Sent: Tuesday, June 30, 2015 2:20 PM
To: Rahila Syed
Cc: PostgreSQL-development
Subject: Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

On 30 June 2015 at 08:37, Rahila Syed <rahilasyed90@gmail.com<mailto:rahilasyed90@gmail.com>> wrote:
Hello Hackers,

Following is a proposal for feature to calculate VACUUM progress.

Use Case : Measuring progress of long running VACUUMs to help DBAs make informed decision
whether to continue running VACUUM or abort it.

Design:

A shared preload library to store progress information from different backends running VACUUM, calculate remaining time for each and display progress in the
in the form a view.

VACUUM needs to be instrumented with a hook to collect progress information (pages vacuumed/scanned) periodically.
The patch attached implements a new hook to store vacuumed_pages and scanned_pages count at the end of each page scanned by VACUUM.
This information is stored in a shared memory structure.
In addition to measuring progress this function using hook also calculates remaining time for VACUUM.

The frequency of collecting progress information can be reduced by appending delays in between hook function calls.
Also, a GUC parameter
log_vacuum_min_duration can be used.
This will cause VACUUM progress to be calculated only if VACUUM runs more than specified milliseconds.
A value of zero calculates VACUUM progress for each page processed. -1 disables logging.

Progress calculation :

percent_complete = scanned_pages * 100 / total_pages_to_be_scanned;

remaining_time = elapsed_time * (total_pages_to_be_scanned - scanned_pages) / scanned_pages;

Shared memory struct:

typedef struct PgStat_VacuumStats

{

Oid databaseoid;

Oid tableoid;

Int32 vacuumed_pages;

Int32 total_pages;

Int32 scanned_pages;

double elapsed_time;

double remaining_time;

} PgStat_VacuumStats[max_connections];

Reporting :
A view named 'pg_maintenance_progress' can be created using the values in the struct above.
pg_stat_maintenance can be called from any other backend and will display progress of
each running VACUUM.

Other uses of hook in VACUUM:

Cost of VACUUM in terms of pages hit , missed and dirtied same as autovacuum can be collected using this hook.
Autovacuum does it at the end of VACUUM for each table. It can be done while VACUUM on a table is in progress.
This can be helpful to track manual VACUUMs also not just autovacuum.

Read/Write(I/O) rates can be computed on the lines of autovacuum.
Read rate patterns can be used to help tuning future vacuum on the table(like shared buffers tuning)
Other resource usages can also be collected using progress checker hook.

Attached patch is POC patch of progress calculation for a single backend.
Also attached is a brief snapshot of the output log.

@@ -559,7 +567,9 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
* following blocks.
*/
if (next_not_all_visible_block - blkno > SKIP_PAGES_THRESHOLD)
+ {
skipping_all_visible_blocks = true;
+ }
There's no need to add those curly braces, or to subsequent if blocks.
Also, is this patch taking the visibility map into account for its calculations?

--
Thom

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

Hello,

Thank you for suggestions.

Yes, I suggest just a single column on pg_stat_activity called pct_complete

Reporting remaining time also can be crucial to make decisions regarding
continuing or aborting VACUUM.
The same has been suggested in the thread below,

/messages/by-id/13072.1284826206@sss.pgh.pa.us

trace_completion_interval = 5s (default)

Every interval, we report the current % complete for any operation that

supports it. We just show NULL if the current operation has not reported
anything or never will.

We do this for VACUUM first, then we can begin adding other operations as

we work out how (for that operation).

Thank you for explaining. This design seems good to me except, adding more
than one columns(percent_complete, remaining_time) if required to
pg_stat_activity can be less user intuitive than having a separate view for
VACUUM.

-Rahila Syed

On Tue, Jun 30, 2015 at 2:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Hello,

I though about the possibilities of progress visualization - and one

possibility is one or two special column in pg_stat_activity table - this
info can be interesting for VACUUM >started by autovacuum too.

Thank you for suggestion. The design with hooks and a separate view was
mainly to keep most of the code outside core as the feature proposed is
specific to VACUUM command. Also, having a separate view can give more
flexibility in terms of displaying various progress parameters.

FWIW ,there was resistance to include columns in pg_stat_activity earlier
in the following thread,
/messages/by-id/AANLkTi=TcuMA38oGUKX9p5WVPpY+M3L0XUp7=PLT+LCT@mail.gmail.com

Thank you,
Rahila Syed

On Tue, Jun 30, 2015 at 1:22 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

On 2015-07-02 AM 11:41, Rahila Syed wrote:

Hello,

I though about the possibilities of progress visualization - and one

possibility is one or two special column in pg_stat_activity table - this
info can be interesting for VACUUM >started by autovacuum too.

Thank you for suggestion. The design with hooks and a separate view was
mainly to keep most of the code outside core as the feature proposed is
specific to VACUUM command. Also, having a separate view can give more
flexibility in terms of displaying various progress parameters.

Unless I am missing something, I guess you can still keep the actual code that
updates counters outside the core if you adopt an approach that Simon
suggests. Whatever the view (existing/new), any related counters would have a
valid (non-NULL) value when read off the view iff hooks are set perhaps
because you have an extension that sets them. I guess that means any operation
that "supports" progress tracking would have an extension with suitable hooks
implemented.

Of course unless I misinterpreted Simon's words.

Thanks,
Amit

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

Hello,

Thank you for suggestion. The design with hooks and a separate view was

mainly to keep most of the >code outside core as the feature proposed is
specific to VACUUM command. Also, having a separate view >can give more
flexibility in terms of displaying various progress parameters.

FWIW ,there was resistance to include columns in pg_stat_activity earlier

in the following thread,

/messages/by-id/AANLkTi=TcuMA38oGUKX9p5WVPpY+M3L0XUp7=PLT+LCT@...

Perhaps as suggested in the link, the progress could be made available via a
function call which does progress calculation "on demand". Then we do not
need a separate view, or clutter pg_stat_activity, and also has benefit of
calculating progress just when it's needed.

--
View this message in context: http://postgresql.nabble.com/PROPOSAL-VACUUM-Progress-Checker-tp5855849p5856192.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

On 2 July 2015 at 03:00, Rahila Syed <rahilasyed90@gmail.com> wrote:

Yes, I suggest just a single column on pg_stat_activity called

pct_complete

Reporting remaining time also can be crucial to make decisions regarding
continuing or aborting VACUUM.
The same has been suggested in the thread below,

/messages/by-id/13072.1284826206@sss.pgh.pa.us

trace_completion_interval = 5s (default)

Every interval, we report the current % complete for any operation that

supports it. We just show NULL if the current operation has not reported
anything or never will.

We do this for VACUUM first, then we can begin adding other operations

as we work out how (for that operation).

Thank you for explaining. This design seems good to me except, adding more
than one columns(percent_complete, remaining_time)

It is attractive to have a "remaining_time" column, or a
"predicted_completion_timestamp", but those columns are prediction
calculations rather than actual progress reports. I'm interested in seeing
a report that relates to actual progress made.

Predicted total work required is also interesting, but is much less
trustworthy figure.

I think we'll need to get wider input about the user interface for this
feature.

if required to pg_stat_activity can be less user intuitive than having a
separate view for VACUUM.

I think it is a mistake to do something just for VACUUM.

Monitoring software will look at pg_stat_activity. I don't think we should
invent a separate view for progress statistics because it will cause users
to look in two places rather than just one. Reporting progress is fairly
cheap instrumentation, calculating a prediction of completion time might be
expensive.

Having said that, monitoring systems currently use a polling mechanism to
retrieve status data. They look at information published by the backend. We
don't currently have a mechanism to defer publication of expensive
monitoring information until requested by the monitoring system. If you
have a design for how that might work then say so, otherwise we need to
assume a simple workflow: the backend publishes whatever it chooses,
whenever it chooses and then that is made available via the monitoring
system via views.

Your current design completely misses the time taken to scan indexes, which
is significant.

There might be a justification to put this out of core, but measuring
progress of VACUUM wouldn't be it, IMHO.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Le 2 juil. 2015 7:28 AM, "Simon Riggs" <simon@2ndquadrant.com> a écrit :

On 2 July 2015 at 03:00, Rahila Syed <rahilasyed90@gmail.com> wrote:

Yes, I suggest just a single column on pg_stat_activity called

pct_complete

Reporting remaining time also can be crucial to make decisions regarding

continuing or aborting VACUUM.

The same has been suggested in the thread below,

/messages/by-id/13072.1284826206@sss.pgh.pa.us

trace_completion_interval = 5s (default)

Every interval, we report the current % complete for any operation that

supports it. We just show NULL if the current operation has not reported
anything or never will.

We do this for VACUUM first, then we can begin adding other operations

as we work out how (for that operation).

Thank you for explaining. This design seems good to me except, adding

more than one columns(percent_complete, remaining_time)

It is attractive to have a "remaining_time" column, or a

"predicted_completion_timestamp", but those columns are prediction
calculations rather than actual progress reports. I'm interested in seeing
a report that relates to actual progress made.

Agreed.

Predicted total work required is also interesting, but is much less

trustworthy figure.

And it is something a client app or an extension can compute. No need to
put this in core as long as we have the actual progress.

I think we'll need to get wider input about the user interface for this

feature.

if required to pg_stat_activity can be less user intuitive than having a

separate view for VACUUM.

I think it is a mistake to do something just for VACUUM.

Monitoring software will look at pg_stat_activity. I don't think we

should invent a separate view for progress statistics because it will cause
users to look in two places rather than just one. Reporting progress is
fairly cheap instrumentation, calculating a prediction of completion time
might be expensive.

+1

Having said that, monitoring systems currently use a polling mechanism to

retrieve status data. They look at information published by the backend. We
don't currently have a mechanism to defer publication of expensive
monitoring information until requested by the monitoring system. If you
have a design for how that might work then say so, otherwise we need to
assume a simple workflow: the backend publishes whatever it chooses,
whenever it chooses and then that is made available via the monitoring
system via views.

Your current design completely misses the time taken to scan indexes,

which is significant.

There might be a justification to put this out of core, but measuring

progress of VACUUM wouldn't be it, IMHO.

Hello,

Unless I am missing something, I guess you can still keep the actual code that updates counters outside the core if you adopt an approach that Simon suggests.

Yes. The code to extract progress information from VACUUM and storing in shared memory can be outside core even with pg_stat_activity as a user interface.

Whatever the view (existing/new), any related counters would have a valid (non-NULL) value when read off the view iff hooks are set perhaps because you have an extension that sets them.
I guess that means any operation that "supports" progress tracking would have an extension with suitable hooks implemented.

Do you mean to say , any operation/application that want progress tracking feature will dynamically load the progress checker module which will set the hooks for progress reporting?
If yes , unless I am missing something such dynamic loading cannot happen if we use pg_stat_activity as it gets values from shared memory. Module has to be a shared_preload_library
to allocate a shared memory. So this will mean the module can be loaded only at server restart. Am I missing something?

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

"Syed, Rahila" <Rahila.Syed@nttdata.com> writes:

Hello,

Unless I am missing something, I guess you can still keep the actual code that updates counters outside the core if you adopt an approach that Simon suggests.

Yes. The code to extract progress information from VACUUM and storing in shared memory can be outside core even with pg_stat_activity as a user interface.

Whatever the view (existing/new), any related counters would have a valid (non-NULL) value when read off the view iff hooks are set perhaps because you have an extension that sets them.
I guess that means any operation that "supports" progress tracking would have an extension with suitable hooks implemented.

Do you mean to say , any operation/application that want progress tracking feature will dynamically load the progress checker module which will set the hooks for progress reporting?
If yes , unless I am missing something such dynamic loading cannot happen if we use pg_stat_activity as it gets values from shared memory. Module has to be a shared_preload_library
to allocate a shared memory. So this will mean the module can be loaded

only at server restart. Am I missing something?

TBH, I think that designing this as a hook-based solution is adding a
whole lot of complexity for no value. The hard parts of the problem are
collecting the raw data and making the results visible to users, and
both of those require involvement of the core code. Where is the benefit
from pushing some trivial intermediate arithmetic into an external module?
If there's any at all, it's certainly not enough to justify problems such
as you mention here.

So I'd just create a "pgstat_report_percent_done()" type of interface in
pgstat.c and then teach VACUUM to call it directly.

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

On 2015-07-02 PM 11:00, Syed, Rahila wrote:

Hello,

Unless I am missing something, I guess you can still keep the actual code that updates counters outside the core if you adopt an approach that Simon suggests.

Yes. The code to extract progress information from VACUUM and storing in shared memory can be outside core even with pg_stat_activity as a user interface.

Whatever the view (existing/new), any related counters would have a valid (non-NULL) value when read off the view iff hooks are set perhaps because you have an extension that sets them.
I guess that means any operation that "supports" progress tracking would have an extension with suitable hooks implemented.

Do you mean to say , any operation/application that want progress tracking feature will dynamically load the progress checker module which will set the hooks for progress reporting?
If yes , unless I am missing something such dynamic loading cannot happen if we use pg_stat_activity as it gets values from shared memory. Module has to be a shared_preload_library
to allocate a shared memory. So this will mean the module can be loaded only at server restart. Am I missing something?

Assuming that set of hooks per command and shared memory structure(s) is a way
to go, I meant to say that hook implementations per command would be in their
separate modules, of course loaded at the server start for shared memory). Of
those, your proposed patch has vacuum_progress, for example. And in context of
my comment above, that means the view would say NULL for commands for which
the module has not been set up in advance. IOW, between showing NULL in the
view and dynamically loading hook functions, we choose the former because I
don't know what the latter means in postgres.

Having said that, Tom's suggestion to export pgstat.c function(s) for
command(s) may be a more appealing way to go.

Thanks,
Amit

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

Hello,

TBH, I think that designing this as a hook-based solution is adding a whole lot of complexity for no value. The hard parts of the problem are collecting the raw data and making the results visible to users, and both of those require involvement of the core code. Where is the benefit from pushing some trivial >intermediate arithmetic into an external module?
If there's any at all, it's certainly not enough to justify problems such as you mention here.

So I'd just create a "pgstat_report_percent_done()" type of interface in pgstat.c and then teach VACUUM to call it directly.

Thank you for suggestion. I agree that adding code in core will reduce code complexity with no additional overhead.
Going by the consensus, I will update the patch with code to collect and store progress information from vacuum in pgstat.c and
UI using pg_stat_activity view.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

Hello,

Please find attached updated patch with an interface to calculate command
progress in pgstat.c. This interface currently implements VACUUM progress
tracking .
A column named percent_complete has been added in pg_stat_activity to
report progress.

VACUUM calls the progress calculation interface periodically at an interval
specified by pgstat_track_progress GUC in ms.
Progress calculation can be disabled by setting pgstat_track_progress as
-1.

Remaining_time for VACUUM is not included in current patch to avoid
cluttering pg_stat_activity with too many columns.
But the estimate as seen from previous implementation seems reasonable
enough to be included in progress information , may be as an exclusive view
for vacuum progress information.

GUC parameter 'pgstat_track_progress' is currently PGC_SUSET in line with
'track_activities' GUC parameter. Although IMO, pgstat_track_progress can
be made PGC_USERSET in order to provide more flexibility to any user to
enable/disable progress calculation provided progress is tracked only if
track_activities GUC parameter is enabled.

In this patch, index scans are not taken into account for progress
calculation as of now .

Thank you,
Rahila Syed.

On 2015-07-16 AM 05:18, Rahila Syed wrote:

GUC parameter 'pgstat_track_progress' is currently PGC_SUSET in line with
'track_activities' GUC parameter.

Naming the GUC pgstat* seems a little inconsistent. It could be called,
say, track_maintenance_progress_interval/track_vacuum_progress_interval.
That way, it will look similar to existing track_* parameters:

#track_activities = on
#track_counts = on
#track_io_timing = off
#track_functions = none # none, pl, all
#track_activity_query_size = 1024 # (change requires restart)

Also, adding the new GUC to src/backend/utils/misc/postgresql.conf.sample
might be helpful.

Thanks,
Amit

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

On Thu, Jul 16, 2015 at 5:18 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello,

Please find attached updated patch with an interface to calculate command
progress in pgstat.c.

Thanks for updating the patch!

I got the following compiler warning.

guc.c:2316: warning: initialization makes pointer from integer without a cast

make check-world caused lots of failures in my environment.

The following query caused a segmentation fault.

SELECT name FROM (SELECT pg_catalog.lower(name) AS name FROM
pg_catalog.pg_settings UNION ALL SELECT 'session authorization'
UNION ALL SELECT 'all') ss WHERE substring(name,1,3)='tra';

Regards,

--
Fujii Masao

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

Hello,

Your current design completely misses the time taken to scan indexes, which

is significant.
I tried to address this issue in the attached patch.
The patch calculates index scan progress by measuring against scanned pages
in LVRelStats. It checks for a change current page being scanned and
increments the progress counter. When counter reaches scanned pages number
in LVRelStats, progress is 100% complete. For now the progress is emitted as
a warning (so no config changes needed to see progress)
Thoughts?
regards
Sameer IndexScanProgress.patch
<http://postgresql.nabble.com/file/n5858109/IndexScanProgress.patch&gt;

--
View this message in context: http://postgresql.nabble.com/PROPOSAL-VACUUM-Progress-Checker-tp5855849p5858109.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

On Thu, Jul 16, 2015 at 1:30 PM, Sameer Thakur-2 wrote:

Thoughts?
regards
Sameer IndexScanProgress.patch
<http://postgresql.nabble.com/file/n5858109/IndexScanProgress.patch&gt;

I am not really willing to show up as the picky guy here, but could it
be possible to receive those patches as attached to emails instead of
having them referenced by URL? I imagine that you are directly using
the nabble interface.
--
Michael

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

Hello,

I am not really willing to show up as the picky guy here, but could it be possible to receive those patches as attached to emails instead of having them referenced by URL? I >imagine that you are directly using the nabble interface.

Just configured a new mail client for nabble, did not know how to use it within an existing conversation.
Now I can send the patch attached!
Thanks
Sameer

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

Hi

On Wed, Jul 15, 2015 at 9:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Jul 16, 2015 at 5:18 AM, Rahila Syed <rahilasyed90@gmail.com>
wrote:

Hello,

Please find attached updated patch with an interface to calculate command
progress in pgstat.c.

Thanks for updating the patch!

I got the following compiler warning.

guc.c:2316: warning: initialization makes pointer from integer without a
cast

make check-world caused lots of failures in my environment.

Yeah, i got couple of warnings with plain make.

The following query caused a segmentation fault.

It was the typo I believe. I see the problem is with GUC definition in
guc.c. There should be "NULL" between gettext_noop and GUC_UNIT_MS.

Regards,
Dinesh
manojadinesh.blogspot.com

SELECT name FROM (SELECT pg_catalog.lower(name) AS name FROM

Hello,

Naming the GUC pgstat* seems a little inconsistent.

Sorry, there is a typo in the mail. The GUC name is 'track_activity_progress'.

Also, adding the new GUC to src/backend/utils/misc/postgresql.conf.sample
might be helpful

Yes. I will update.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

On 7/15/15 11:38 PM, Thakur, Sameer wrote:

Hello,

I am not really willing to show up as the picky guy here, but could it be possible to receive those patches as attached to emails instead of having them referenced by URL? I >imagine that you are directly using the nabble interface.

Just configured a new mail client for nabble, did not know how to use it within an existing conversation.

Does this actually handle multiple indexes? It doesn't appear so, which
I'd think is a significant problem... :/

I'm also not seeing how this will deal with exhausting
maintenance_work_mem. ISTM that when that happens you'd definitely want
a better idea of what's going on...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Hello,

Does this actually handle multiple indexes? It doesn't appear so, which I'd think is a significant problem... :/

Please find v2 attached which does this.

I'm also not seeing how this will deal with exhausting maintenance_work_mem. ISTM that when that happens you'd definitely want a better idea of what's going on...

Will work on this aspect in v3.
Thank you,
Sameer

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

On 7/20/15 4:32 AM, Thakur, Sameer wrote:

Hello,

Does this actually handle multiple indexes? It doesn't appear so, which I'd think is a significant problem... :/

Please find v2 attached which does this.

I think it'd be better to combine both numbers into one report:

elog(WARNING,"Current/Overall index percentage completion %f/%f",
current_index_progress * 100, all_index_progress);

It'd also be good to standardize on where the * 100 is happening.

Also, AFAIK:

(itemptr->ip_blkid.bi_hi != vacrelstats->last_scanned_page.bi_hi) ||
(itemptr->ip_blkid.bi_lo != vacrelstats->last_scanned_page.bi_lo)

can be replaced by

(itemptr->ipblkid != vacrelstats->last_scanned_page)

and

vacrelstats->current_index_scanned_page_count =
vacrelstats->current_index_scanned_page_count + 1;

can simply be

vacrelstats->current_index_scanned_page_count++;
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On Tue, Jun 30, 2015 at 4:32 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Yes, I suggest just a single column on pg_stat_activity called pct_complete

trace_completion_interval = 5s (default)

Every interval, we report the current % complete for any operation that
supports it. We just show NULL if the current operation has not reported
anything or never will.

I am deeply skeptical about the usefulness of a progress-reporting
system that can only report one number. I think that, in many cases,
it won't be possible to compute an accurate completion percentage, and
even if it is, people may want more data than that for various reasons
anyway.

For example, in the case of VACUUM, suppose there is a table with
1,000,000 heap pages and 200,000 index pages (so it's probably
over-indexed, but whatever). After reading 500,000 heap pages, we
have found 0 dead tuples. What percentage of the work have we
finished?

It's hard to say. If we don't find any dead tuples, we've read half
the pages we will eventually read and are therefore half done. But if
we find even 1 dead tuple, then we've got to scan all 200,000 index
pages, so we've read only 41.7% of the pages we'll eventually touch.
If we find so many dead tuples that we have to scan the indexes
multiple times for lack of maintenance_work_mem, we'll eventually read
1,000,000 + 200,000k pages, where k is the number of index scans; if
say k = 5 then we are only 25% done. All of these scenarios are
plausible because, in all likelihood, the dirty pages in the table are
concentrated near the end.

Now we could come up with ways of making good guesses about what is
likely to happen. We could look at the data from pg_stat_all_tables,
historical results of vacuuming this table, the state of the
visibility map, and so on. And that all might help. But it's going
to be fairly hard to produce a percentage of completion that is
monotonically increasing and always accurately reflects the time
remaining. Even if we can do it, it doesn't seem like a stretch to
suppose that sometimes people will want to look at the detail data.
Instead of getting told "we're X% done" (according to some arcane
formula), it's quite reasonable to think that people will want to get
a bunch of values, e.g.:

1. For what percentage of heap pages have we completed phase one (HOT
prune + mark all visible if appropriate + freeze + remember dead
tuples)?
2. For what percentage of heap pages have we completed phase two (mark
line pointers unused)?
3. What percentage of maintenance_work_mem are we currently using to
remember tuples?

For a query, the information we want back is likely to be even more
complicated; e.g. EXPLAIN output with row counts and perhaps timings
to date for each plan node. We can insist that all status reporting
get boiled down to one number, but I suspect we would be better off
asking ourselves how we could let commands return a richer set of
data.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 21 July 2015 at 21:24, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Jun 30, 2015 at 4:32 AM, Simon Riggs <simon@2ndquadrant.com>
wrote:

Yes, I suggest just a single column on pg_stat_activity called

pct_complete

trace_completion_interval = 5s (default)

Every interval, we report the current % complete for any operation that
supports it. We just show NULL if the current operation has not reported
anything or never will.

I am deeply skeptical about the usefulness of a progress-reporting
system that can only report one number. I think that, in many cases,
it won't be possible to compute an accurate completion percentage, and
even if it is, people may want more data than that for various reasons
anyway.

The goal here was to have a common metric for all tasks. Multiple numbers
are OK for me, but not extended detail (yet!)

As I said later:

Simon Riggs <simon@2ndquadrant.com> wrote:

I'm interested in seeing a report that relates to actual progress made.
Predicted total work required is also interesting, but is much less

trustworthy figure.

I'm aware of the difficulties for VACUUM in particular and agree with your
scenario/details.

That does not deter me from wishing to see high level information, even it
varies or is inaccurate. The "arrival time" on my Sat Nav is still useful,
even if it changes because of traffic jams that develop while my journey is
in progress. If the value bothers me, I look at the detail. So both summary
and detail information are useful, but summary is more important even
though it is less accurate.

Instead of getting told "we're X% done" (according to some arcane

formula), it's quite reasonable to think that people will want to get
a bunch of values, e.g.:

1. For what percentage of heap pages have we completed phase one (HOT
prune + mark all visible if appropriate + freeze + remember dead
tuples)?
2. For what percentage of heap pages have we completed phase two (mark
line pointers unused)?
3. What percentage of maintenance_work_mem are we currently using to
remember tuples?

For a query, the information we want back is likely to be even more
complicated; e.g. EXPLAIN output with row counts and perhaps timings
to date for each plan node. We can insist that all status reporting
get boiled down to one number, but I suspect we would be better off
asking ourselves how we could let commands return a richer set of
data.

I agree that it is desirable to have a more detailed breakdown of what is
happening. As soon as we do that we hit the need for very action-specific
information reporting, which renders the topic much harder and much more
specific.

For me, the user workflow looks like these....

Worried: "Task X is taking ages? When is it expected to finish?"
Ops: 13:50
<sometime later, about 14:00>
Worried: "Task X is still running? But I thought its ETA was 13:50?"
Ops: Now says 14:30
Worried: "Is it stuck, or is it making progress?"
Ops: Looks like its making progress
Worried: "Can we have a look at it and find out what its doing?"

Worried: "When will Task Y finish?"
Ops: Monday at 11am
Worried: "Bad news! We should cancel it on Sunday evening."

The point is that nobody looks at the detailed info until we have looked at
the summary. So the summary of progress/completion time is important, even
if it is possibly wrong. The detail is also useful. I think we should have
both, but I'd like to see the summary info first, because it is the most
useful, best leading indicator of problems.

In terms of VACUUM specifically: VACUUM should be able to assess beforehand
whether it will scan the indexes, or it can just assume that it will need
to scan the indexes. Perhaps VACUUM can pre-scan the VM to decide how big a
task it has before it starts.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello,

I think it'd be better to combine both numbers into one report:
It'd also be good to standardize on where the * 100 is happening.

Done

can be replaced by
(itemptr->ipblkid != vacrelstats->last_scanned_page)

Get compiler error : invalid operands to binary != (have ‘BlockIdData’ and ‘BlockIdData’)

vacrelstats->current_index_scanned_page_count++;

Done
Please find v3 attached.

I am struggling to create maintenance work memory exhaustion. Did the following
maintenance_work_mem=1MB.
Inserted 10 million records in tbl1 with 3 indexes. Deleted 5 million and vacuumed. So far no error. I could keep bumping up the records to say 100 million and try to get this error.
This seems a tedious manner to simulate maintenance work memory exhaustion. Is there a better way?
To insert I am using COPY (from a csv which has 10 million records) and building indexes after insert is complete.
Thank you
Sameer

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

Thakur, Sameer wrote:

Hello,

I think it'd be better to combine both numbers into one report:
It'd also be good to standardize on where the * 100 is happening.

Done

can be replaced by
(itemptr->ipblkid != vacrelstats->last_scanned_page)

Get compiler error : invalid operands to binary != (have ‘BlockIdData’ and ‘BlockIdData’)

vacrelstats->current_index_scanned_page_count++;

Done
Please find v3 attached.

I am struggling to create maintenance work memory exhaustion. Did the following
maintenance_work_mem=1MB.
Inserted 10 million records in tbl1 with 3 indexes. Deleted 5 million and vacuumed. So far no error. I could keep bumping up the records to say 100 million and try to get this error.
This seems a tedious manner to simulate maintenance work memory exhaustion. Is there a better way?
To insert I am using COPY (from a csv which has 10 million records) and building indexes after insert is complete.

I don't think there's any maintenance work exhaustion that results in an
error. The system is designed to use all the memory it is allowed to,
and to have other strategies when it's not sufficient to do the whole
sort.

Not sure what Jim meant. Maybe he meant to be aware of when spilling to
disk happens? Obviously, things become slower, so maybe you need to
consider it for progress reporting purposes.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Jul 22, 2015 at 8:19 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Not sure what Jim meant. Maybe he meant to be aware of when spilling to
disk happens? Obviously, things become slower, so maybe you need to
consider it for progress reporting purposes.

Perhaps the m_w_m determines how many dead tuples lazy_scan_heap() can
keep track of before doing a lazy_vacuum_indexes() +
lazy_vacuum_heap() round. Smaller the m_w_m, more the number of index
scans, slower the progress?

Thanks,
Amit

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

On Wed, Jul 22, 2015 at 3:02 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

For me, the user workflow looks like these....

Worried: "Task X is taking ages? When is it expected to finish?"
Ops: 13:50
<sometime later, about 14:00>
Worried: "Task X is still running? But I thought its ETA was 13:50?"
Ops: Now says 14:30
Worried: "Is it stuck, or is it making progress?"
Ops: Looks like its making progress
Worried: "Can we have a look at it and find out what its doing?"

How does Ops know that it is making progress? Just because the
completion percentage is changing?

In terms of VACUUM specifically: VACUUM should be able to assess beforehand
whether it will scan the indexes, or it can just assume that it will need to
scan the indexes. Perhaps VACUUM can pre-scan the VM to decide how big a
task it has before it starts.

Well, we can assume that it will scan the indexes exactly once, but
the actual number may be more or less; and the cost of rescanning the
heap in phase 2 is also hard to estimate.

Maybe I'm worrying over nothing, but I have a feeling that if we try
to do what you're proposing here, we're gonna end up with this:

https://xkcd.com/612/

Most of the progress estimators I have seen over the ~30 years that
I've been playing with computers have been unreliable, and many of
those have been unreliable to the point of being annoying. I think
that's likely to happen with what you are proposing too, though of
course like all predictions of the future it could turn out to be
wrong.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 22 July 2015 at 13:00, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Jul 22, 2015 at 3:02 AM, Simon Riggs <simon@2ndquadrant.com>
wrote:

For me, the user workflow looks like these....

Worried: "Task X is taking ages? When is it expected to finish?"
Ops: 13:50
<sometime later, about 14:00>
Worried: "Task X is still running? But I thought its ETA was 13:50?"
Ops: Now says 14:30
Worried: "Is it stuck, or is it making progress?"
Ops: Looks like its making progress
Worried: "Can we have a look at it and find out what its doing?"

How does Ops know that it is making progress? Just because the
completion percentage is changing?

You could, but that is not the way I suggested.

We need
* Some measure of actual progress (the definition of which may vary from
action to action, e.g. blocks scanned)
* Some estimate of the total work required
* An estimate of the estimated time of completion - I liked your view that
this prediction may be costly to request

In terms of VACUUM specifically: VACUUM should be able to assess

beforehand

whether it will scan the indexes, or it can just assume that it will

need to

scan the indexes. Perhaps VACUUM can pre-scan the VM to decide how big a
task it has before it starts.

Well, we can assume that it will scan the indexes exactly once, but
the actual number may be more or less; and the cost of rescanning the
heap in phase 2 is also hard to estimate.

Maybe I'm worrying over nothing, but I have a feeling that if we try
to do what you're proposing here, we're gonna end up with this:

https://xkcd.com/612/

Most of the progress estimators I have seen over the ~30 years that
I've been playing with computers have been unreliable, and many of
those have been unreliable to the point of being annoying. I think
that's likely to happen with what you are proposing too, though of
course like all predictions of the future it could turn out to be
wrong.

Almost like an Optimizer then. Important, often annoyingly wrong, needs
more work.

I'm not proposing this feature, I'm merely asking for it to be defined in a
way that makes it work for more than just VACUUM. Once we have a way of
reporting useful information, other processes can be made to follow that
mechanism, like REINDEX, ALTER TABLE etc.. I believe those things are
important, even if we never get such information for user queries. But I
hope we do.

I won't get in the way of your search for detailed information in more
complex forms. Both things are needed.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jul 22, 2015 at 8:24 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

* An estimate of the estimated time of completion - I liked your view that
this prediction may be costly to request

I'm saying it may be massively unreliable, not that it may be costly.
(Someone else may have said that it would be costly, but I don't think
it was me.)

Most of the progress estimators I have seen over the ~30 years that
I've been playing with computers have been unreliable, and many of
those have been unreliable to the point of being annoying. I think
that's likely to happen with what you are proposing too, though of
course like all predictions of the future it could turn out to be
wrong.

Almost like an Optimizer then. Important, often annoyingly wrong, needs more
work.

Yes, but with an important difference. If the optimizer mis-estimates
the row count by 3x or 10x or 1000x, but the plan is OK anyway, it's
often the case that no one cares. Except when the plan is bad, people
don't really care about the method used to derive it. The same is not
true here: people will rely on the progress estimates directly, and
they will really care if they are not right.

I'm not proposing this feature, I'm merely asking for it to be defined in a
way that makes it work for more than just VACUUM. Once we have a way of
reporting useful information, other processes can be made to follow that
mechanism, like REINDEX, ALTER TABLE etc.. I believe those things are
important, even if we never get such information for user queries. But I
hope we do.

I won't get in the way of your search for detailed information in more
complex forms. Both things are needed.

OK.

One idea I have is to create a system where we expose a command tag
(i.e. VACUUM) plus a series of generic fields whose specific meanings
are dependent on the command tag. Say, 6 bigint counters, 6 float8
counters, and 3 strings up to 80 characters each. So we have a
fixed-size chunk of shared memory per backend, and each backend that
wants to expose progress information can fill in those fields however
it likes, and we expose the results.

This would be sorta like the way pg_statistic works: the same columns
can be used for different purposes depending on what estimator will be
used to access them.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 7/22/15 6:58 AM, Amit Langote wrote:

On Wed, Jul 22, 2015 at 8:19 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Not sure what Jim meant. Maybe he meant to be aware of when spilling to
disk happens? Obviously, things become slower, so maybe you need to
consider it for progress reporting purposes.

Perhaps the m_w_m determines how many dead tuples lazy_scan_heap() can
keep track of before doing a lazy_vacuum_indexes() +
lazy_vacuum_heap() round. Smaller the m_w_m, more the number of index
scans, slower the progress?

Yes. Any percent completion calculation will have to account for the
case of needing multiple passes through all the indexes.

Each dead tuple requires 6 bytes (IIRC) of maintenance work mem. So if
you're deleting 5M rows with m_w_m=1MB you should be getting many passes
through the indexes. Studying the output of VACUUM VERBOSE will confirm
that (or just throw a temporary WARNING in the path where we start the
scan).
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On 7/22/15 9:15 AM, Robert Haas wrote:

I'm not proposing this feature, I'm merely asking for it to be defined in a

way that makes it work for more than just VACUUM. Once we have a way of
reporting useful information, other processes can be made to follow that
mechanism, like REINDEX, ALTER TABLE etc.. I believe those things are
important, even if we never get such information for user queries. But I
hope we do.

I won't get in the way of your search for detailed information in more
complex forms. Both things are needed.

OK.

One idea I have is to create a system where we expose a command tag
(i.e. VACUUM) plus a series of generic fields whose specific meanings
are dependent on the command tag. Say, 6 bigint counters, 6 float8
counters, and 3 strings up to 80 characters each. So we have a
fixed-size chunk of shared memory per backend, and each backend that
wants to expose progress information can fill in those fields however
it likes, and we expose the results.

This would be sorta like the way pg_statistic works: the same columns
can be used for different purposes depending on what estimator will be
used to access them.

If we want to expose that level of detail, I think either JSON or arrays
would make more sense, so we're not stuck with a limited amount of info.
Perhaps DDL would be OK with the numbers you suggested, but
https://www.pgcon.org/2013/schedule/events/576.en.html would not, and I
think wanting query progress is much more common.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Hello,

Yes. Any percent completion calculation will have to account for the case of needing multiple passes through all the indexes.

Each dead tuple requires 6 bytes (IIRC) of maintenance work mem. So if you're deleting 5M rows with m_w_m=1MB you should be getting many passes through the indexes. >Studying the output of VACUUM VERBOSE will confirm that (or just throw a temporary WARNING in the path where we start the scan).

Yes I see the problem now. I get the message "WARNING: Overall index percentage completion 100.000000" logged > 25 times while vacuuming after 5 million records deleted.
Figuring out number of multiple index passes beforehand, accurately, is the problem to solve. Clearly need to study this some more.
Thank you,
Sameer Thakur | Senior Software Specialist | NTTDATA Global Delivery Services Private Ltd | w. +91.20.6641.7146 | VoIP: 8834.8146 | m. +91 989.016.6656 | sameer.thakur@nttdata.com | Follow us on Twitter@NTTDATAAmericas

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

Hello,

logged > 25 times

Sorry, it is much lower at 7 times. Does not change overall point though
regards
Sameer Thakur | Senior Software Specialist | NTTDATA Global Delivery Services Private Ltd | w. +91.20.6641.7146 | VoIP: 8834.8146 | m. +91 989.016.6656 | sameer.thakur@nttdata.com | Follow us on Twitter@NTTDATAAmericas

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

On Wed, Jul 22, 2015 at 11:28 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

If we want to expose that level of detail, I think either JSON or arrays
would make more sense, so we're not stuck with a limited amount of info.
Perhaps DDL would be OK with the numbers you suggested, but
https://www.pgcon.org/2013/schedule/events/576.en.html would not, and I
think wanting query progress is much more common.

You need to restrict the amount of info, because you've got to
preallocate enough shared memory to store all the data that somebody
might report.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 7/23/15 2:43 PM, Robert Haas wrote:

On Wed, Jul 22, 2015 at 11:28 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

If we want to expose that level of detail, I think either JSON or arrays
would make more sense, so we're not stuck with a limited amount of info.
Perhaps DDL would be OK with the numbers you suggested, but
https://www.pgcon.org/2013/schedule/events/576.en.html would not, and I
think wanting query progress is much more common.

You need to restrict the amount of info, because you've got to
preallocate enough shared memory to store all the data that somebody
might report.

I was thinking your DSM stuff would come into play here. We wouldn't
want to be reallocating during execution, but I'd expect we would know
during setup how much memory we actually needed.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On Fri, Jul 24, 2015 at 2:00 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

You need to restrict the amount of info, because you've got to
preallocate enough shared memory to store all the data that somebody
might report.

I was thinking your DSM stuff would come into play here. We wouldn't want to
be reallocating during execution, but I'd expect we would know during setup
how much memory we actually needed.

You could make that work, but it would be a pretty significant amount
of new mechanism. Also, if it's to be practical to report progress
frequently, it's got to be cheap, and that precludes reporting vast
volumes of data.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 7/23/15 5:18 AM, Thakur, Sameer wrote:

Hello,

logged > 25 times

Sorry, it is much lower at 7 times. Does not change overall point though

I think it's related to the problem of figuring out how many dead tuples
you expect to find in the overall heap, which you need to do to have any
hope of this being a comprehensive estimate.

My inclination at this point is to provide a simple means of providing
the raw numbers and let users test it in the wild. A really crude method
of doing that might be to trap SIGINFO (if we're not using it already)
and elog current status.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On 07/24/2015 11:06 AM, Jim Nasby wrote:

On 7/23/15 5:18 AM, Thakur, Sameer wrote:

Hello,

logged > 25 times

Sorry, it is much lower at 7 times. Does not change overall point though

I think it's related to the problem of figuring out how many dead tuples
you expect to find in the overall heap, which you need to do to have any
hope of this being a comprehensive estimate.

What about just reporting scanned pages/total pages ? That would be
easy and cheap to track. It would result in some herky-jerky
"progress", but would still be an improvement over the feedback we don't
have now.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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

​Something like that would be helpful. I just had to stop one after an
hour and have no idea how much longer it would have taken.​

__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike.Blackwell@rrd.com
http://www.rrdonnelley.com

<http://www.rrdonnelley.com/&gt;
* <Mike.Blackwell@rrd.com>*

On Fri, Jul 24, 2015 at 5:41 PM, Josh Berkus <josh@agliodbs.com> wrote:

2015-07-25 0:41 GMT+02:00 Josh Berkus <josh@agliodbs.com>:

On 07/24/2015 11:06 AM, Jim Nasby wrote:

On 7/23/15 5:18 AM, Thakur, Sameer wrote:

Hello,

logged > 25 times

Sorry, it is much lower at 7 times. Does not change overall point though

I think it's related to the problem of figuring out how many dead tuples
you expect to find in the overall heap, which you need to do to have any
hope of this being a comprehensive estimate.

What about just reporting scanned pages/total pages ? That would be
easy and cheap to track. It would result in some herky-jerky
"progress", but would still be an improvement over the feedback we don't
have now.

I like this idea.

Regards

Pavel

I think it's related to the problem of figuring out how many dead tuples

you expect to find in the overall heap, which you need to do to have >any
hope of this being a comprehensive estimate.

An estimate of number of index scans while vacuuming can be done using
estimate of total dead tuples in the relation and maintenance work mem.
n_dead_tuples in pg_stat_all_tables can be used as an estimate of dead
tuples.

Following can be a way to estimate,

if nindexes == 0
index_scans =0
else if pages_all_visible
index_scans =0
else
index_scans = Max((n_dead_tuples * space occupied by single dead
tuple)/m_w_m,1)

This estimates index_scans = 1 if n_dead_tuples = 0 assuming lazy scan heap
is likely to find some dead_tuples.
If n_dead_tuples is non zero the above estimate gives a lower bound on
number of index scans possible.

Thank you,
Rahila Syed

I think the only way to produce usable estimates is to report more than
one number. And in the particular case of lazy vacuuming, ISTM the way
to do it is to consider heap scanning as one phase, index cleanup as
another phase; these two phases can be interleaved. And there's a final
heap scan which is a third phase, which can only run after phases one
and two are complete.

So you would report either "we're in phases one/two" or "we're in phase
three". If we're in phases one/two, then we need to report

1. what's the page number of heap scan (i.e. how much more do we have to
go yet?)
2. how many index scans have we done so far
3. if phase two, how many index pages have we scanned (total, i.e.
across all indexes).

The total number of heap pages is known, and the total number of index
pages is also known, so it's possible to derive a percentage out of
this part. Maybe it would be useful to report how much time it's been
in phases one and two respectively; with that, I think it is possible to
extrapolate the total time.

If we're in third phase, we report the heap page number we're in.

This looks pretty complicated to understand from the user POV, but
anything other than this seems to me too simplistic to be of any use.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 07/31/2015 11:21 AM, Alvaro Herrera wrote:

This looks pretty complicated to understand from the user POV, but
anything other than this seems to me too simplistic to be of any use.

I would agree and I don't think it is all that complicated. This is an
RDBMS not a web browser downloading a file.

JD

--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.

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

On Fri, Jul 31, 2015 at 2:21 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

I think the only way to produce usable estimates is to report more than
one number. And in the particular case of lazy vacuuming, ISTM the way
to do it is to consider heap scanning as one phase, index cleanup as
another phase; these two phases can be interleaved. And there's a final
heap scan which is a third phase, which can only run after phases one
and two are complete.

That's not really right. There's a phase three for each phase two.

Put in terms of the code, what we're calling phase one is
lazy_scan_heap(), which prunes all pages, sets hint bits, collects
dead TIDs, and maybe marks the page all-visible.

When lazy_scan_heap() fills up maintenance_work_mem, or when it
reaches the end of the heap, it does phase two, which is
lazy_vacuum_index(), and phase three, which is lazy_vacuum_heap().
Phase one - lazy_scan_heap() - then keeps going from where it left
off.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Robert Haas wrote:

On Fri, Jul 31, 2015 at 2:21 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

I think the only way to produce usable estimates is to report more than
one number. And in the particular case of lazy vacuuming, ISTM the way
to do it is to consider heap scanning as one phase, index cleanup as
another phase; these two phases can be interleaved. And there's a final
heap scan which is a third phase, which can only run after phases one
and two are complete.

That's not really right. There's a phase three for each phase two.

Put in terms of the code, what we're calling phase one is
lazy_scan_heap(), which prunes all pages, sets hint bits, collects
dead TIDs, and maybe marks the page all-visible.

When lazy_scan_heap() fills up maintenance_work_mem, or when it
reaches the end of the heap, it does phase two, which is
lazy_vacuum_index(), and phase three, which is lazy_vacuum_heap().
Phase one - lazy_scan_heap() - then keeps going from where it left
off.

Hmm, you're right. I don't think it changes the essence of what I
suggest, though.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

The total number of heap pages is known, and the total number of index
pages is also known, so it's possible to derive a percentage out of
this part.

The total number of index pages scanned during entire vacuum will depend on
number
of index scans that happens.
In order to extrapolate percent complete for phase two(index scan) we need
number of index scans * total index pages.
The number of index scans can vary from 1 to n (n depending on
maintenance_work_mem)

Summarizing suggestions in previous mails, following information can be
reported
Phase 1.heap pages scanned / total heap pages
Phase 2.index pages scanned / total index pages (across all indexes)
Phase 3.count of heap pages vacuumed

Additional info like number of index scans so far, number of dead tuples
being vacuumed in one batch can also be provided.

A combined estimate for vacuum percent complete can be provided by summing
up heap pages scanned, index pages scanned against total heap pages, total
index pages * number of index scans.

Thank you,
Rahila Syed

Hello,

Say, 6 bigint counters, 6 float8
counters, and 3 strings up to 80 characters each. So we have a
fixed-size chunk of shared memory per backend, and each backend that
wants to expose progress information can fill in those fields however
it likes, and we expose the results.
This would be sorta like the way pg_statistic works: the same columns
can be used for different purposes depending on what estimator will be
used to access them.

After thinking more on this suggestion, I came up with following generic
structure which can be used to store progress of any command per backend in
shared memory.

Struct PgBackendProgress
{
int32 *counter[COMMAND_NUM_SLOTS];
float8 *counter_float[COMMAND_NUM_SLOTS];

char *progress_message[COMMAND_NUM_SLOTS];
}

COMMAND_NUM_SLOTS will define maximum number of slots(phases) for any
command.
Progress of command will be measured using progress of each phase in
command.
For some command the number of phases can be singular and rest of the slots
will be NULL.

Each phase will report n integer counters, n float counters and a progress
message.
For some phases , any of the above fields can be NULL.

For VACUUM , there can 3 phases as discussed in the earlier mails.

Phase 1. Report 2 integer counters: heap pages scanned and total heap
pages, 1 float counter: percentage_complete and progress message.
Phase 2. Report 2 integer counters: index pages scanned and total index
pages(across all indexes) and progress message.
Phase 3. 1 integer counter: heap pages vacuumed.

This structure can be accessed by statistics collector to display progress
via new view.

Thank you,
Rahila Syed

On Mon, Aug 10, 2015 at 1:36 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello,

Say, 6 bigint counters, 6 float8
counters, and 3 strings up to 80 characters each. So we have a
fixed-size chunk of shared memory per backend, and each backend that
wants to expose progress information can fill in those fields however
it likes, and we expose the results.
This would be sorta like the way pg_statistic works: the same columns
can be used for different purposes depending on what estimator will be
used to access them.

After thinking more on this suggestion, I came up with following generic
structure which can be used to store progress of any command per backend in
shared memory.

Struct PgBackendProgress
{
int32 *counter[COMMAND_NUM_SLOTS];
float8 *counter_float[COMMAND_NUM_SLOTS];

char *progress_message[COMMAND_NUM_SLOTS];
}

COMMAND_NUM_SLOTS will define maximum number of slots(phases) for any
command.
Progress of command will be measured using progress of each phase in
command.
For some command the number of phases can be singular and rest of the slots
will be NULL.

Each phase will report n integer counters, n float counters and a progress
message.
For some phases , any of the above fields can be NULL.

For VACUUM , there can 3 phases as discussed in the earlier mails.

Phase 1. Report 2 integer counters: heap pages scanned and total heap pages,
1 float counter: percentage_complete and progress message.
Phase 2. Report 2 integer counters: index pages scanned and total index
pages(across all indexes) and progress message.
Phase 3. 1 integer counter: heap pages vacuumed.

This structure can be accessed by statistics collector to display progress
via new view.

I have one question about this.

When we're in Phase2 or 3, don't we need to report the number of total
page scanned or percentage of how many table pages scanned, as well?
As Robert said, Phase2(means lazy_vacuum_index here) and 3(means
lazy_vacuum_heap here) could be called whenever lazy_scan_heap fills
up the maintenance_work_mem. And phase 3 could be called at the end of
scanning single page if table doesn't have index.
So if vacuum progress checker reports the only current phase
information, we would not be able to know where we are in now.

Regards,

--
Masahiko Sawada

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

Hello,

When we're in Phase2 or 3, don't we need to report the number of total page scanned or percentage of how many table pages scanned, as well?

The total heap pages scanned need to be reported with phase 2 or 3. Complete progress report need to have numbers from each phase when applicable.

Phase 1. Report 2 integer counters: heap pages scanned and total heap
pages,
1 float counter: percentage_complete and progress message.
Phase 2. Report 2 integer counters: index pages scanned and total
index pages(across all indexes) and progress message.
Phase 3. 1 integer counter: heap pages vacuumed.

Sorry for being unclear here. What I meant to say is, each phase of a command will correspond to a slot in COMMAND_NUM_SLOTS. Each phase will be a separate array element and
will comprise of n integers, n floats, string. So , in the view reporting progress, VACUUM command can have 3 entries one for each phase.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

On 10 August 2015 at 15:59, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

When we're in Phase2 or 3, don't we need to report the number of total

page scanned or percentage of how many table pages scanned, as well?
The total heap pages scanned need to be reported with phase 2 or 3.
Complete progress report need to have numbers from each phase when
applicable.

Phase 1. Report 2 integer counters: heap pages scanned and total heap
pages,
1 float counter: percentage_complete and progress message.
Phase 2. Report 2 integer counters: index pages scanned and total
index pages(across all indexes) and progress message.
Phase 3. 1 integer counter: heap pages vacuumed.

Sorry for being unclear here. What I meant to say is, each phase of a
command will correspond to a slot in COMMAND_NUM_SLOTS. Each phase will be
a separate array element and
will comprise of n integers, n floats, string. So , in the view reporting
progress, VACUUM command can have 3 entries one for each phase.

VACUUM has 3 phases now, but since phases 2 and 3 repeat, you can have an
unbounded number of phases. But that assumes that we don't count truncation
as a 4th phase of VACUUM...

SELECT statements also have a variable number of phases, hash, materialize,
sorts all act as blocking nodes where you cannot progress to next phase
until it is complete and you don't know for certain how much data will come
in later phases.

I think the best you'll do is an array of pairs of values [(current blocks,
total blocks), ... ]

Knowing how many phases there are is a tough problem. I think the only way
forwards is to admit that we will publish our best initial estimate of
total workload size and then later we may realise it was wrong and publish
a better number (do until complete). It's not wonderful, but la vida es
loca.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Aug 11, 2015 at 12:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 10 August 2015 at 15:59, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

When we're in Phase2 or 3, don't we need to report the number of total
page scanned or percentage of how many table pages scanned, as well?

The total heap pages scanned need to be reported with phase 2 or 3.
Complete progress report need to have numbers from each phase when
applicable.

Phase 1. Report 2 integer counters: heap pages scanned and total heap
pages,
1 float counter: percentage_complete and progress message.
Phase 2. Report 2 integer counters: index pages scanned and total
index pages(across all indexes) and progress message.
Phase 3. 1 integer counter: heap pages vacuumed.

Sorry for being unclear here. What I meant to say is, each phase of a
command will correspond to a slot in COMMAND_NUM_SLOTS. Each phase will be a
separate array element and
will comprise of n integers, n floats, string. So , in the view reporting
progress, VACUUM command can have 3 entries one for each phase.

VACUUM has 3 phases now, but since phases 2 and 3 repeat, you can have an
unbounded number of phases. But that assumes that we don't count truncation
as a 4th phase of VACUUM...

Yeah.
This topic may have been already discussed but, why don't we use just
total scanned pages and total pages?

The mechanism of VACUUM is complicated a bit today, and other
maintenance command is as well.
It would be tough to trace these processing, and these might be
changed in the future.
But basically, we can trace total scanned pages of target relation
easily, and such information would be enough at many case.

Regards,

--
Masahiko Sawada

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

Masahiko Sawada wrote:

This topic may have been already discussed but, why don't we use just
total scanned pages and total pages?

Because those numbers don't extrapolate nicely. If the density of dead
tuples is irregular across the table, such absolute numbers might be
completely meaningless: you could scan 90% of the table without seeing
any index scan, and then at the final 10% be hit by many index scans
cleaning dead tuples. Thus you would see progress go up to 90% very
quickly and then take hours to have it go to 91%. (Additionally, and a
comparatively minor point: since you don't know how many index scans are
going to happen, there's no way to know the total number of blocks
scanned, unless you don't count index blocks at all, and then the
numbers become a lie.)

If you instead track number of heap pages separately from index pages,
and indicate how many index scans have taken place, you have a chance of
actually figuring out how many heap pages are left to scan and how many
more index scans will occur.

The mechanism of VACUUM is complicated a bit today,

Understatement of the week ...

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Aug 11, 2015 at 1:50 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Masahiko Sawada wrote:

This topic may have been already discussed but, why don't we use just
total scanned pages and total pages?

Because those numbers don't extrapolate nicely. If the density of dead
tuples is irregular across the table, such absolute numbers might be
completely meaningless: you could scan 90% of the table without seeing
any index scan, and then at the final 10% be hit by many index scans
cleaning dead tuples. Thus you would see progress go up to 90% very
quickly and then take hours to have it go to 91%. (Additionally, and a
comparatively minor point: since you don't know how many index scans are
going to happen, there's no way to know the total number of blocks
scanned, unless you don't count index blocks at all, and then the
numbers become a lie.)
If you instead track number of heap pages separately from index pages,
and indicate how many index scans have taken place, you have a chance of
actually figuring out how many heap pages are left to scan and how many
more index scans will occur.

Thank you for your explanation!
I understood about this.

VACUUM has 3 phases now, but since phases 2 and 3 repeat, you can have an unbounded number of phases. But that assumes that we don't count truncation as a 4th phase of VACUUM...

In case of vacuum, I think we need to track the number of scanned heap
pages at least, and the information about index scan is the additional
information.
The another idea for displaying progress is to have two kind of
information: essential information and additional information.

Essential information has one numeric data, which is stored
essentially information regarding of its processing.
Additional information has two data: text and numeric. These data is
free-style data which is stored by each backend as it like.
And these three data are output at same time.

For example, In case of vacuum, essential information is the number of
total scanned heap page.

* When lazy_scan_heap starts, the two additional data are NULL.

* When lazy_vacuum_index starts, the backend set additional data like
followings.
- "Index vacuuming" into text data which describes what we're doing
now actually.
- "50" into numeric data which describes how many index pages we scanned.

* And when lazy_vacuum_index is done, backend sets additional data NULL again.

Regards,

--
Masahiko Sawada

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

On 10 August 2015 at 17:50, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Masahiko Sawada wrote:

This topic may have been already discussed but, why don't we use just
total scanned pages and total pages?

Because those numbers don't extrapolate nicely. If the density of dead
tuples is irregular across the table, such absolute numbers might be
completely meaningless: you could scan 90% of the table without seeing
any index scan, and then at the final 10% be hit by many index scans
cleaning dead tuples. Thus you would see progress go up to 90% very
quickly and then take hours to have it go to 91%. (Additionally, and a
comparatively minor point: since you don't know how many index scans are
going to happen, there's no way to know the total number of blocks
scanned, unless you don't count index blocks at all, and then the
numbers become a lie.)

If you instead track number of heap pages separately from index pages,
and indicate how many index scans have taken place, you have a chance of
actually figuring out how many heap pages are left to scan and how many
more index scans will occur.

I think this overstates the difficulty.

Autovacuum knows what % of a table needs to be cleaned - that is how it is
triggered. When a vacuum runs we should calculate how many TIDs we will
collect and therefore how many trips to the indexes we need for given
memory. We can use the VM to find out how many blocks we'll need to scan in
the table. So overall, we know how many blocks we need to scan.

I think just storing (total num blocks, scanned blocks) is sufficiently
accurate to be worth holding, rather than make it even more complex.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello,

Autovacuum knows what % of a table needs to be cleaned - that is how it is triggered.
When a vacuum runs we should calculate how many TIDs we will collect and therefore how many trips to the indexes we need for given memory.
We can use the VM to find out how many blocks we'll need to scan in the table. So overall, we know how many blocks we need to scan.

Total heap pages to be scanned can be obtained from VM as mentioned. To figure out number of index scans we need an estimate of dead tuples.

IIUC, autovacuum acquires information that a table has to be cleaned by looking at pgstat entry for the table. i.e n_dead_tuples.
Hence,initial estimate of dead tuple TIDs can be made using n_dead_tuples in pgstat.
n_dead_tuples in pgstat table entry is the value updated by last analyze and may not be up to date.
In cases where pgstat entry for table is NULL, number of dead tuples TIDs cannot be estimated.
In such cases where TIDs cannot be estimated , we can start with an initial estimate of 1 index scan and later go on adding number of index pages to the total count of pages(heap+index) if count of index scan exceeds.

Thank you,
Rahila Syed.

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

In case of vacuum, I think we need to track the number of scanned heap
pages at least, and the information about index scan is the additional
information

Actually the progress of heap pages scan depend on index scans. So complete
VACUUM progress
needs to have a count of index pages scanned too. So, progress can be
calculated by measuring index_pages_scanned + heap_pages_scanned
against total_index_pages + total_heap_pages. This can make essential
information.

This can be followed by additional individual phase information.

Following fields common across different commands can be used to display
progress

Command work done total work percent complete message

VACUUM x y z
total progress

u v w
phase 1

The command code can be divided into distinct phases and each phase
progress can be represented separately. With a summary of entire command
progress as the first entry. The summary can be the summation of individual
phase entries.

If the phase repeats during command execution the previous entry for the
phase will be replaced.(for ex. index scan in vacuum)

Essential information has one numeric data, which is stored
essentially information regarding of its processing.

We may need more than one numeric data as mentioned above to represent
scanned blocks versus total blocks.

Additional information has two data: text and numeric. These data is
free-style data which is stored by each backend as it like.

If I understand your point correctly, I think you are missing following,
The amount of additional information for each command can be different. We
may need an array of text and numeric data to represent more additional
information.

Thank you,
Rahila Syed

On 8/17/15 5:07 PM, Rahila Syed wrote:

In case ofvacuum, I think we need to track the number of scanned heap
pages at least, and the information about index scan is the additional
information

Actually the progress of heap pages scan depend on index scans. So
complete VACUUM progress
needs to have a count of index pages scanned too. So, progress can be
calculated by measuring index_pages_scanned + heap_pages_scanned
against total_index_pages + total_heap_pages. This can make essential
information.

There's absolutely no way to get a reasonable status report in the case
of multiple index passes unless you somehow count the passes, especially
since index cleanup is frequently MUCH longer than the heap cleanup.

What should work is exporting the number of index passes we've already
made. If > 0 we know we're in a multiple scan situation. At the end of
each index pass, do index_passes++; index_pages=0; index_pages_scanned=0.

Personally, I think we should use SIGINFO to signal a backend to output
status data to a file in pg_stat_tmp/ (but not the main stats file) and
be done with it. That allows us to easily handle variable length stuff
with minimal fuss. No normal user is going to hammer away at that, and
anyone that's really worried about performance will have that directory
sitting on a ramdisk anyway.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On Mon, Aug 10, 2015 at 12:36 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello,

Say, 6 bigint counters, 6 float8
counters, and 3 strings up to 80 characters each. So we have a
fixed-size chunk of shared memory per backend, and each backend that
wants to expose progress information can fill in those fields however
it likes, and we expose the results.
This would be sorta like the way pg_statistic works: the same columns
can be used for different purposes depending on what estimator will be
used to access them.

After thinking more on this suggestion, I came up with following generic
structure which can be used to store progress of any command per backend in
shared memory.

Struct PgBackendProgress
{
int32 *counter[COMMAND_NUM_SLOTS];
float8 *counter_float[COMMAND_NUM_SLOTS];

char *progress_message[COMMAND_NUM_SLOTS];
}

This can't actually work, because we don't have a dynamic allocator
for shared memory. What you need to do is something like this:

struct PgBackendProgress
{
uint64 progress_integer[N_PROGRESS_INTEGER];
float8 progress_float[N_PROGRESS_FLOAT];
char progress_string[PROGRESS_STRING_LENGTH][N_PROGRESS_STRING];
};

You probably want to protect this with the st_changecount protocol, or
just put the fields in PgBackendStatus.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Hello,

On Jul 16, 2015 1:48 AM, "Rahila Syed" <rahilasyed90@gmail.com> wrote:

Hello,

Please find attached updated patch >with an interface to calculate

command progress in pgstat.c. This interface currently
implements VACUUM progress tracking .

I have added this patch to CommitFest 2015-09. It is marked as
Waiting on author . I will post an updated patch as per review comments
soon.

Thank you,
Rahila Syed

Hello,

Please find attached updated VACUUM progress checker patch.
Following have been accomplished in the patch

1. Accounts for index pages count while calculating total progress of VACUUM.
2. Common location for storing progress parameters for any command. Idea is every command which needs to report progress can populate and interpret the shared variables in its own way.
Each can display progress by implementing separate views.
3. Separate VACUUM progress view to display various progress parameters has been implemented . Progress of various phases like heap scan, index scan, total pages scanned along with
completion percentage is reported.
4.This view can display progress for all active backends running VACUUM.

Basic testing has been performed. Thorough testing is yet to be done. Marking it as Needs Review in Sept-Commitfest.

ToDo:
Display count of heap pages actually vacuumed(marking line pointers unused)
Display percentage of work_mem being used to store dead tuples.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

On 11 September 2015 at 15:43, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

Please find attached updated VACUUM progress checker patch.
Following have been accomplished in the patch

1. Accounts for index pages count while calculating total progress of
VACUUM.
2. Common location for storing progress parameters for any command. Idea
is every command which needs to report progress can populate and interpret
the shared variables in its own way.
Each can display progress by implementing separate views.
3. Separate VACUUM progress view to display various progress parameters
has been implemented . Progress of various phases like heap scan, index
scan, total pages scanned along with
completion percentage is reported.
4.This view can display progress for all active backends running VACUUM.

Basic testing has been performed. Thorough testing is yet to be done.
Marking it as Needs Review in Sept-Commitfest.

ToDo:
Display count of heap pages actually vacuumed(marking line pointers unused)
Display percentage of work_mem being used to store dead tuples.

Thank you,
Rahila Syed

This doesn't seem to compile:

make[4]: Leaving directory `/home/thom/Development/postgresql/src/backend'
make[3]: Leaving directory `/home/thom/Development/postgresql/src/common'
make -C catalog schemapg.h
make[3]: Entering directory
`/home/thom/Development/postgresql/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3308
make[3]: *** [postgres.bki] Error 1
make[3]: Leaving directory
`/home/thom/Development/postgresql/src/backend/catalog'
make[2]: *** [submake-schemapg] Error 2
make[2]: Leaving directory `/home/thom/Development/postgresql/src/backend'
make[1]: *** [install-backend-recurse] Error 2
make[1]: Leaving directory `/home/thom/Development/postgresql/src'
make: *** [install-src-recurse] Error 2
make: Leaving directory `/home/thom/Development/postgresql'

Thom

This doesn't seem to compile

Oh. It compiled successfully when applied on HEAD on my machine. Anyways,
the OID is changed to 3309 in the attached patch. 3308 / 3309 both are part
of OIDs in unused OID list.

Thank you,
Rahila Syed

Rahila Syed wrote:

This doesn't seem to compile

Oh. It compiled successfully when applied on HEAD on my machine. Anyways,
the OID is changed to 3309 in the attached patch. 3308 / 3309 both are part
of OIDs in unused OID list.

I think Thom may have patched on top of some other patch.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 11 September 2015 at 22:34, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Rahila Syed wrote:

This doesn't seem to compile

Oh. It compiled successfully when applied on HEAD on my machine. Anyways,
the OID is changed to 3309 in the attached patch. 3308 / 3309 both are

part

of OIDs in unused OID list.

I think Thom may have patched on top of some other patch.

I think you might be right. I had run "git stash" and thought that would
be sufficient, but it seems "git clean -f" was necessary.

It builds fine now.

Thom

On 11 September 2015 at 15:43, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

Please find attached updated VACUUM progress checker patch.
Following have been accomplished in the patch

1. Accounts for index pages count while calculating total progress of
VACUUM.
2. Common location for storing progress parameters for any command. Idea
is every command which needs to report progress can populate and interpret
the shared variables in its own way.
Each can display progress by implementing separate views.
3. Separate VACUUM progress view to display various progress parameters
has been implemented . Progress of various phases like heap scan, index
scan, total pages scanned along with
completion percentage is reported.
4.This view can display progress for all active backends running VACUUM.

Basic testing has been performed. Thorough testing is yet to be done.
Marking it as Needs Review in Sept-Commitfest.

ToDo:
Display count of heap pages actually vacuumed(marking line pointers unused)
Display percentage of work_mem being used to store dead tuples.

Okay, I've just tested this with a newly-loaded table (1,252,973 of jsonb
data), and it works fine during a vacuum. I can see the scanned_pages,
scanned_heap_pages and percent_complete increasing, but after it's
finished, I end up with this:

json=# select * from pg_stat_vacuum_progress;
-[ RECORD 1 ]-------+-------
pid | 5569
total_pages | 217941
scanned_pages | 175243
total_heap_pages | 175243
scanned_heap_pages | 175243
total_index_pages | 42698
scanned_index_pages |
percent_complete | 80

This was running with a VACUUM ANALYZE. This output seems to suggest that
it didn't complete.

After, I ran VACUUM FULL. pg_stat_vacuum_progress didn't change from
before, so that doesn't appear to show up in the view.

I then deleted 40,000 rows from my table, and ran VACUUM ANALYZE again.
This time it progressed and percent_complete reached 100.

--
Thom

Hello Thom,

Okay, I've just tested this with a newly-loaded table (1,252,973 of jsonb data),

Thanks a lot!

but after it's finished, I end up with this:
json=# select * from pg_stat_vacuum_progress;
-[ RECORD 1 ]-------+-------
pid                 | 5569
total_pages         | 217941
scanned_pages       | 175243
total_heap_pages    | 175243
scanned_heap_pages  | 175243
total_index_pages   | 42698
scanned_index_pages |
percent_complete    | 80
This was running with a VACUUM ANALYZE.  This output seems to suggest that it didn't complete.

Ok. The patch fails here because 'total pages to be scanned' takes into account index pages and no index pages are actually scanned.
So the scanned pages count does not reach total pages count . I will fix this.
It seems that no index pages were scanned during this because there were no dead tuples to be cleaned as the table was newly loaded.

After, I ran VACUUM FULL.  pg_stat_vacuum_progress didn't change from before, so that doesn't appear to show up in the view.

The scope of this patch is to report progress of basic VACUUM . It does not take into account VACUUM FULL yet. I think this can be included after basic VACUUM progress is done.

I then deleted 40,000 rows from my table, and ran VACUUM ANALYZE again. This time it progressed and percent_complete reached 100

OK.

Thank you,
Rahila Syed.

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

On Tue, Sep 15, 2015 at 11:35 PM, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello Thom,

Okay, I've just tested this with a newly-loaded table (1,252,973 of jsonb data),

Thanks a lot!

but after it's finished, I end up with this:
json=# select * from pg_stat_vacuum_progress;
-[ RECORD 1 ]-------+-------
pid | 5569
total_pages | 217941
scanned_pages | 175243
total_heap_pages | 175243
scanned_heap_pages | 175243
total_index_pages | 42698
scanned_index_pages |
percent_complete | 80
This was running with a VACUUM ANALYZE. This output seems to suggest that it didn't complete.

Ok. The patch fails here because 'total pages to be scanned' takes into account index pages and no index pages are actually scanned.
So the scanned pages count does not reach total pages count . I will fix this.
It seems that no index pages were scanned during this because there were no dead tuples to be cleaned as the table was newly loaded.

After, I ran VACUUM FULL. pg_stat_vacuum_progress didn't change from before, so that doesn't appear to show up in the view.

The scope of this patch is to report progress of basic VACUUM . It does not take into account VACUUM FULL yet. I think this can be included after basic VACUUM progress is done.

I then deleted 40,000 rows from my table, and ran VACUUM ANALYZE again. This time it progressed and percent_complete reached 100

OK.

I tested this patch with some cases.
And the followings seems to be bug.

* After running "pgbench -i -s 100" and "VACUUM FREEZE
pgbench_accounts", the pg_stat_vacuum_progress is,

-[ RECORD 1 ]-------+-------
pid | 2298
total_pages | 27422
scanned_pages | 163935
total_heap_pages |
scanned_heap_pages | 163935
total_index_pages | 27422
scanned_index_pages |
percent_complete | 597

The value of percent_complete column exceeds 100%.
And, why are the total_heap_pages and scanned_index_pages columns NULL?

* Also, after dropping primary key of pgbench_accounts, I got
assertion error when I execute "VACUUM FREEZE pgbench_accounts".

=# VACUUM FREEZE pgbench_accounts;
ERROR: floating-point exception
DETAIL: An invalid floating-point operation was signaled. This
probably means an out-of-range result or an invalid operation, such as
division by zero.
STATEMENT: vacuum freeze pgbench_accounts ;
TRAP: FailedAssertion("!((beentry->st_changecount & 1) == 0)", File:
"pgstat.c", Line: 2934)

* The progress of vacuum by autovacuum seems not to be displayed.

Regards,

--
Masahiko Sawada

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

Hello,

Please find attached patch with bugs reported by Thom and Sawada-san solved.

* The progress of vacuum by autovacuum seems not to be displayed.

The progress is stored in shared variables during autovacuum. I guess the reason they are not visible is that the entries are deleted as soon as the process exits.
But the progress can be viewed while autovacuum worker is running.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

On Wed, Sep 23, 2015 at 12:24 AM, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

Please find attached patch with bugs reported by Thom and Sawada-san solved.

* The progress of vacuum by autovacuum seems not to be displayed.

The progress is stored in shared variables during autovacuum. I guess the reason they are not visible is that the entries are deleted as soon as the process exits.
But the progress can be viewed while autovacuum worker is running.

Thank you for updating the patch.

I tested the latest version patch.
The followings are my review comments and questions.

* pg_stat_vacuum_progress should have the oid of relation being vacuumed.

When we run "VACUUM;", the all tables of current database will be vacuumed.
So pg_stat_vacuum_progress should have these oid in order to show
which table is vacuumed now.

* progress_message variable in PgBackendStatus is not used at all.
IIRC, progress_message variable is set the description of processing.

* The progress of VACUUM FULL seems wrong.
When I run VACUUM FULL for a table, I got following progress.

postgres(1)=# select * from pg_stat_vacuum_progress ;
-[ RECORD 1 ]-------+------
pid | 19190
total_pages | 1
scanned_pages | 1
total_heap_pages | 1
scanned_heap_pages | 1
total_index_pages |
scanned_index_pages |
percent_complete | 100

The table being vacuumed is 400MB, so it's not 1 page table.

* The vacuum by autovacuum is not displayed.
I tested about this by the executing the following queries in a row,
but the vacuum by autovacuum is not displayed,

postgres(1)=# select datname, pid, backend_start, query, state from
pg_stat_activity ;
datname | pid | backend_start |
query | state
----------+-------+-------------------------------+--------------------------------------------------------------------------+--------
postgres | 20123 | 2015-09-24 17:44:26.467021+09 | autovacuum: VACUUM
ANALYZE public.hoge | active
postgres | 19779 | 2015-09-24 17:42:31.57918+09 | select datname,
pid, backend_start, query, state from pg_stat_activity ; | active
(3 rows)

postgres(1)=# selecT * from pg_stat_vacuum_progress ;
pid | total_pages | scanned_pages | total_heap_pages |
scanned_heap_pages | total_index_pages | scanned_index_pages |
percent_complete
-----+-------------+---------------+------------------+--------------------+-------------------+---------------------+------------------
(0 rows)

postgres(1)=# select datname, pid, backend_start, query, state from
pg_stat_activity ;
datname | pid | backend_start |
query | state
----------+-------+-------------------------------+--------------------------------------------------------------------------+--------
postgres | 20123 | 2015-09-24 17:44:26.467021+09 | autovacuum: VACUUM
ANALYZE public.hoge | active
postgres | 19779 | 2015-09-24 17:42:31.57918+09 | select datname,
pid, backend_start, query, state from pg_stat_activity ; | active
(3 rows)

The vacuuming for hoge table took about 2min, but the progress of
vacuum is never displayed.
Could you check this on your environment?

Regards,

--
Masahiko Sawada

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

On Wed, Sep 23, 2015 at 12:24 AM, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

Please find attached patch with bugs reported by Thom and Sawada-san solved.

The regression test failed on my machine, so you need to update the
regression test,
I think.

Regards,

--
Fujii Masao

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

On 9/24/15 7:37 AM, Masahiko Sawada wrote:

* The progress of VACUUM FULL seems wrong.
When I run VACUUM FULL for a table, I got following progress.

It never occurred to me that this patch was attempting to measure the
progress of a CLUSTER (aka VACUUM FULL). I'm not sure that's such a
great idea, as the progress estimation presumably needs to be
significantly different.

More to the point, you can't estimate a CLUSTER unless you can estimate
the progress of an index build. That'd be a cool feature to have as
well, but it seems like a bad idea to mix that in with this patch.

Keep in mind that running a VACUUM FULL is presumably a LOT less common
than regular vacuums, so I don't think leaving it out for now is that
big a deal.

* The vacuum by autovacuum is not displayed.
I tested about this by the executing the following queries in a row,
but the vacuum by autovacuum is not displayed,

IIRC this is the second problem related to autovacuum... is there some
way to regression test that? Maybe disable autovac on a table, dirty it,
then re-enable (all with an absurdly low autovacuum naptime)?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On Thu, Sep 24, 2015 at 8:37 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

When we run "VACUUM;", the all tables of current database will be vacuumed.
So pg_stat_vacuum_progress should have these oid in order to show
which table is vacuumed now.

Hmm, I would tend to instead show the schema & table name, like "foo"."bar".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Mon, Sep 28, 2015 at 11:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 24, 2015 at 8:37 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

When we run "VACUUM;", the all tables of current database will be vacuumed.
So pg_stat_vacuum_progress should have these oid in order to show
which table is vacuumed now.

Hmm, I would tend to instead show the schema & table name, like "foo"."bar".

Yes, it looks better.

Regards,

--
Masahiko Sawada

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

On Fri, Sep 25, 2015 at 2:03 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Sep 23, 2015 at 12:24 AM, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

Hello,

Please find attached patch with bugs reported by Thom and Sawada-san solved.

The regression test failed on my machine, so you need to update the
regression test,
I think.

Here are another review comments.

You removed some empty lines, for example, in vacuum.h.
Which seems useless to me.

+ uint32 progress_param[N_PROGRESS_PARAM];

Why did you use an array to store the progress information of VACUUM?
I think that it's better to use separate specific variables for them for
better code readability, for example, variables scanned_pages,
heap_total_pages, etc.

+ double progress_param_float[N_PROGRESS_PARAM];

Currently only progress_param_float[0] is used. So there is no need to
use an array here.

progress_param_float[0] saves the percetage of VACUUM progress.
But why do we need to save that information into shared memory?
We can just calculate the percentage whenever pg_stat_get_vacuum_progress()
is executed, instead. There seems to be no need to save that information.

+ char progress_message[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM];

As Sawada pointed out, there is no user of this variable.

+#define PG_STAT_GET_PROGRESS_COLS 30

Why did you use 30?

+ FROM pg_stat_get_vacuum_progress(NULL) AS S;

You defined pg_stat_get_vacuum_progress() so that it accepts one argument.
But as far as I read the function, it doesn't use any argument at all.
I think that pg_stat_get_vacuum_progress() should be defined as a function
having no argument.

+        /* Report values for only those backends which are running VACUUM */
+        if(!beentry || (strncmp(beentry->st_activity,"VACUUM",6)
+                        && strncmp(beentry->st_activity,"vacuum",6)))
+            continue;

This design looks bad to me. There is no guarantee that st_activity of
the backend running VACUUM displays "VACUUM" or "vacuum".
For example, st_activity of autovacuum worker displays "autovacuum ...".
So as Sawada reported, he could not find any entry for autovacuum in
pg_stat_vacuum_progress.

I think that you should add the flag or something which indicates
whether this backend is running VACUUM or not, into PgBackendStatus.
pg_stat_vacuum_progress should display the entries of only backends
with that flag set true. This design means that you need to set the flag
to true when starting VACUUM and reset at the end of VACUUM progressing.

Non-superuser cannot see some columns of the superuser's entry in
pg_stat_activity, for permission reason. We should treat
pg_stat_vacuum_progress in the same way? That is, non-superuser
should not be allowed to see the pg_stat_vacuum_progress entry
of superuser running VACUUM?

+                if(!scan_all)
+                {
+                    total_heap_pages = total_heap_pages -
(next_not_all_visible_block - blkno);
+                    total_pages = total_pages -
(next_not_all_visible_block - blkno);
+                }

This code may cause total_pages and total_heap_pages to decrease
while VACUUM is running. This sounds strange and confusing. I think
that total values basically should be fixed. And heap_scanned_pages
should increase, instead.

Regards,

--
Fujii Masao

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

On 2015/10/02 15:38, Fujii Masao wrote:

+ uint32 progress_param[N_PROGRESS_PARAM];

Why did you use an array to store the progress information of VACUUM?
I think that it's better to use separate specific variables for them for
better code readability, for example, variables scanned_pages,
heap_total_pages, etc.

+ double progress_param_float[N_PROGRESS_PARAM];

Currently only progress_param_float[0] is used. So there is no need to
use an array here.

I think this kind of design may have come from the ideas expressed here
(especially the last paragraph):

/messages/by-id/CA+TgmoYnWtNJRmVWAJ+wGLOB_x8vNOTrZnEDio=GaPi5HK73oQ@mail.gmail.com

Thanks,
Amit

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

On Fri, Oct 2, 2015 at 3:14 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2015/10/02 15:38, Fujii Masao wrote:

+ uint32 progress_param[N_PROGRESS_PARAM];

Why did you use an array to store the progress information of VACUUM?
I think that it's better to use separate specific variables for them for
better code readability, for example, variables scanned_pages,
heap_total_pages, etc.

+ double progress_param_float[N_PROGRESS_PARAM];

Currently only progress_param_float[0] is used. So there is no need to
use an array here.

I think this kind of design may have come from the ideas expressed here
(especially the last paragraph):

/messages/by-id/CA+TgmoYnWtNJRmVWAJ+wGLOB_x8vNOTrZnEDio=GaPi5HK73oQ@mail.gmail.com

Right. This design is obviously silly if we only care about exposing
VACUUM progress. But if we want to be able to expose progress from
many utility commands, and slightly different kinds of information for
each one, then I think it could be quite useful.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Hi!

On 2015-09-22 15:24:38 +0000, Syed, Rahila wrote:

Please find attached patch with bugs reported by Thom and Sawada-san solved.

This thread has seen a bunch of reviews and new patch versions, but
doesnt yet seem to have arrived in a committable state. As the
commitfest ended and this patch has gotten attention, I'm moving the
entry to the next fest.

Greetings,

Andres Freund

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

Hello Fujii-san,

Here are another review comments

Thank you for review. Please find attached an updated patch.

You removed some empty lines, for example, in vacuum.h.
Which seems useless to me.

Has been corrected in the attached.

Why did you use an array to store the progress information of VACUUM?
I think that it's better to use separate specific variables for them for better code readability, for example, variables scanned_pages, heap_total_pages, etc.

It is designed this way to keep it generic for all the commands which can store different progress parameters in shared memory.

Currently only progress_param_float[0] is used. So there is no need to use an array here.

Same as before . This is for later use by other commands.

progress_param_float[0] saves the percetage of VACUUM progress.
But why do we need to save that information into shared memory?
We can just calculate the percentage whenever pg_stat_get_vacuum_progress() is executed, instead. There seems to be no need to save that information.

This has been corrected in the attached.

char progress_message[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM];
As Sawada pointed out, there is no user of this variable.

Have used it to store table name in the updated patch. It can also be used to display index names, current phase of VACUUM.
This has not been included in the patch yet to avoid cluttering the display with too much information.

For example, st_activity of autovacuum worker displays "autovacuum ...".
So as Sawada reported, he could not find any entry for autovacuum in pg_stat_vacuum_progress.

In the attached patch , I have performed check for autovacuum also.

I think that you should add the flag or something which indicates whether this backend is running VACUUM or not, into PgBackendStatus.
pg_stat_vacuum_progress should display the entries of only backends with that flag set true. This design means that you need to set the flag to true when starting VACUUM and reset at the end of VACUUM progressing.

This design seems better in the sense that we don’t rely on st_activity entry to display progress values.
A variable which stores flags for running commands can be created in PgBackendStatus. These flags can be used at the time of display of progress of particular command.

That is, non-superuser should not be allowed to see the pg_stat_vacuum_progress entry of superuser running VACUUM?

This has been included in the updated patch.

This code may cause total_pages and total_heap_pages to decrease while VACUUM is running.

Yes. This is because the initial count of total pages to be vacuumed and the pages which are actually vacuumed can vary depending on visibility of tuples.
The pages which are all visible are skipped and hence have been subtracted from total page count.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

Hello,

Please check the attached patch as the earlier one had typo in regression test output.

+#define PG_STAT_GET_PROGRESS_COLS 30
Why did you use 30?

That has come from N_PROGRESS_PARAM * 3 where N_PROGRESS_PARAM = 10 is the number of progress parameters of each type stored in shared memory.
There are three such types (int, float, string) hence total number of progress parameters can be 30.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

Hello,

I think that you should add the flag or something which indicates whether this backend is running VACUUM or not, into PgBackendStatus.
pg_stat_vacuum_progress should display the entries of only backends with that flag set true. This design means that you need to set the flag to true when starting VACUUM and reset at the end of VACUUM progressing.

Please find attached updated patch which adds a flag in PgBackendStatus which indicates whether this backend in running VACUUM.
Also, pgstat_report_progress function is changed to make it generic for all commands reporting progress.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

On Tue, Oct 20, 2015 at 4:58 AM, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:

I think that you should add the flag or something which indicates whether this backend is running VACUUM or not, into PgBackendStatus.
pg_stat_vacuum_progress should display the entries of only backends with that flag set true. This design means that you need to set the flag to true when starting VACUUM and reset at the end of VACUUM progressing.

Please find attached updated patch which adds a flag in PgBackendStatus which indicates whether this backend in running VACUUM.

Flag isn't reset on error.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Syed, Rahila wrote:

@@ -355,6 +356,7 @@ vacuum(int options, RangeVar *relation, Oid relid, VacuumParams *params,
vac_update_datfrozenxid();
}

+ pgstat_reset_activityflag;
/*
* Clean up working storage --- note we must do this after
* StartTransactionCommand, else we might be trying to delete the active

Does this actually compile?

@@ -596,11 +630,42 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
/* Log cleanup info before we touch indexes */
vacuum_log_cleanup_info(onerel, vacrelstats);

+			/*
+			 * If passes through indexes exceed 1 add
+			 * pages equal to rel_index_pages to the count of
+			 * total pages to be scanned.
+			 */
+			if (vacrelstats->num_index_scans >= 1)
+			{
+				total_index_pages = total_index_pages + rel_index_pages;
+				total_pages = total_heap_pages + total_index_pages;
+			}

Having the keep total_pages updated each time you change one of the
summands seems tedious and error-prone. Why can't it be computed
whenever it is going to be used instead?

+				memcpy((char *) progress_message[0], schemaname, schemaname_len);
+				progress_message[0][schemaname_len] = '\0';
+				strcat(progress_message[0],".");
+				strcat(progress_message[0],relname);

snprintf()? I don't think you need to keep track of schemaname_len at
all.

+			scanned_index_pages += RelationGetNumberOfBlocks(Irel[i]);
+			scanned_total_pages = scanned_total_pages + RelationGetNumberOfBlocks(Irel[i]);
+			/* Report progress to the statistics collector */
+			progress_param[0] = total_pages;
+			progress_param[1] = scanned_total_pages;
+			progress_param[2] = total_heap_pages;
+			progress_param[3] = vacrelstats->scanned_pages;
+			progress_param[4] = total_index_pages;
+			progress_param[5] = scanned_index_pages;

In fact, I wonder if you need to send total_pages at all -- surely the
client can add both total_heap_pages and total_index_pages by itself ...

+			memcpy((char *) progress_message[0], schemaname, schemaname_len);
+			progress_message[0][schemaname_len] = '\0';
+			strcat(progress_message[0],".");
+			strcat(progress_message[0],relname);

snprintf().

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index ab018c4..f97759e 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2851,6 +2851,55 @@ 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(uint *param1, int num_of_int, double *param2, int num_of_float,
+						char param3[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM],
+						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->progress_param[i] = param1[i];
+	}
+	for (i = 0; i < num_of_float; i++)
+	{
+		beentry->progress_param_float[i] = param2[i];
+	}
+	for (i = 0; i < num_of_string; i++)
+	{
+		strcpy((char *)beentry->progress_message[i], param3[i]);
+	}
+	pgstat_increment_changecount_after(beentry);
+}

It seems a bit strange that the remaining progress_param entries are not
initialized to anything. Also, why aren't the number of params of each
type saved too? In the receiving code you check whether each value
equals 0, and if it does then report NULL, but imagine vacuuming a table
with no indexes where the number of index pages is going to be zero.
Shouldn't we display zero there rather than null? Maybe I'm missing
something and that does work fine.

This patch lacks a comment somewhere explaining how this whole thing
works.

diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 9ecc163..4214b3d 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -20,6 +20,7 @@
#include "utils/hsearch.h"
#include "utils/relcache.h"

+#include "storage/block.h"

I believe you don't need this include.

@@ -776,6 +779,12 @@ typedef struct PgBackendStatus

/* current command string; MUST be null-terminated */
char	   *st_activity;
+
+	uint32		flag_activity;
+	uint32		progress_param[N_PROGRESS_PARAM];
+	double		progress_param_float[N_PROGRESS_PARAM];
+	char		progress_message[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM];
+
} PgBackendStatus;

This not only adds an unnecessary empty line at the end of the struct
declaration, but also fails to preserve the "st_" prefix used in all the
other fields.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Hello,

Please find attached an updated patch.

Flag isn't reset on error.

Corrected in the attached.

+ pgstat_reset_activityflag;
Does this actually compile?

It does compile but with no effect. It has been corrected.

snprintf()? I don't think you need to keep track of schemaname_len at all.

memcpy() has been replaced by snprintf() to avoid calculating schemaname_len.

In fact, I wonder if you need to send total_pages at all -- surely the client can add both total_heap_pages and total_index_pages by itself ...

This has been corrected in the attached patch.

It seems a bit strange that the remaining progress_param entries are not initialized to anything. Also, why aren't the number of params of each type saved too?

The number of params for each command remains constant hence it has been hardcoded.

In the receiving code you check whether each value equals 0, and if it does then report NULL, but imagine vacuuming a table with no indexes where the number of index pages is going to be zero.
Shouldn't we display zero there rather than null?

Agree. IIUC, NULL should rather be used when a value is invalid. But for valid values like 'zero index pages' it is clearer to display 0. It has been corrected in the attached.

This patch lacks a comment somewhere explaining how this whole thing works.

Have added few lines in pgstat.h inside PgBackendStatus struct.

I believe you don't need this include.

Corrected.

This not only adds an unnecessary empty line at the end of the struct declaration, but also fails to preserve the "st_" prefix used in all the other fields

Corrected.

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

On 2015/10/29 23:22, Syed, Rahila wrote:

Please find attached an updated patch.

Thanks for the v6. A few quick comments:

- duplicate_oids error in HEAD.

- a compiler warning:

pgstat.c:2898: warning: no previous prototype for �pgstat_reset_activityflag�

To fix that use void for empty parameter list -

-extern void pgstat_reset_activityflag();
+extern void pgstat_reset_activityflag(void);

One more change you could do is 's/activityflag/activity_flag/g', which I
guess is a naming related guideline in place.

Thanks,
Amit

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

Hello, I have some random comments on this patch addition to
Amit's comments.

- Type of the flag of vacuum activity.

ACTIVITY_IS_VACUUM is the alone entry in the enum, and the
variable to store it is named as *flag. If you don't have any
plan to extend this information, the name of this variable would
seems better to be something like pgstat_report_vacuum_running
and in the type of boolean.

- Type of st_progress_param and so.

The variable st_progress_param has very generic name but as
looking the pg_stat_get_vacuum_progress, every elements of it is
in a definite role. If so, the variable should be a struct.

st_progress_param_float is currently totally useless.

- 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. The following snprintf,

| snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", schemaname);

certainly destroys the data already stored in it if any.

- snprintf()

You are so carefully to use snprintf,

+    snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", schemaname);
+	strcat(progress_message[0],".");
+	strcat(progress_message[0],relname);

but the strcats following ruin it.

- Calculation of total_heap_pages in lazy_scan_heap.

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.

I'll be happy if this can be of any help.

regards,

At Tue, 10 Nov 2015 14:44:23 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <56418437.5080003@lab.ntt.co.jp>

Thanks for the v6. A few quick comments:

- duplicate_oids error in HEAD.

- a compiler warning:

pgstat.c:2898: warning: no previous prototype for ‘pgstat_reset_activityflag’

To fix that use void for empty parameter list -

-extern void pgstat_reset_activityflag();
+extern void pgstat_reset_activityflag(void);

One more change you could do is 's/activityflag/activity_flag/g', which I
guess is a naming related guideline in place.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

On 2015/10/29 23:22, Syed, Rahila wrote:

Please find attached an updated patch.

A few more comments on v6:

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", schemaname);
+	strcat(progress_message[0],".");
+	strcat(progress_message[0],relname);

How about the following instead -

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s",
+ 					generate_relation_name(onerel));

if (next_not_all_visible_block >= SKIP_PAGES_THRESHOLD)
+	{
skipping_all_visible_blocks = true;
+		if(!scan_all)
+			total_heap_pages = total_heap_pages - next_not_all_visible_block;
+	}
else
skipping_all_visible_blocks = false;

...

*/
if (next_not_all_visible_block - blkno > SKIP_PAGES_THRESHOLD)
+			{
skipping_all_visible_blocks = true;
+				if(!scan_all)
+					total_heap_pages = total_heap_pages - (next_not_all_visible_block - blkno);
+			}

Fujii-san's review comment about these code blocks does not seem to be
addressed. He suggested to keep total_heap_pages fixed while adding number
of skipped pages to that of scanned pages. For that, why not add a
scanned_heap_pages variable instead of using vacrelstats->scanned_pages.

+		if (has_privs_of_role(GetUserId(), beentry->st_userid))
+		{
+			values[2] = UInt32GetDatum(beentry->st_progress_param[0]);
+			values[3] = UInt32GetDatum(beentry->st_progress_param[1]);
+			values[4] = UInt32GetDatum(beentry->st_progress_param[2]);
+			values[5] = UInt32GetDatum(beentry->st_progress_param[3]);
+			values[6] = UInt32GetDatum(total_pages);
+			values[7] = UInt32GetDatum(scanned_pages);
+
+			if (total_pages != 0)
+				values[8] = Float8GetDatum(scanned_pages * 100 / total_pages);
+			else
+				nulls[8] = true;
+		}
+		else
+		{
+			values[2] = CStringGetTextDatum("<insufficient privilege>");
+			nulls[3] = true;
+			nulls[4] = true;
+			nulls[5] = true;
+			nulls[6] = true;
+			nulls[7] = true;
+			nulls[8] = true;
+		}

This is most likely not correct, that is, putting a text datum into
supposedly int4 column. I see this when I switch to a unprivileged user:

pgbench=# \x
pgbench=# \c - other
pgbench=> SELECT * FROM pg_stat_vacuum_progress;
-[ RECORD 1 ]-------+------------------------
pid | 20395
table_name | public.pgbench_accounts
total_heap_pages | 44895488
scanned_heap_pages |
total_index_pages |
scanned_index_pages |
total_pages |
scanned_pages |
percent_complete |

I'm not sure if applying the privilege check for columns of
pg_stat_vacuum_progress is necessary, but I may be wrong.

Thanks,
Amit

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

On Tue, Nov 10, 2015 at 5:02 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2015/10/29 23:22, Syed, Rahila wrote:
How about the following instead -

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s",
+                                       generate_relation_name(onerel));

That was a useless suggestion, sorry. It still could be rewritten as -

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s",
+                          get_namespace_name(RelationGetNamespace(rel)),
+                          relname);

Thanks,
Amit

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

On 2015/11/10 17:02, Amit Langote wrote:

On 2015/10/29 23:22, Syed, Rahila wrote:

Please find attached an updated patch.

A few more comments on v6:

I backed up a little, studied the proposal and the patch in little some
more detail. Here are still more comments -

Going through the thread, it seems there are following problems being solved -

1) General purpose interface for (maintenance?) commands to report a set
of internal values of different internal types using shared memory as IPC;
values are exposed to users as is and/or some derived values like
percent_done using view/functions

2) For a start, instrumenting lazy vacuum to report such internal values

And maybe,

3) Estimating the amount of work to be done and time required based on
historical statistics like n_dead_tup, visibility map and run-time
resources available like maintenance_work_mem

Latest version of the patch (v6) implements 1 and 2. The code is starting
to look good though see some comments below.

* Regarding (2): Some random thoughts on the patch and in general -

For lazy vacuum, lazy_scan_heap() seems like the best place which can
provide granular progress report in terms of the heap block number (of
total number of heap blocks in the relation) currently undergoing
per-block pass 1 processing. About pass 2, ie, lazy_index_vacuum() and
lazy_vacuum_heap(), I don't see how we can do better than reporting its
progress only after finishing all of it without any finer-grained
instrumentation. They are essentially block-box as far as the proposed
instrumentation approach is concerned. Being able to report progress per
index seems good but as a whole, a user would have to wait arbitrarily
long before numbers move forward. We might as well just report a bool
saying we're about to enter a potentially time-consuming index vacuum
round with possibly multiple indexes followed by lazy_vacuum_heap()
processing. Additionally, we can report the incremented count of the
vacuuming round (pass 2) once we are through it. So, we'd report two
values viz. waiting_vacuum_pass (bool) and num_vacuum_pass (int). The
former is reported twice - 'true' as we are about to begin the round and
'false' once done. We can keep the total_index_pages (counting all
indexes) and index_pages_done as the patch currently reports. The latter
moves forward for every index we finish processing, and also should be
reset for every pass 2 round. Note that we can leave them out of
percent_done of overall vacuum progress. Until we have a good solution for
number (3) above, it seems to difficult to incorporate index pages into
overall progress.

As someone pointed out upthread, the final heap truncate phase can take
arbitrarily long and is outside the scope of lazy_scan_heap() to
instrument. Perhaps a bool, say, waiting_heap_trunc could be reported for
the same. Note that, it would have to be reported from lazy_vacuum_rel().

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

* Regarding (1): These are mostly code comments -

IMHO, float progress parameters (st_progress_param_float[]) can be taken
out. They are currently unused and it's unlikely that some command would
want to report them. OTOH, as suggested in above paragraph, why not have
bool parameters? In addition to a few I mentioned in the context of lazy
vacuum instrumentation, it seems likely that they would be useful for
other commands, too.

Instead of st_activity_flag, how about st_command and calling
ACTIVITY_IS_VACUUM, say, COMMAND_LAZY_VACUUM?
pgstat_report_activity_flag() then would become pgstat_report_command().

Like floats, I would think we could take out st_progress_message[][]. I
see that it is currently used to report table name. For that, we might as
well add a single st_command_target[NAMEDATALEN] string which is set at
the beginning of command processing using, say,
pgstat_report_command_target(). It stores the name of relation/object that
the command is going to work on.

Maybe, we don't need each command to proactively pgstat_reset_command().
That would be similar to how st_activity is not proactively cleared but is
rather reset by the next query/command or when some other backend uses the
shared memory slot. Also, we could have a SQL function
pg_stat_reset_local_progress() which clears the st_command after which the
backend is no longer shown in the progress view.

I think it would be better to report only changed parameter arrays when
performing pgstat_report_progress(). So, if we have following shared
memory progress parameters and the reporting function signature:

typedef struct PgBackendStatus
{
...
uint16 st_command;
char st_command[NAMEDATALEN];
uint32 st_progress_uint32_param[N_PROGRESS_PARAM];
bool st_progress_bool_param[N_PROGRESS_PARAM];
} PgBackendStatus;

void pgstat_report_progress(uint32 *uint32_param, int num_uint32_param,
bool *bool_param, int num_bool_param);

and if we need to report a bool parameter change, say, waiting_vacuum_pass
in lazy_scan_heap(), we do -

pgstat_report_progress(NULL, 0, progress_bool_param, 2);

That is, no need for pgstat_report_progress() to overwrite the shared
st_progress_uint32_param if none of its members have changed since the
last report.

Currently, ACTIVITY_IS_VACUUM is reported even for VACOPT_ANALYZE and
VACOPT_FULL commands. They are not covered by lazy_scan_heap(), so such
commands are needlessly shown in the progress view with 0s in most of the
fields.

Regarding pg_stat_get_vacuum_progress(): I think a backend can simply be
skipped if (!has_privs_of_role(GetUserId(), beentry->st_userid)) (cannot
put that in plain English, :))

Please add documentation for the newly added view and SQL functions, if any.

I'm marking this as "Waiting on author" in the commitfest app. Also, let's
hear from more people.

Thanks,
Amit

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

On 2015/11/19 16:18, Amit Langote wrote:

I'm marking this as "Waiting on author" in the commitfest app. Also, let's
hear from more people.

Well, it seems Michael beat me to it.

Thanks,
Amit

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

On Thu, Nov 19, 2015 at 4:30 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2015/11/19 16:18, Amit Langote wrote:

I'm marking this as "Waiting on author" in the commitfest app. Also, let's
hear from more people.

Well, it seems Michael beat me to it.

Yeah, some other folks provided feedback that's why I did it.
@Rahila: are you planning more work on this patch? It seems that at
this point we're waiting for you. If not, and because I have a couple
of times waited for long VACUUM jobs to finish on some relations
without having much information about their progress, I would be fine
to spend time hacking this thing. That's up to you.
Regards,
--
Michael

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

Hello Michael,

I am planning to continue contributing to this feature in any way be it by
reviewing the patch or making one. Though I haven't been able to reply to
the comments or post an updated patch lately. I plan to do that soon.

Thank you,
Rahila

On Thu, Nov 19, 2015 at 1:09 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On 11/19/15 1:18 AM, Amit Langote wrote:

1) General purpose interface for (maintenance?) commands to report a set

I'm surprised no one has picked up on using this for DML. Certainly
anyone works with ETL processes would love to be able to get some clue
on the status of a long running query...

About pass 2, ie, lazy_index_vacuum() and
lazy_vacuum_heap(), I don't see how we can do better than reporting its
progress only after finishing all of it without any finer-grained
instrumentation. They are essentially block-box as far as the proposed
instrumentation approach is concerned. Being able to report progress per
index seems good but as a whole, a user would have to wait arbitrarily
long before numbers move forward. We might as well just report a bool
saying we're about to enter a potentially time-consuming index vacuum
round with possibly multiple indexes followed by lazy_vacuum_heap()
processing. Additionally, we can report the incremented count of the
vacuuming round (pass 2) once we are through it.

Another option is to provide the means for the index scan routines to
report their progress. Maybe every index AM won't use it, but it'd
certainly be a lot better than staring at a long_running boolean.

Note that we can leave them out of
percent_done of overall vacuum progress. Until we have a good solution for
number (3) above, it seems to difficult to incorporate index pages into
overall progress.

IMHO we need to either put a big caution sign on any % estimate that it
could be wildly off, or just forgo it completely for now. I'll bet that
if we don't provide it some enterprising users will figure out the best
way to do this (similar to how the bloat estimate query has evolved over
time).

Even if we never get a % done indicator, just being able to see what
'position' a command is at will be very valuable.

As someone pointed out upthread, the final heap truncate phase can take
arbitrarily long and is outside the scope of lazy_scan_heap() to
instrument. Perhaps a bool, say, waiting_heap_trunc could be reported for
the same. Note that, it would have to be reported from lazy_vacuum_rel().

ISTM this is similar to the problem of reporting index status, namely
that a progress reporting method needs to accept reports from multiple
places in the code.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On 2015/11/20 0:57, Jim Nasby wrote:

On 11/19/15 1:18 AM, Amit Langote wrote:

1) General purpose interface for (maintenance?) commands to report a set

I'm surprised no one has picked up on using this for DML. Certainly anyone
works with ETL processes would love to be able to get some clue on the
status of a long running query...

Instrumenting query execution for progress info would be a complex beast
though. Also, what kind of reporting interface it would require is also
not clear, at least to me. Jan Urbanski's PGCon presentation[1]http://www.pgcon.org/2013/schedule/events/576.en.html is a good
source on the matter I discovered in this thread, thanks! But IMHO, for
now, it would be worthwhile to focus our resources on the modest goal of
implementing a reporting interface for utility commands. Sure it would be
nice to investigate how much the requirements of the two overlap.

About pass 2, ie, lazy_index_vacuum() and
lazy_vacuum_heap(), I don't see how we can do better than reporting its
progress only after finishing all of it without any finer-grained
instrumentation. They are essentially block-box as far as the proposed
instrumentation approach is concerned. Being able to report progress per
index seems good but as a whole, a user would have to wait arbitrarily
long before numbers move forward. We might as well just report a bool
saying we're about to enter a potentially time-consuming index vacuum
round with possibly multiple indexes followed by lazy_vacuum_heap()
processing. Additionally, we can report the incremented count of the
vacuuming round (pass 2) once we are through it.

Another option is to provide the means for the index scan routines to
report their progress. Maybe every index AM won't use it, but it'd
certainly be a lot better than staring at a long_running boolean.

The boolean would be a workaround for sure. I'm also slightly tempted by
the idea of instrumenting vacuum scans of individual index AM's bulkdelete
methods. One precedent is how vacuum_delay_point() are sprinkled around in
the code. Another problem to solve would be to figure out how to pass
progress parameters around - via some struct or could they be globals just
like VacuumCost* variables are...

Note that we can leave them out of
percent_done of overall vacuum progress. Until we have a good solution for
number (3) above, it seems to difficult to incorporate index pages into
overall progress.

IMHO we need to either put a big caution sign on any % estimate that it
could be wildly off, or just forgo it completely for now. I'll bet that if
we don't provide it some enterprising users will figure out the best way
to do this (similar to how the bloat estimate query has evolved over time).

Even if we never get a % done indicator, just being able to see what
'position' a command is at will be very valuable.

Agreed. If we provide enough information in whatever view we choose to
expose, that would be a good start.

As someone pointed out upthread, the final heap truncate phase can take
arbitrarily long and is outside the scope of lazy_scan_heap() to
instrument. Perhaps a bool, say, waiting_heap_trunc could be reported for
the same. Note that, it would have to be reported from lazy_vacuum_rel().

ISTM this is similar to the problem of reporting index status, namely that
a progress reporting method needs to accept reports from multiple places
in the code.

Yes.

Thanks,
Amit

[1]: http://www.pgcon.org/2013/schedule/events/576.en.html

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

On Thu, Nov 19, 2015 at 2:18 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

As someone pointed out upthread, the final heap truncate phase can take
arbitrarily long and is outside the scope of lazy_scan_heap() to
instrument. Perhaps a bool, say, waiting_heap_trunc could be reported for
the same. Note that, it would have to be reported from lazy_vacuum_rel().

I don't think reporting booleans is a very good idea. 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.

IMHO, float progress parameters (st_progress_param_float[]) can be taken
out. They are currently unused and it's unlikely that some command would
want to report them.

If they are not used, they shouldn't be included in this patch, but we
should be open to adding them later if it proves useful.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 11/19/15 7:29 PM, Amit Langote wrote:

Another option is to provide the means for the index scan routines to

report their progress. Maybe every index AM won't use it, but it'd
certainly be a lot better than staring at a long_running boolean.

The boolean would be a workaround for sure. I'm also slightly tempted by
the idea of instrumenting vacuum scans of individual index AM's bulkdelete
methods. One precedent is how vacuum_delay_point() are sprinkled around in
the code. Another problem to solve would be to figure out how to pass
progress parameters around - via some struct or could they be globals just
like VacuumCost* variables are...

It just occurred to me that we could do the instrumentation in
lazy_tid_reaped(). It might seem bad to do in increment for every tuple
in an index, but we're already doing a bsearch over the dead tuple list.
Presumably that's going to be a lot more expensive than an increment
operation.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On Sat, Nov 21, 2015 at 12:38 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

On 11/19/15 7:29 PM, Amit Langote wrote:

Another option is to provide the means for the index scan routines to

report their progress. Maybe every index AM won't use it, but it'd
certainly be a lot better than staring at a long_running boolean.

The boolean would be a workaround for sure. I'm also slightly tempted by
the idea of instrumenting vacuum scans of individual index AM's bulkdelete
methods. One precedent is how vacuum_delay_point() are sprinkled around in
the code. Another problem to solve would be to figure out how to pass
progress parameters around - via some struct or could they be globals just
like VacuumCost* variables are...

It just occurred to me that we could do the instrumentation in
lazy_tid_reaped(). It might seem bad to do in increment for every tuple in
an index, but we're already doing a bsearch over the dead tuple list.
Presumably that's going to be a lot more expensive than an increment
operation.

I think the cost of doing an increment there would be negligible. I'm
not quite sure whether that's the right place to instrument - though
it looks like it might be - but I think the cost of ++something in
that function isn't gonna be a problem at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2015/11/21 14:38, Jim Nasby wrote:

On 11/19/15 7:29 PM, Amit Langote wrote:

Another option is to provide the means for the index scan routines to

report their progress. Maybe every index AM won't use it, but it'd
certainly be a lot better than staring at a long_running boolean.

The boolean would be a workaround for sure. I'm also slightly tempted by
the idea of instrumenting vacuum scans of individual index AM's bulkdelete
methods. One precedent is how vacuum_delay_point() are sprinkled around in
the code. Another problem to solve would be to figure out how to pass
progress parameters around - via some struct or could they be globals just
like VacuumCost* variables are...

It just occurred to me that we could do the instrumentation in
lazy_tid_reaped(). It might seem bad to do in increment for every tuple in
an index, but we're already doing a bsearch over the dead tuple list.
Presumably that's going to be a lot more expensive than an increment
operation.

Just to clarify, does this mean we report index vacuum progress in terms
of index items processed (not pages)? If so, how do we get total number of
index items to process (presumably across all indexes) for a given phase 2
round? As a context, we'd report phase 1 progress in terms of heap pages
processed of total heap pages.

Thanks,
Amit

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

On 2015/11/21 5:46, Robert Haas wrote:

On Thu, Nov 19, 2015 at 2:18 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

As someone pointed out upthread, the final heap truncate phase can take
arbitrarily long and is outside the scope of lazy_scan_heap() to
instrument. Perhaps a bool, say, waiting_heap_trunc could be reported for
the same. Note that, it would have to be reported from lazy_vacuum_rel().

I don't think reporting booleans is a very good idea. 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.

Yeah, that might be better. One possible downside of booleans I didn't
foresee is that too many of them might clutter the progress view. What
would've been the names of boolean columns in the progress view are better
reported as strings as the value of a single column, as you seem to suggest.

IMHO, float progress parameters (st_progress_param_float[]) can be taken
out. They are currently unused and it's unlikely that some command would
want to report them.

If they are not used, they shouldn't be included in this patch, but we
should be open to adding them later if it proves useful.

Certainly.

Thanks,
Amit

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

On 11/24/15 2:02 AM, Amit Langote wrote:

It just occurred to me that we could do the instrumentation in

lazy_tid_reaped(). It might seem bad to do in increment for every tuple in
an index, but we're already doing a bsearch over the dead tuple list.
Presumably that's going to be a lot more expensive than an increment
operation.

Just to clarify, does this mean we report index vacuum progress in terms
of index items processed (not pages)? If so, how do we get total number of
index items to process (presumably across all indexes) for a given phase 2
round? As a context, we'd report phase 1 progress in terms of heap pages
processed of total heap pages.

You'd get it from pg_class.reltuples for each index. Since all index
vacuuming is done strictly on a per-index-tuple basis, that's probably
the most accurate way to do it anyway.

Also, while it might be interesting to look at the total number of index
tuples, I think it's probably best to always report on a per-index
basis, as well as which index is being processed. I suspect there could
be a very large variance of tuple processing speed for different index
types. Eventually it might be worth it to allow index AMs to provide
their own vacuuming feedback, but I think that's way out of scope for
this patch. :)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

On 2015/11/25 9:32, Jim Nasby wrote:

On 11/24/15 2:02 AM, Amit Langote wrote:

Just to clarify, does this mean we report index vacuum progress in terms
of index items processed (not pages)? If so, how do we get total number of
index items to process (presumably across all indexes) for a given phase 2
round? As a context, we'd report phase 1 progress in terms of heap pages
processed of total heap pages.

You'd get it from pg_class.reltuples for each index. Since all index
vacuuming is done strictly on a per-index-tuple basis, that's probably the
most accurate way to do it anyway.

Important to remember though that the reltuples would be latest as of the
last VACUUM/ANALYZE.

Also, while it might be interesting to look at the total number of index
tuples, I think it's probably best to always report on a per-index basis,
as well as which index is being processed. I suspect there could be a very
large variance of tuple processing speed for different index types.
Eventually it might be worth it to allow index AMs to provide their own
vacuuming feedback, but I think that's way out of scope for this patch. :)

Agreed.

Thanks,
Amit

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

On 11/24/15 7:02 PM, Amit Langote wrote:

You'd get it from pg_class.reltuples for each index. Since all index

vacuuming is done strictly on a per-index-tuple basis, that's probably the
most accurate way to do it anyway.

Important to remember though that the reltuples would be latest as of the
last VACUUM/ANALYZE.

True, but in cases where you care about monitoring a vacuum I suspect
it'll be close enough.

Might be worth a little extra effort to handle the 0 case though. If you
really wanted to get fancy you could see how the current heap
tuples/page count compares to reltuples/relpages from pg_class for the
heap... but I suspect that's pretty serious overkill.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

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

Thanks for the v7.
Please check the comment below.
-Table name in the vacuum progress

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s",
schemaname,relname);

In the vacuum progress, column table_name is showing first 30 characters of
table name.
postgres=# create table test_vacuum_progress_in_postgresql(c1 int,c2 text);
postgres=# select * from pg_stat_vacuum_progress ;
-[ RECORD 1 ]-------+------------------------------
pid | 12284
table_name | public.test_vacuum_progress_i
phase | Scanning Heap
total_heap_pages | 41667
scanned_heap_pages | 25185
percent_complete | 60
total_index_pages | 0
scanned_index_pages | 0
index_scan_count | 0

-----
Regards,
Vinayak,

--
View this message in context: http://postgresql.nabble.com/PROPOSAL-VACUUM-Progress-Checker-tp5855849p5875614.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

Hello,

At Mon, 30 Nov 2015 19:10:44 -0700 (MST), Vinayak <vinpokale@gmail.com> wrote in <1448935844520-5875614.post@n5.nabble.com>

Thanks for the v7.
Please check the comment below.
-Table name in the vacuum progress

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s",
schemaname,relname);

In the vacuum progress, column table_name is showing first 30 characters of
table name.

Yeah, it is actually restricted in that length. But if we allow
the buffer to store whole the qualified names, it will need 64 *
2 + 1 +1 = 130 bytes * 10 1300 bytes for each beentry... It might
be acceptable by others, but I don't think that is preferable..

Separating namespace and relation name as below reduces the
required length of the field but 62 bytes is still too long for
most of the information and in turn too short for longer messages
in some cases.

As a more dractic change in design, since these fields are
written/read in sequential manner, providing one free buffer of
the size of.. so.. about 128 bytes for each beentry and storing
strings delimiting with '\0' and numbers in binary format, as an
example, would do. Additional functions to write into/read from
this buffer safely would be needed but this gives both the
ability to store longer messages and relatively short total
buffer size, and allows arbitrary number of parameters limited
only by the length of the free buffer.

What do you think about this?

By the way, how about giving separate columns for relname and
namespace? I think it is more usual way to designate a relation
in this kind of view and it makes the snprintf to concatenate
name and schema unnecessary(it's not significant, though). (The
following example is after pg_stat_all_tables)

postgres=# create table test_vacuum_progress_in_postgresql(c1 int,c2 text);
postgres=# select * from pg_stat_vacuum_progress ;
pid | 12284
schemaname | public
relname | test_vacuum_progress_i...
phase | Scanning Heap
total_heap_pages | 41667

...

And I have some comments about code.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

Sorry for the confusing description and the chopped sentsnce.

At Tue, 01 Dec 2015 16:25:57 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20151201.162557.184519961.horiguchi.kyotaro@lab.ntt.co.jp>

Hello,

At Mon, 30 Nov 2015 19:10:44 -0700 (MST), Vinayak <vinpokale@gmail.com> wrote in <1448935844520-5875614.post@n5.nabble.com>

Thanks for the v7.
Please check the comment below.
-Table name in the vacuum progress

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s",
schemaname,relname);

In the vacuum progress, column table_name is showing first 30 characters of
table name.

Yeah, it is actually restricted in that length. But if we allow
the buffer to store whole the qualified names, it will need 64 *
2 + 1 +1 = 130 bytes * 10 1300 bytes for each beentry... It might
be acceptable by others, but I don't think that is preferable..

Separating namespace and relation name as below reduces the
required length of the field but 62 bytes is still too long for
most of the information and in turn too short for longer messages
in some cases.

As a more dractic change in design, since these fields are
written/read in sequential manner, providing one free buffer of
the size of.. so.. about 128 bytes for each beentry and storing
strings delimiting with '\0' and numbers in binary format, as an
example, would do.

This would fail to make sense.. I suppose this can be called
'packed format', as opposed to fixed-length format. Sorry for
poor wording.

Additional functions to write into/read from
this buffer safely would be needed but this gives both the
ability to store longer messages and relatively short total
buffer size, and allows arbitrary number of parameters limited
only by the length of the free buffer.

What do you think about this?

By the way, how about giving separate columns for relname and
namespace? I think it is more usual way to designate a relation
in this kind of view and it makes the snprintf to concatenate
name and schema unnecessary(it's not significant, though). (The
following example is after pg_stat_all_tables)

postgres=# create table test_vacuum_progress_in_postgresql(c1 int,c2 text);
postgres=# select * from pg_stat_vacuum_progress ;
pid | 12284
schemaname | public
relname | test_vacuum_progress_i...
phase | Scanning Heap
total_heap_pages | 41667

...

And I have some comments about code.

This is just what I forgot to delete. I'll mention them later if
necessary.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

On 2015/12/01 16:25, Kyotaro HORIGUCHI wrote:

At Mon, 30 Nov 2015 19:10:44 -0700 (MST), Vinayak <vinpokale@gmail.com> wrote

Thanks for the v7.
Please check the comment below.
-Table name in the vacuum progress

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s",
schemaname,relname);

In the vacuum progress, column table_name is showing first 30 characters of
table name.

Yeah, it is actually restricted in that length. But if we allow
the buffer to store whole the qualified names, it will need 64 *
2 + 1 +1 = 130 bytes * 10 1300 bytes for each beentry... It might
be acceptable by others, but I don't think that is preferable..

Separating namespace and relation name as below reduces the
required length of the field but 62 bytes is still too long for
most of the information and in turn too short for longer messages
in some cases.

As done in the patch, the table name is stored in one of the slots of
st_progress_message which has the width limit of PROGRESS_MESSAGE_LENGTH
bytes. Whereas users of pgstat_report_progress interface could make sure
that strings of their choosing to be stored in st_progress_param slots are
within the PROGRESS_MESSAGE_LENGTH limit, the same cannot be ensured for
the table name. Maybe, the table name is a different kind of information
than other reported parameters that it could be treated specially. How
about a separate st_* member, say, st_command_target[2*NAMDATALEN+1] for
the table name? It would be reported using a separate interface, say,
pgstat_report_command_target() once the name is determined. Moreover,
subsequent pgstat_report_progress() invocations need not copy the table
name needlessly as part of copying argument values to st_progress_param
(which is a separate suggestion in its own right though).

Thanks,
Amit

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

On Mon, Nov 30, 2015 at 9:10 PM, Vinayak <vinpokale@gmail.com> wrote:

Thanks for the v7.
Please check the comment below.
-Table name in the vacuum progress

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s.%s",
schemaname,relname);

Uh, I hope that line doesn't appear in the patch. We're scarcely
likely to commit anything that has such an obvious SQL-injection risk
built into it.

https://xkcd.com/327/

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Tue, Dec 1, 2015 at 2:25 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Yeah, it is actually restricted in that length. But if we allow
the buffer to store whole the qualified names, it will need 64 *
2 + 1 +1 = 130 bytes * 10 1300 bytes for each beentry... It might
be acceptable by others, but I don't think that is preferable..

There's no such thing as a free lunch here, but we probably don't need
room for 10 strings. If we allowed say 4 strings per beentry and
limited each one to, say, 140 characters for Twitter-compatibility,
that's 560 bytes per backend. Throw in some int8 counters and you're
up to maybe 600 bytes per backend. So that's ~60kB of memory for 100
backends. That doesn't sound like a huge problem to me, but it
wouldn't be stupid to have a PGC_POSTMASTER GUC to turn this feature
on and off, for the benefit of people who may want to run this in
low-memory environments.

As a more dractic change in design, since these fields are
written/read in sequential manner, providing one free buffer of
the size of.. so.. about 128 bytes for each beentry and storing
strings delimiting with '\0' and numbers in binary format, as an
example, would do. Additional functions to write into/read from
this buffer safely would be needed but this gives both the
ability to store longer messages and relatively short total
buffer size, and allows arbitrary number of parameters limited
only by the length of the free buffer.

What do you think about this?

I think it sounds like a mess with uncertain benefits. Now instead of
having individual fields that maybe don't fit and have to be
truncated, you have to figure out what to leave out when the overall
message doesn't fit. That's likely to lead to a lot of messy logic on
the server side, and even messier logic for any clients that read the
data and try to parse it programmatically.

By the way, how about giving separate columns for relname and
namespace? I think it is more usual way to designate a relation
in this kind of view and it makes the snprintf to concatenate
name and schema unnecessary(it's not significant, though). (The
following example is after pg_stat_all_tables)

I could go either way on this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Vinayak wrote:

In the vacuum progress, column table_name is showing first 30 characters of
table name.
postgres=# create table test_vacuum_progress_in_postgresql(c1 int,c2 text);
postgres=# select * from pg_stat_vacuum_progress ;
-[ RECORD 1 ]-------+------------------------------
pid | 12284
table_name | public.test_vacuum_progress_i

Actually, do we really need to have the table name as a string at all
here? Why not just report the table OID? Surely whoever wants to check
the progress can connect to the database in question to figure out the
table name.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Hello, sorry for the cloberred CC list.

# I restored it manually from upthread..

At Wed, 2 Dec 2015 13:42:01 -0500, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobcN=3qa9X7c8_G18x53HDCpEYbWP4tnR_es5d=tYvrkQ@mail.gmail.com>

On Tue, Dec 1, 2015 at 2:25 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Yeah, it is actually restricted in that length. But if we allow
the buffer to store whole the qualified names, it will need 64 *
2 + 1 +1 = 130 bytes * 10 1300 bytes for each beentry... It might
be acceptable by others, but I don't think that is preferable..

There's no such thing as a free lunch here, but we probably don't need
room for 10 strings. If we allowed say 4 strings per beentry and
limited each one to, say, 140 characters for Twitter-compatibility,
that's 560 bytes per backend. Throw in some int8 counters and you're
up to maybe 600 bytes per backend. So that's ~60kB of memory for 100
backends. That doesn't sound like a huge problem to me, but it
wouldn't be stupid to have a PGC_POSTMASTER GUC to turn this feature
on and off, for the benefit of people who may want to run this in
low-memory environments.

This is similar to Amit-L's proposal and either sound fair for me.

As a more dractic change in design, since these fields are
written/read in sequential manner, providing one free buffer of
the size of.. so.. about 128 bytes for each beentry and storing
strings delimiting with '\0' and numbers in binary format, as an
example, would do. Additional functions to write into/read from
this buffer safely would be needed but this gives both the
ability to store longer messages and relatively short total
buffer size, and allows arbitrary number of parameters limited
only by the length of the free buffer.

What do you think about this?

I think it sounds like a mess with uncertain benefits. Now instead of
having individual fields that maybe don't fit and have to be
truncated, you have to figure out what to leave out when the overall
message doesn't fit. That's likely to lead to a lot of messy logic on
the server side, and even messier logic for any clients that read the
data and try to parse it programmatically.

Ok, I understood that the packed format itself is unaccetable.

By the way, how about giving separate columns for relname and
namespace? I think it is more usual way to designate a relation
in this kind of view and it makes the snprintf to concatenate
name and schema unnecessary(it's not significant, though). (The
following example is after pg_stat_all_tables)

I could go either way on this.

It would depends on the field length but 140 bytes can hold a
whole qualified names.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

Hello,

At Wed, 2 Dec 2015 15:48:20 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20151202184820.GL2763@alvherre.pgsql>

Vinayak wrote:

In the vacuum progress, column table_name is showing first 30 characters of
table name.
postgres=# create table test_vacuum_progress_in_postgresql(c1 int,c2 text);
postgres=# select * from pg_stat_vacuum_progress ;
-[ RECORD 1 ]-------+------------------------------
pid | 12284
table_name | public.test_vacuum_progress_i

Actually, do we really need to have the table name as a string at all
here? Why not just report the table OID? Surely whoever wants to check
the progress can connect to the database in question to figure out the
table name.

I thought the same thing but found that the same kind of view
(say, pg_stat_user_tables) has separate relanme and shcemaname in
string (not a qualified name, though).

Apart from the representation of the relation, OID would be
better as a field in beentry.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

On 2015/12/03 13:47, Kyotaro HORIGUCHI wrote:

At Wed, 2 Dec 2015 15:48:20 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote

Actually, do we really need to have the table name as a string at all
here? Why not just report the table OID? Surely whoever wants to check
the progress can connect to the database in question to figure out the
table name.

I thought the same thing but found that the same kind of view
(say, pg_stat_user_tables) has separate relanme and shcemaname in
string (not a qualified name, though).

Apart from the representation of the relation, OID would be
better as a field in beentry.

I wonder if the field should be a standalone field or as yet another
st_progress_* array?

IMHO, there are some values that a command would report that should not be
mixed with pgstat_report_progress()'s interface. That is, things like
command ID/name, command target (table name or OID) should not be mixed
with actual progress parameters like num_pages, num_indexes (integers),
processing "phase" (string) that are shared via st_progress_* fields. The
first of them already has its own reporting interface in proposed patch
in the form of pgstat_report_activity_flag(). Although, we must be careful
to choose these interfaces carefully.

Thanks,
Amit

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

Hello,

At Thu, 3 Dec 2015 14:18:50 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <565FD0BA.5020202@lab.ntt.co.jp>

On 2015/12/03 13:47, Kyotaro HORIGUCHI wrote:

At Wed, 2 Dec 2015 15:48:20 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote

Actually, do we really need to have the table name as a string at all
here? Why not just report the table OID? Surely whoever wants to check
the progress can connect to the database in question to figure out the
table name.

I thought the same thing but found that the same kind of view
(say, pg_stat_user_tables) has separate relanme and shcemaname in
string (not a qualified name, though).

Apart from the representation of the relation, OID would be
better as a field in beentry.

I wonder if the field should be a standalone field or as yet another
st_progress_* array?

IMHO, there are some values that a command would report that should not be
mixed with pgstat_report_progress()'s interface. That is, things like
command ID/name, command target (table name or OID) should not be mixed
with actual progress parameters like num_pages, num_indexes (integers),
processing "phase" (string) that are shared via st_progress_* fields. The
first of them already has its own reporting interface in proposed patch
in the form of pgstat_report_activity_flag(). Although, we must be careful
to choose these interfaces carefully.

Sorry I misunderstood the patch.

Agreed. The patch already separates integer values and texts.
And re-reviewing the patch, there's no fields necessary to be
passed as string.

total_heap_pages, scanned_heap_pages, total_index_pages,
scanned_index_pages, vacrelstats->num_index_scans are currently
in int32.

Phase can be in integer, and schema_name and relname can be
represented by one OID, uint32.

Finally, *NO* text field is needed at least this usage. So
progress_message is totally useless regardless of other usages
unknown to us.

Am I missing somethig?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

Hi,

On 2015/12/03 15:27, Kyotaro HORIGUCHI wrote:

At Thu, 3 Dec 2015 14:18:50 +0900, Amit Langote wrote

On 2015/12/03 13:47, Kyotaro HORIGUCHI wrote:

Apart from the representation of the relation, OID would be
better as a field in beentry.

I wonder if the field should be a standalone field or as yet another
st_progress_* array?

IMHO, there are some values that a command would report that should not be
mixed with pgstat_report_progress()'s interface. That is, things like
command ID/name, command target (table name or OID) should not be mixed
with actual progress parameters like num_pages, num_indexes (integers),
processing "phase" (string) that are shared via st_progress_* fields. The
first of them already has its own reporting interface in proposed patch
in the form of pgstat_report_activity_flag(). Although, we must be careful
to choose these interfaces carefully.

Sorry I misunderstood the patch.

Agreed. The patch already separates integer values and texts.
And re-reviewing the patch, there's no fields necessary to be
passed as string.

total_heap_pages, scanned_heap_pages, total_index_pages,
scanned_index_pages, vacrelstats->num_index_scans are currently
in int32.

Phase can be in integer, and schema_name and relname can be
represented by one OID, uint32.

AIUI, st_progress_message (strings) are to be used to share certain
messages as progress information. I think the latest vacuum-progress patch
uses it to report which phase lazy_scan_heap() is in, for example,
"Scanning heap" for phase 1 of its processing and "Vacuuming index and
heap" for phase 2. Those values are shown to the user in a text column
named "phase" of the pg_stat_vacuum_progress view. That said, reporting
phase as an integer value may also be worth a consideration. Some other
command might choose to do that.

Finally, *NO* text field is needed at least this usage. So
progress_message is totally useless regardless of other usages
unknown to us.

I think it may be okay at this point to add just those st_progress_*
fields which are required by lazy vacuum progress reporting. If someone
comes up with instrumentation ideas for some other command, they could
post patches to add more st_progress_* fields and to implement
instrumentation and a progress view for that command. This is essentially
what Robert said in [1]/messages/by-id/CA+TgmoYdZk9nPDtS+_kOt4S6fDRQLW+1jnJBmG0pkRT4ynxO=Q@mail.gmail.com in relation to my suggestion of taking out
st_progress_param_float from this patch.

By the way, there are some non-st_progress_* fields that I was talking
about in my previous message. For example, st_activity_flag (which I have
suggested to rename to st_command instead). It needs to be set once at the
beginning of the command processing and need not be touched again. I think
it may be a better idea to do the same for table name or OID. It also
won't change over the duration of the command execution. So, we could set
it once at the beginning where that becomes known. Currently in the patch,
it's reported in st_progress_message[0] by every pgstat_report_progress()
invocation. So, the table name will be strcpy()'d to shared memory for
every scanned block that's reported.

Thanks,
Amit

[1]: /messages/by-id/CA+TgmoYdZk9nPDtS+_kOt4S6fDRQLW+1jnJBmG0pkRT4ynxO=Q@mail.gmail.com
/messages/by-id/CA+TgmoYdZk9nPDtS+_kOt4S6fDRQLW+1jnJBmG0pkRT4ynxO=Q@mail.gmail.com

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

Hello,

At Thu, 3 Dec 2015 16:24:32 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <565FEE30.8010906@lab.ntt.co.jp>

Agreed. The patch already separates integer values and texts.
And re-reviewing the patch, there's no fields necessary to be
passed as string.

total_heap_pages, scanned_heap_pages, total_index_pages,
scanned_index_pages, vacrelstats->num_index_scans are currently
in int32.

Phase can be in integer, and schema_name and relname can be
represented by one OID, uint32.

AIUI, st_progress_message (strings) are to be used to share certain
messages as progress information. I think the latest vacuum-progress patch
uses it to report which phase lazy_scan_heap() is in, for example,
"Scanning heap" for phase 1 of its processing and "Vacuuming index and
heap" for phase 2. Those values are shown to the user in a text column
named "phase" of the pg_stat_vacuum_progress view. That said, reporting
phase as an integer value may also be worth a consideration. Some other
command might choose to do that.

Finally, *NO* text field is needed at least this usage. So
progress_message is totally useless regardless of other usages
unknown to us.

I think it may be okay at this point to add just those st_progress_*
fields which are required by lazy vacuum progress reporting. If someone
comes up with instrumentation ideas for some other command, they could
post patches to add more st_progress_* fields and to implement
instrumentation and a progress view for that command. This is essentially
what Robert said in [1] in relation to my suggestion of taking out
st_progress_param_float from this patch.

Yes. After taking a detour, though.

By the way, there are some non-st_progress_* fields that I was talking
about in my previous message. For example, st_activity_flag (which I have
suggested to rename to st_command instead). It needs to be set once at the
beginning of the command processing and need not be touched again. I think
it may be a better idea to do the same for table name or OID. It also
won't change over the duration of the command execution. So, we could set
it once at the beginning where that becomes known. Currently in the patch,
it's reported in st_progress_message[0] by every pgstat_report_progress()
invocation. So, the table name will be strcpy()'d to shared memory for
every scanned block that's reported.

If we don't have dedicate reporting functions for each phase
(that is, static data phase and progress phase), the one function
pgstat_report_progress does that by having some instruction from
the caller and it would be a bitfield.

Reading the flags for making decision whether every field to copy
or not and branching by that seems too much for int32 (or maybe
64?) fields. Alhtough it would be in effective when we have
progress_message fields, I don't think it is a good idea without
having progress_message.

pgstat_report_progress(uint32 *param1, uint16 param1_bitmap,
char param2[][..], uint16 param2_bitmap)
{
...
for(i = 0; i < 16 ; i++)
{
if (param1_bitmap & (1 << i))
beentry->st_progress_param[i] = param1[i];
if (param2_bitmap & (1 << i))
strcpy(beentry->..., param2[i]);
}

Thoughts?

Thanks,
Amit

[1]
/messages/by-id/CA+TgmoYdZk9nPDtS+_kOt4S6fDRQLW+1jnJBmG0pkRT4ynxO=Q@mail.gmail.com

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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