Here is my review for this patch
Submission Review
----------------
-Patch applies cleanly
-Patch does not include documentation changes. At a minimum: update the
table that lists what pg_stat_activity and pg_stat_replication includes
in monitoring.sgml but I propose more below.
-No tests are included but writing unit tests that depend on produce the
same output involving the hostname of the client is not possible.
Usability review
----------------
See my comments below in the testing section. The patch does do what it
says but the log_hostname issue is a usability issue (it not being
obvious why you get only null owhen log_hostname is turned off).
Documenting it might be fine. If log_hostname were new to 9.1 I would
encourage renaming it to something that implies it does more than just
control logging output but I don't see this as a good enough reason to
rename a parameter.
I think being able to see the hostnames connections in pg_stat_activity
come from is useful and it is a feature we don't already have.
Feature Test
----------------
If my connection is authorized through a line in pg_hba that uses
client_hostname then the column shows what I expect even with
log_hostname set to off.
However if I connect with a line in pg_hba that matches on an IP network
then my client_hostname is always null unless log_hostname is set to
true. This is consistent with the behavior you describe but I think the
average user will find it a bit confusing. Having a column that is
always null unless a GUC is set is less than ideal but I understand why
log_hostname isn't on by default.
I would be inclined to add an entry for these views in catalogs.sgml
that where we can then give users a pointer that they need to set
log_hostname to get anything out of this column.
If I connect through unix sockets (say psql -h /tmp --port 5433)
I was also expecting to see "[local]" in client_hostname but I am just
getting NULL. This this lookup is static I don't see why it should be
dependent on log_hostname (even with log_hostname set I'm not seeing
[local])
I have not tested this with ipv6
Performance Review
------------------
The lookup is done when the connection is established not each time the
view is queried. I don't see any performance issues
Coding Review
-------------
As Alvaro pointed out BackendStatusShmemSize should be updated.
To answer his question about why clientaddr works: clientaddr is a
SockAddr which is a structure not a pointer so the data gets copied by
value to beentry. That won't work for strings, I have no issues with
how your allocating space in beentry and then strlcpy'ing the values.
I see no issues with the implementation
I'm marking this as 'Waiting for Author' pending documentation changes
and maybe a fix the behaviour with unix sockets
Steve
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
