[WIP] pg_ping utility

On 13 October 2012 22:19, Phil Sorber <phil@omniti.com> wrote:

Based on a previous thread
(http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I
have put together a first attempt of a pg_ping utility. I am attaching
two patches. One for the executable and one for the docs.

I would also like to make a regression tests and translations, but
wanted to get feedback on what I had so far.

pg_pong:

1 packets transmitted, 1 received, 0% packet loss, time 2 days

Well this works for me, and I raised a couple typos directly to Phil.
The advantage of this over "pg_ctl status" is that it doesn't have to
be run on the machine local to the database, and access to the data
directory isn't required if it is run locally. The advantage over
connecting using a regular connection is that authentication and
authorisation isn't necessary, and if all connections are in use, it
will still return the desired result. And it does what it says on the
tin.

So +1 from me.

--
Thom

On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote:

On 13 October 2012 22:19, Phil Sorber <phil@omniti.com> wrote:

Based on a previous thread
(http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I
have put together a first attempt of a pg_ping utility. I am attaching
two patches. One for the executable and one for the docs.

I would also like to make a regression tests and translations, but
wanted to get feedback on what I had so far.

pg_pong:

1 packets transmitted, 1 received, 0% packet loss, time 2 days

Well this works for me, and I raised a couple typos directly to Phil.
The advantage of this over "pg_ctl status" is that it doesn't have to
be run on the machine local to the database, and access to the data
directory isn't required if it is run locally. The advantage over
connecting using a regular connection is that authentication and
authorisation isn't necessary, and if all connections are in use, it
will still return the desired result. And it does what it says on the
tin.

So +1 from me.

Why not add a pg_ctl subcommand for that? For me that sounds like a good place
for it...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Oct 15, 2012 at 5:32 PM, Andres Freund <andres@2ndquadrant.com> wrote:

On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote:

On 13 October 2012 22:19, Phil Sorber <phil@omniti.com> wrote:

Based on a previous thread
(http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I
have put together a first attempt of a pg_ping utility. I am attaching
two patches. One for the executable and one for the docs.

I would also like to make a regression tests and translations, but
wanted to get feedback on what I had so far.

pg_pong:

1 packets transmitted, 1 received, 0% packet loss, time 2 days

Well this works for me, and I raised a couple typos directly to Phil.
The advantage of this over "pg_ctl status" is that it doesn't have to
be run on the machine local to the database, and access to the data
directory isn't required if it is run locally. The advantage over
connecting using a regular connection is that authentication and
authorisation isn't necessary, and if all connections are in use, it
will still return the desired result. And it does what it says on the
tin.

So +1 from me.

Why not add a pg_ctl subcommand for that? For me that sounds like a good place
for it...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

We discussed that in the other thread. pg_ctl is often only (always?)
packaged with the server binaries and not client. Discussed adding it
to psql, but Tom said he'd prefer to see it as a standalone binary
anyway. I don't have any real strong opinion about it going into an
existing binary like psql (I have a patch for this too) or being
standalone, I just think we should have some way to do this from the
command line on a client. It seems trivial, but I think it's very
useful and if our libpq already supports this, why not?

FWIW pg_ctl does use the same API (PQping), but it doesn't expose it
as an option you can use exclusively. It just uses it to make sure the
server is up/down depending on what it is trying to do.

Andres Freund <andres@2ndquadrant.com> writes:

Why not add a pg_ctl subcommand for that? For me that sounds like a good place
for it...

I think that's a bad fit, because every other pg_ctl subcommand requires
access to the data directory. It would be very confusing if this one
subcommand worked remotely when the others didn't.

There was also some discussion of wedging it into psql, which would at
least have the advantage that it'd typically be installed on the right
side of the client/server divide. But I still think "wedging into" is
the appropriate verb there: psql is a tool for making a connection and
executing some SQL commands, and "ping" is not that.

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does. But a lot of the programs in src/bin/scripts are not much
bigger. (In fact that might be the best place for this.)

regards, tom lane

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-
owner@postgresql.org] On Behalf Of Tom Lane
Sent: Monday, October 15, 2012 7:13 PM
To: Andres Freund
Cc: pgsql-hackers@postgresql.org; Thom Brown; Phil Sorber
Subject: Re: [HACKERS] [WIP] pg_ping utility

Andres Freund <andres@2ndquadrant.com> writes:

Why not add a pg_ctl subcommand for that? For me that sounds like a
good place for it...

I think that's a bad fit, because every other pg_ctl subcommand requires
access to the data directory. It would be very confusing if this one
subcommand worked remotely when the others didn't.

There was also some discussion of wedging it into psql, which would at

least

have the advantage that it'd typically be installed on the right side of

the

client/server divide. But I still think "wedging into" is the appropriate

verb

there: psql is a tool for making a connection and executing some SQL
commands, and "ping" is not that.

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does. But a lot of the programs in src/bin/scripts are not much

bigger.

(In fact that might be the best place for this.)

regards, tom lane

This seems to be begging for a canonical "pg_monitor" command where
"pg_ping" would be one sub-command. A bit much for a single command but it
would provide a frame onto which additional user interfaces could be hung -
though I am lacking for concrete examples at the moment. pg_monitor would
be focused on "database" monitoring and not "cluster" monitoring generally
but pg_ping would be a necessary pre-requisite since if the cluster is not
available database monitoring doesn't make any sense.

With the recent focus on pg_stat_statements and the current WIP on
"pg_lwlocks" having an official UI for accessing much of this kind data has
merit. Encapsulating the queries into commands makes actually using them
easier and there can be associated documentation discussing how to interpret
those specific "commands" and some level of consistency when asking for data
for bug and performance reports. It may be that psql already does much of
this as I am just not that familiar with the program but if that is the case
then classifying it as "making a connection and executing some SQL commands"
is a limited description. pg_ping is arguably doing at least the first part
of that.

David J.

On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@2ndquadrant.com> writes:

Why not add a pg_ctl subcommand for that? For me that sounds like a good place
for it...

I think that's a bad fit, because every other pg_ctl subcommand requires
access to the data directory. It would be very confusing if this one
subcommand worked remotely when the others didn't.

There was also some discussion of wedging it into psql, which would at
least have the advantage that it'd typically be installed on the right
side of the client/server divide. But I still think "wedging into" is
the appropriate verb there: psql is a tool for making a connection and
executing some SQL commands, and "ping" is not that.

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does. But a lot of the programs in src/bin/scripts are not much
bigger. (In fact that might be the best place for this.)

regards, tom lane

I considered src/bin/scripts but all those are for maintenance tasks
on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the
bits in common.h/common.c, nor does it need some of the includes that
the build process has. I would also like it to have a regression test
which none of those seem to have. Seems like it would be a bit of a
wedge there as well, but if we did, maybe we call it pingdb instead?

If there is consensus that we want it to live there, I can write a
patch for that as well. Though just to be clear my preference at this
point is still for a standalone binary.

"David Johnston" <polobo@yahoo.com> writes:

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does. But a lot of the programs in src/bin/scripts are not much
bigger. (In fact that might be the best place for this.)

This seems to be begging for a canonical "pg_monitor" command where
"pg_ping" would be one sub-command. A bit much for a single command but it
would provide a frame onto which additional user interfaces could be hung -
though I am lacking for concrete examples at the moment.

Meh. If we had near-term plans for more such subcommands, that might
make sense. But I think all that's really wanted here is a command-line
wrapper around the libpq PQping() functionality. People who are trying
to build more complex monitoring tools are likely to be using PQping()
directly anyway, rather than invoking a command-line tool.

With the recent focus on pg_stat_statements and the current WIP on
"pg_lwlocks" having an official UI for accessing much of this kind data has
merit.

None of that stuff is accessible without opening up an actual database
connection, and IMO the whole point of PQping is that it *doesn't* open
a connection and thus doesn't get into problems of user authentication.
So I really think it's a different sort of animal that needs a separate
API.

regards, tom lane

Phil Sorber <phil@omniti.com> writes:

On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does. But a lot of the programs in src/bin/scripts are not much
bigger. (In fact that might be the best place for this.)

I considered src/bin/scripts but all those are for maintenance tasks
on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the
bits in common.h/common.c, nor does it need some of the includes that
the build process has.

Well, we classify all those programs as client-side tools in the
documentation, so I don't see that pg_ping doesn't belong there.

The alternative is to give it its very own subdirectory under src/bin/;
which increases the infrastructure burden *significantly* (eg, now it
needs its own NLS message catalog) for not a lot of value IMO.

I would also like it to have a regression test
which none of those seem to have.

[ shrug... ] There is nothing in the current regression infrastructure
that would work for this, so that desire is pie-in-the-sky regardless of
where you put it in the source tree. Also, PQping itself is exercised
in every buildfarm run as part of "pg_ctl start", so I don't feel a real
strong need to test pg_ping separately.

regards, tom lane

On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Phil Sorber <phil@omniti.com> writes:

I would also like it to have a regression test
which none of those seem to have.

[ shrug... ] There is nothing in the current regression infrastructure
that would work for this, so that desire is pie-in-the-sky regardless of
where you put it in the source tree. Also, PQping itself is exercised
in every buildfarm run as part of "pg_ctl start", so I don't feel a real
strong need to test pg_ping separately.

My plan was to borrow heavily from the pg_upgrade test. I want to
verify the exit status based on known database state as presumably
people would be using this for monitoring/load balancing, etc. Hoping
to prevent silly breakage like the help output from returning an
'Accepting Connections' exit status.

On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Phil Sorber <phil@omniti.com> writes:

On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does. But a lot of the programs in src/bin/scripts are not much
bigger. (In fact that might be the best place for this.)

I considered src/bin/scripts but all those are for maintenance tasks
on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the
bits in common.h/common.c, nor does it need some of the includes that
the build process has.

Well, we classify all those programs as client-side tools in the
documentation, so I don't see that pg_ping doesn't belong there.

The alternative is to give it its very own subdirectory under src/bin/;
which increases the infrastructure burden *significantly* (eg, now it
needs its own NLS message catalog) for not a lot of value IMO.

I would also like it to have a regression test
which none of those seem to have.

[ shrug... ] There is nothing in the current regression infrastructure
that would work for this, so that desire is pie-in-the-sky regardless of
where you put it in the source tree. Also, PQping itself is exercised
in every buildfarm run as part of "pg_ctl start", so I don't feel a real
strong need to test pg_ping separately.

regards, tom lane

Here is the new patch. I renamed the utility from pg_ping to pingdb to
go along with the naming convention of src/bin/scripts. Updated docs
and made some other minor improvements.

Phil Sorber <phil@omniti.com> writes:

Here is the new patch. I renamed the utility from pg_ping to pingdb to
go along with the naming convention of src/bin/scripts.

Uh, no, that's not a step forward. Leaving out a "pg" prefix from those
script names is universally agreed to have been a mistake. We've not
felt that changing the legacy names is worth the amount of pain it'd
cause, but that doesn't mean that we should propagate the mistake into
brand new executable names.

regards, tom lane

On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Phil Sorber <phil@omniti.com> writes:

Here is the new patch. I renamed the utility from pg_ping to pingdb to
go along with the naming convention of src/bin/scripts.

Uh, no, that's not a step forward. Leaving out a "pg" prefix from those
script names is universally agreed to have been a mistake. We've not
felt that changing the legacy names is worth the amount of pain it'd
cause, but that doesn't mean that we should propagate the mistake into
brand new executable names.

regards, tom lane

Ok. I asked about this and got no response so I assume there were no
strong opinions. I have reverted to the pg_ping name. Patches
attached.

On 10/22/12 11:47 AM, Phil Sorber wrote:

On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Phil Sorber <phil@omniti.com> writes:

Here is the new patch. I renamed the utility from pg_ping to pingdb to
go along with the naming convention of src/bin/scripts.

Uh, no, that's not a step forward. Leaving out a "pg" prefix from those
script names is universally agreed to have been a mistake. We've not
felt that changing the legacy names is worth the amount of pain it'd
cause, but that doesn't mean that we should propagate the mistake into
brand new executable names.

regards, tom lane

Ok. I asked about this and got no response so I assume there were no
strong opinions. I have reverted to the pg_ping name. Patches
attached.

Quick review ...

Code:

*************** install: all installdirs
*** 54,59 ****
--- 55,61 ----
        $(INSTALL_PROGRAM) clusterdb$(X)  '$(DESTDIR)$(bindir)'/clusterdb$(X)
        $(INSTALL_PROGRAM) vacuumdb$(X)   '$(DESTDIR)$(bindir)'/vacuumdb$(X)
        $(INSTALL_PROGRAM) reindexdb$(X)  '$(DESTDIR)$(bindir)'/reindexdb$(X)
+       $(INSTALL_PROGRAM) pg_ping$(X)     '$(DESTDIR)$(bindir)'/pg_ping$(X)

installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'

Whitespace misaligned

+ exit(3); // Return UNKNOWN

No // comments.

+ while (NULL != conn_opt_ptr->keyword)

Better writte as

while (conn_opt_ptr->keyword != NULL)

or

while (conn_opt_ptr->keyword)

Also, it seems that about 75% of the patch is connection options processing. How about
we get rid of all that and just have them specify a connection string? It would be a break
with tradition, but maybe it's time for something new.

Functionality:

I'm missing the typical ping functionality to ping continuously. If we're going to call
it pg_ping, it ought to do something similar to ping, I think.

On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 10/22/12 11:47 AM, Phil Sorber wrote:
Also, it seems that about 75% of the patch is connection options processing. How about
we get rid of all that and just have them specify a connection string? It would be a break
with tradition, but maybe it's time for something new.

I'd be pretty pleased if it had just two ways to get configured:
a) A connection string (which might, in the new order of things, be a
JDBC-like URI), or
b) Environment values drawn in from PGHOST/PGPORT/...

