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.
