Re: log_hostname and pg_stat_activity - Mailing list pgsql-hackers

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