That's pretty much enough configurability, I'd think.

Functionality:

I'm missing the typical ping functionality to ping continuously. If we're going to call
it pg_ping, it ought to do something similar to ping, I think.

Yep, should have equivalents to:
-i, an interval between pings,
-c, a count
-w/-W, a timeout interval

Might be nice to have analogues to:
-D printing timestamp before each line
-q quiets output
-v verbose output (got it, check!)
-V version (got it, check!)
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

Quick review ...

Code:

*************** install: all installdirs
*** 54,59 ****
--- 55,61 ----
$(INSTALL_PROGRAM) clusterdb$(X)  '$(DESTDIR)$(bindir)'/clusterdb$(X)
$(INSTALL_PROGRAM) vacuumdb$(X)   '$(DESTDIR)$(bindir)'/vacuumdb$(X)
$(INSTALL_PROGRAM) reindexdb$(X)  '$(DESTDIR)$(bindir)'/reindexdb$(X)
+       $(INSTALL_PROGRAM) pg_ping$(X)     '$(DESTDIR)$(bindir)'/pg_ping$(X)

installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'

Whitespace misaligned

Fixed.

+ exit(3); // Return UNKNOWN

No // comments.

Changed.

+ while (NULL != conn_opt_ptr->keyword)

Better writte as

