Discussion:
[TODO] Track number of files ready to be archived in pg_stat_archiver
Julien Rouhaud
2014-08-21 08:17:14 UTC
Permalink
Hello,

Attached patch implements the following TODO item :

Track number of WAL files ready to be archived in pg_stat_archiver

However, it will track the total number of any file ready to be
archived, not only WAL files.

Please let me know what you think about it.

Regards.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
Gilles Darold
2014-08-25 17:00:09 UTC
Permalink
Post by Julien Rouhaud
Hello,
Track number of WAL files ready to be archived in pg_stat_archiver
However, it will track the total number of any file ready to be
archived, not only WAL files.
Please let me know what you think about it.
Regards.
Hi,

Maybe looking at archive ready count will be more efficient if it is
done in the view definition through a function. This will avoid any
issue with incrementing/decrement of archiverStats.ready_count and the
patch will be more simple. Also I don't think we need an other memory
allocation for that, the counter information is always in the number of
.ready files in the archive_status directory and the call to
pg_stat_archiver doesn't need high speed performances.

For example having a new function called
pg_stat_get_archive_ready_count() that does the same at what you add
into pgstat_read_statsfiles() and the pg_stat_archiver defined as follow:

CREATE VIEW pg_stat_archiver AS
s.failed_count,
s.last_failed_wal,
s.last_failed_time,
pg_stat_get_archive_ready() as ready_count,
s.stats_reset
FROM pg_stat_get_archiver() s;

The function pg_stat_get_archive_ready_count() will also be available
for any other querying.
--
Gilles Darold
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Julien Rouhaud
2014-08-27 22:37:37 UTC
Permalink
Post by Gilles Darold
Post by Julien Rouhaud
Hello,
Track number of WAL files ready to be archived in pg_stat_archiver
However, it will track the total number of any file ready to be
archived, not only WAL files.
Please let me know what you think about it.
Regards.
Hi,
Maybe looking at archive ready count will be more efficient if it is
done in the view definition through a function. This will avoid any
issue with incrementing/decrement of archiverStats.ready_count and the
patch will be more simple. Also I don't think we need an other memory
allocation for that, the counter information is always in the number of
.ready files in the archive_status directory and the call to
pg_stat_archiver doesn't need high speed performances.
For example having a new function called
pg_stat_get_archive_ready_count() that does the same at what you add
CREATE VIEW pg_stat_archiver AS
s.failed_count,
s.last_failed_wal,
s.last_failed_time,
pg_stat_get_archive_ready() as ready_count,
s.stats_reset
FROM pg_stat_get_archiver() s;
The function pg_stat_get_archive_ready_count() will also be available
for any other querying.
Indeed, this approach should be more efficient. It also avoid unexpected
results, like if someone has the bad idea to remove a .ready file in
pg_xlog/archive_status directory.

Attached v2 patch implements this approach. All the work is still done
in pg_stat_get_archiver, as I don't think that having a specific
function for that information would be really interesting.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
Michael Paquier
2014-08-28 03:58:44 UTC
Permalink
On Thu, Aug 28, 2014 at 7:37 AM, Julien Rouhaud
Post by Julien Rouhaud
Attached v2 patch implements this approach. All the work is still done
in pg_stat_get_archiver, as I don't think that having a specific
function for that information would be really interesting.
Please be sure to add that to the next commit fest. This is a feature
most welcome within this system view.
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Brightwell, Adam
2014-10-21 05:35:53 UTC
Permalink
Julien,

The following is an initial review:

* Applies cleanly to master (f330a6d).
* Regression tests updated and pass, including 'check-world'.
* Documentation updated and builds successfully.
* Might want to consider replacing the following magic number with a
constant or perhaps calculated value.

+ int basenamelen = (int) strlen(rlde->d_name) - 6;

* Wouldn't it be easier, or perhaps more reliable to use "strrchr()" with
the following instead?

+ strcmp(rlde->d_name + basenamelen, ".ready") == 0)

char *extension = strrchr(ride->d_name, '.');
...
strcmp(extension, ".ready") == 0)

I think this approach might also help to resolve the magic number above.
For example:

char *extension = strrchr(ride->d_name, '.');
int basenamelen = (int) strlen(ride->d_name) - strlen(extension);

-Adam
--
Adam Brightwell - ***@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Julien Rouhaud
2014-10-21 10:23:00 UTC
Permalink
On Tue, Oct 21, 2014 at 7:35 AM, Brightwell, Adam <
Post by Brightwell, Adam
Julien,
Thanks for the review.
Post by Brightwell, Adam
* Applies cleanly to master (f330a6d).
* Regression tests updated and pass, including 'check-world'.
* Documentation updated and builds successfully.
* Might want to consider replacing the following magic number with a
constant or perhaps calculated value.
+ int basenamelen = (int) strlen(rlde->d_name) - 6;
* Wouldn't it be easier, or perhaps more reliable to use "strrchr()" with
the following instead?
+ strcmp(rlde->d_name + basenamelen, ".ready") == 0)
char *extension = strrchr(ride->d_name, '.');
...
strcmp(extension, ".ready") == 0)
I think this approach might also help to resolve the magic number above.
char *extension = strrchr(ride->d_name, '.');
int basenamelen = (int) strlen(ride->d_name) - strlen(extension);
Actually, I used the same loop as the archiver one (see
backend/postmaster/pgarch.c, function pgarch_readyXlog) to get the exact
same number of files.

If we change it in this patch, it would be better to change it everywhere.
What do you think ?

-Adam
Post by Brightwell, Adam
--
Database Engineer - www.crunchydatasolutions.com
Brightwell, Adam
2014-10-21 15:50:41 UTC
Permalink
Julien,
Post by Julien Rouhaud
Actually, I used the same loop as the archiver one (see
backend/postmaster/pgarch.c, function pgarch_readyXlog) to get the exact
same number of files.
Ah, I see.
Post by Julien Rouhaud
If we change it in this patch, it would be better to change it everywhere.
What do you think ?
Hmm... I'd have to defer to the better judgement of a committer on that
one. Though, I would think that the general desire would be to keep the
patch relevant ONLY to the necessary changes. I would not qualify making
those types of changes as relevant, IMHO. I do think this is potential for
cleanup, however, I would suspect that would be best done in a separate
patch. But again, I'd defer to a committer whether such changes are even
necessary/acceptable.

-Adam
--
Adam Brightwell - ***@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Loading...