Re: [PROPOSAL] VACUUM Progress Checker.

From: "Syed, Rahila" <Rahila(dot)Syed(at)nttdata(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, "PostgreSQL-development (pgsql-hackers(at)postgresql(dot)org)" <pgsql-hackers(at)postgresql(dot)org>, "Robert Haas (robertmhaas(at)gmail(dot)com)" <robertmhaas(at)gmail(dot)com>
Subject: Re: [PROPOSAL] VACUUM Progress Checker.
Date: 2015-10-29 14:22:56
Message-ID: C3C878A2070C994B9AE61077D46C38468EE65241@MAIL703.KDS.KEANE.COM
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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.

Attachment Content-Type Size
Vacuum_progress_checker_v6.patch application/octet-stream 17.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Oleksii Kliukin 2015-10-29 14:29:43 Re: [ADMIN] Replication slots and isolation levels
Previous Message Fujii Masao 2015-10-29 14:16:32 Re: Support for N synchronous standby servers - take 2