while (conn_opt_ptr->keyword != NULL)

or

while (conn_opt_ptr->keyword)

Changed to the latter.

Also, it seems that about 75% of the patch is connection options processing. How about
we get rid of all that and just have them specify a connection string? It would be a break
with tradition, but maybe it's time for something new.

I don't think that should be a part of this patch. If we think that we
should remove connection params and just pass a connection string we
should probably deprecate connection params and remove them everywhere
together over a period of time like with other features. I don't think
we should introduce a new binary that doesn't work the same way as all
the other binaries.

I went back and checked, and realized I didn't do it correctly, but
this patch now does allow a connection string to be passed to -d. This
seems to be the accepted way to do this.

Functionality:

I'm missing the typical ping functionality to ping continuously. If we're going to call
it pg_ping, it ought to do something similar to ping, I think.

It's not named after the network utility but after the libpq function
PQping. Since this doesn't output latencies or really much of anything
else like the network ping, I'm not sure it makes sense to do that,
but I could be convinced otherwise.

Attaching an updated patch.

On Tue, Oct 23, 2012 at 6:22 PM, Christopher Browne <cbbrowne@gmail.com> wrote:

On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 10/22/12 11:47 AM, Phil Sorber wrote:
Also, it seems that about 75% of the patch is connection options processing. How about
we get rid of all that and just have them specify a connection string? It would be a break
with tradition, but maybe it's time for something new.

I'd be pretty pleased if it had just two ways to get configured:
a) A connection string (which might, in the new order of things, be a
JDBC-like URI), or
b) Environment values drawn in from PGHOST/PGPORT/...

When I first wrote this for my own purposes it was basically 'return
PQping("");' with the necessary glue around it and I used the env
var's exactly as you describe. I ended up adding connection parameters
to satisfy the ops guy who was having trouble integrating it how we
wanted to use it. I figured that to go in core it would need that
anyway. I'm not sure why we would want to prevent people from using
command line options like that. That is often the most intuitive way
to use a tool. Either way I think this is probably a separate debate
entirely.

That's pretty much enough configurability, I'd think.

Functionality:

I'm missing the typical ping functionality to ping continuously. If we're going to call
it pg_ping, it ought to do something similar to ping, I think.

Yep, should have equivalents to:
-i, an interval between pings,
-c, a count
-w/-W, a timeout interval

Like I replied to Peter above, I'm not sure it fits the mold of the
ping network utility. If you think it needs a different name please
propose one. I'm not totally closed off to this idea, it's just not
what I had in mind when I put it together. If people find it useful, I
can add it.

Might be nice to have analogues to:
-D printing timestamp before each line
-q quiets output
-v verbose output (got it, check!)
-V version (got it, check!)

Right now it outputs nothing by default. I suppose I could change it
to output "Accepting/Rejecting Connections" by default, and verbose
can add the connection info. Thoughts?

Guys,

May I remind everyone that we still have an commitfest open, which to
date has 23 patches needing some effort, and that this patch, while
probably very useful and interesting, belongs to the next commitfest
which is not yet in progress.

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

On 24 October 2012 15:24, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Guys,

May I remind everyone that we still have an commitfest open, which to
date has 23 patches needing some effort, and that this patch, while
probably very useful and interesting, belongs to the next commitfest
which is not yet in progress.

Phil added it to the 2012-11 commitfest, which appears to be the
correct one. The "in progress" one is 2012-09.

--
Thom

Thom Brown wrote:

On 24 October 2012 15:24, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Guys,

May I remind everyone that we still have an commitfest open, which to
date has 23 patches needing some effort, and that this patch, while
probably very useful and interesting, belongs to the next commitfest
which is not yet in progress.

Phil added it to the 2012-11 commitfest, which appears to be the
correct one. The "in progress" one is 2012-09.

You're correct. So am I -- except that the word "open" in my paragraph
above should have been "in progress". We do need people to work on the
patches in the 2012-09 commitfest.

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