fix for issue #26

[istr]

Fix for issue #26. Finally it was easy to derive looking at the difference between src/ngx_postgres_keepalive.h and nginx src/http/modules/ngx_http_upstream_keepalive_module.c (ngx_http_upstream_keepalive_cache_t vs. ngx_postgres_keepalive_cache_t). The key is to store the socket address in u_char sockaddr[NGX_SOCKADDRLEN] instead of struct sockaddr and memcpy where appropriate. Please review and pull.

[agentzh]

@istr Your patch breaks the build on my side:

/home/agentzh/git/postgres-nginx-module/src/ngx_postgres_keepalive.c: In function ‘ngx_postgres_keepalive_get_peer_single’:
/home/agentzh/git/postgres-nginx-module/src/ngx_postgres_upstream.c: In function ‘ngx_postgres_upstream_get_peer’:
/home/agentzh/git/postgres-nginx-module/src/ngx_postgres_keepalive.c:107:22: error: assignment from incompatible pointer type [-Werror]
         pc->sockaddr = &pgp->sockaddr;
                      ^
/home/agentzh/git/postgres-nginx-module/src/ngx_postgres_upstream.c:299:18: error: assignment from incompatible pointer type [-Werror]
     pc->sockaddr = &pgdt->sockaddr;
                  ^
cc1: all warnings being treated as errors
cc1: all warnings being treated as errors

Will you fix those?

Also, it'll be great if you can add a test case for the corresponding issue to the existing test suite. Thanks!

[agentzh]

@istr Let's split this thing into two phases:

1. Add IPv6 support (we do not support it at all right now).
2. Add unix domain socket support.

For phase 1, I've committed a modified version of your patch (with test cases!) to the ipv6 branch as commit 3b7d3e1d. Will you try it out on your side? Thanks!

[istr]

To be sure that it was not the fault of the ipv6 patch I ran the testsuite against master. There seems to be a problem with the bigpipe test:

Test Summary Report
-------------------
t/bigpipe.t     (Wstat: 1536 Tests: 12 Failed: 6)
  Failed tests:  2, 4, 6, 8, 10, 12
  Non-zero exit status: 6
t/errors.t      (Wstat: 512 Tests: 16 Failed: 2)
  Failed tests:  3-4
  Non-zero exit status: 2

The errors.t tests failing are due to my psql setup (allows login w/o credentials from localhost). It would be great if the test suite could somehow check and/or create the prerequisites for these tests and skip them if it finds that all local connections are allowed.

Example of a failing bigpipe test:

t/bigpipe.t ....... WARNING: TEST 3: asynchronous (with echo filter) - unexpected extra bytes after last chunk in response: "[{"id":2,"name":null},{"id":3,"name":"bob"}]"
t/bigpipe.t ....... 1/12 
#   Failed test 'TEST 3: asynchronous (with echo filter) - response_body - response is expected'
#   at /usr/local/share/perl/5.14.2/Test/Nginx/Socket.pm line 1155.
#          got: ..."der.load( )</script>\x{0a}<script type="text/javascript"...
#       length: 165
#     expected: ..."der.load( [{"id":2,"name":null},{"id":3,"name":"bo"...
#       length: 253
#     strings begin to differ at char 93 (line 2 column 45)
WARNING: TEST 3: asynchronous (with echo filter) - 2014/08/07 11:57:15 [alert] 19687#0: *1 the http output chain is empty while sending to client, client: 127.0.0.1, server: localhost, request: \"GET /bigpipe HTTP/1.1\", host: \"localhost\" at /usr/local/share/perl/5.14.2/Test/Nginx/Socket.pm line 1065.
WARNING: TEST 3: asynchronous (with echo filter) - unexpected extra bytes after last chunk in response: "[{"id":2,"name":null},{"id":3,"name":"bob"}]"

[agentzh]

@istr The "TEST 3" in bigpipe.t uses the echo_location_async directive from the ngx_echo module, which requires the standard ngx_ssi module to function properly. See https://github.com/openresty/echo-nginx-module#known-issues

The standard ngx_ssi module should be enabled by default in nginx. Are you explicitly disabling it in your nginx build with the --without-http_ssi_module option?

Regarding automatically skipping tests in errors.t, I don't think that is a good idea. The developers might accidentally introduce a regression that is covered by those skipped tests (and Murphy's Law always applies ;)).

[istr]

Ok, thanks for the hint wrt bigpipe. I tend to only compile in what is needed for each project. So my configuration looks like this:

./configure \
  --prefix=${PREFIX} \
  --with-cc-opt="-O0 -g -Werror" \
  --with-no-pool-patch \
  --pid-path=/var/run/orsty.pid \
  --error-log-path=/var/log/orsty/error.log \
  --http-fastcgi-temp-path=/var/tmp/orsty/fcgi \
  --http-uwsgi-temp-path=/var/tmp/orsty/uwsgi \
  --http-scgi-temp-path=/var/tmp/orsty/scgi \
  --http-client-body-temp-path=/var/tmp/orsty/cbody \
  --http-proxy-temp-path=/var/tmp/orsty/proxy \
  --with-http_ssl_module \
  --with-http_spdy_module \
  --with-http_auth_request_module \
  --with-http_gzip_static_module \
  --with-http_stub_status_module \
  --with-file-aio \
  --with-debug \
  --without-http_autoindex_module \
  --without-http_browser_module \
  --without-http_map_module \
  --without-http_ssi_module \
  --without-http_memcached_module \
  --without-mail_pop3_module \
  --without-mail_imap_module \
  --without-mail_smtp_module \
  --without-http_userid_module \
  --without-http_uwsgi_module \
  --add-module=${PKG}/patch/nginx/naxsi-0.53-2/naxsi_src \
  --with-http_postgres_module

With "skipping" I did not mean "silently skipping" but skipping with an informative message, just like "skipping: this is not BeOS" and so on. Producing a test failure here might give a useful implicit hint that says: "maybe configuring your postgres to allow unconditional local access is not a good idea in most situations". An explicit skip / warning may be even more helpful, though: "skipping errors.t/2: your postgres allows local access without credentials given"

[agentzh]

@istr It'll require invoking psql or some other "trusted" Pg client to do the check. I'm just wondering if it's worth the trouble :) Anyway, patches welcome :)

[agentzh]

@istr Also, I still think letting the tests fail in this case is a good idea. People tend to be lazy and they won't bother reading any hints when they see "All tests successful". Failures are the best way to encourage people to investigate the issue.

[istr]

Wrt test failure: ok, your argument is right. :-)

The pull request now contains:

• only the changes for ipv6
• your modified patch
• another slight change to that patch to make it compile with "-Werror"

The module passes the complete basic test suite using:

• openresty 1.7.0.1
• (replaced) nginx version 1.7.4
• postgresql 9.3.4

The integration with nginx-eval_module is NOT tested yet. The advertised TEST_NGINX_IGNORE_MISSING_DIRECTIVES=1 does not work for me; I have to move the additional test cases away:

FAILED--Further testing stopped: check_if_missing_directives: Cannot open ...../ngx_postgres-1.0rc3-my/t/servroot/logs/error.log for reading: No such file or directory

It seems that the error.log is discarded too early to perform the "unknown directive" check. Please check; I will put the unix socket patch with a test case in a different branch.

[agentzh]

@istr Now the differences between my ipv6 branch and your master branch are like below (I've manually rebased your master with the upstream master locally):

diff --git a/src/ngx_postgres_keepalive.c b/src/ngx_postgres_keepalive.c
index 6f88563..df01cf9 100644
--- a/src/ngx_postgres_keepalive.c
+++ b/src/ngx_postgres_keepalive.c
@@ -130,7 +130,7 @@ ngx_postgres_keepalive_get_peer_multi(ngx_peer_connection_t *pc,
         item = ngx_queue_data(q, ngx_postgres_keepalive_cache_t, queue);
         c = item->connection;

-        if (ngx_memn2cmp((u_char *) &item->sockaddr, (u_char *) pc->sockaddr,
+        if (ngx_memn2cmp((u_char *) item->sockaddr, (u_char *) pc->sockaddr,
                 item->socklen, pc->socklen) == 0)
         {
             ngx_queue_remove(q);
@@ -243,7 +243,7 @@ ngx_postgres_keepalive_free_peer(ngx_peer_connection_t *pc,
         c->write->log = ngx_cycle->log;

         item->socklen = pc->socklen;
-        ngx_memcpy(&item->sockaddr, pc->sockaddr, pc->socklen);
+        ngx_memcpy(item->sockaddr, pc->sockaddr, pc->socklen);

         item->pgconn = pgp->pgconn;

You said you were seeing C compiler warnings (or errors with -Werror) with the ipv6 branch but I'm not seeing anything on my side with gcc 4.8.3 and clang 3.4. What version of C compiler are you using? What is the raw warning/error messages you were seeing? Also, the way used in my ipv6 branch is also identical to the corresponding code in the src/http/modules/ngx_http_upstream_keepalive_module.c file in nginx 1.7.4. I wonder why you did not get similar warnings while compiling the nginx core. Also, these two ways should be semantically equivalent.

[agentzh]

@istr Regarding TEST_NGINX_IGNORE_MISSING_DIRECTIVES=1, I've tested on my side with the latest git master version of Test::Nginx and seen it is working for me:

$ export TEST_NGINX_IGNORE_MISSING_DIRECTIVES=1
$ prove -I../test-nginx/lib t/eval.t
t/eval.t .. nginx: [emerg] unknown directive "eval_subrequest_in_memory" in /home/agentzh/git/postgres-nginx-module/t/servroot/conf/nginx.conf:43
t/eval.t .. 1/12 nginx: [emerg] unknown directive "eval_subrequest_in_memory" in /home/agentzh/git/postgres-nginx-module/t/servroot/conf/nginx.conf:43
t/eval.t .. ok     
All tests successful.
Files=1, Tests=12,  0 wallclock secs ( 0.03 usr  0.00 sys +  0.15 cusr  0.06 csys =  0.24 CPU)
Result: PASS

Will you try the latest git version here? https://github.com/openresty/test-nginx Thanks!

[agentzh]

@istr BTW, it's still recommended to compile in the ngx_eval module (https://github.com/openresty/nginx-eval-module ) to enable these tests on your side :)

[istr]

Ok, will try to compile in ngx_eval and run the full test suite. The basic test suite passed with valgrind turned on (after a slight patch, see https://github.com/openresty/test-nginx/pull/17). During the test valgrind found a potential problem in lj_str_new (openresty). This is the valgrind.supress rule:

{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:lj_str_new
   fun:lua_pushlstring
   fun:emptybuffer
   fun:luaL_pushresult
   fun:luaL_gsub
   fun:setpath
   fun:luaopen_package
   fun:lj_BC_FUNCC
   fun:luaL_openlibs
   fun:ngx_http_lua_new_state
   fun:ngx_http_lua_init_vm
   fun:ngx_http_lua_init
   fun:ngx_http_block
   fun:ngx_conf_handler
   fun:ngx_conf_parse
   fun:ngx_init_cycle
   fun:main
}

Maybe you can tell if this is a false positive.

[agentzh]

@istr Yes, it is a false positive due to an optimization in the LuaJIT VM. We usually run a special version of LuaJIT that disables such optimizations and also enables system allocators in LuaJIT (to prevent LuaJIT's own allocator from confusing valgrind, the same applies to nginx's own memory pools).

For OpenResty, it's as easy as:

./configure --prefix=/opt/openresty-valgrind --with-debug --with-no-pool-patch \
          --with-luajit-xcflags='-DLUAJIT_USE_VALGRIND -DLUAJIT_USE_SYSMALLOC'

If you really want to use the nginx 1.7.4 core with the OpenResty bundle, then use the following prerelease tarball instead:

https://openresty.org/download/ngx_openresty-1.7.4.1rc0.tar.gz

Do not replace OpenResty's own nginx core directly with the official version because certain things may break like the --with-no-pool-patch option above (also the official nginx still has some known bugs though it is slowly merging my patches).

[agentzh]

@istr FYI, for everyday C-level development, however, I usually use the following scripts to build everything (like ngx_postgres) instead of using the OpenResty bundle directly:

http://agentzh.org/misc/nginx/ngx-postgres-build.sh

https://github.com/openresty/nginx-devel-utils/tree/master/ngx-build

Similar shell scripts are used for building custom LuaJIT and etc :)

[istr]

Ok, thanks for the hint wrt replacing nginx directly from vanilla sources. Up to now I never had any issue with that (maybe good luck?).

Regarding your question to the diff beetween ipv6 branch and my master (now ipv6, too): the issue I had with warning/error was with the duplicate/wrong assignment, just like you, see commit https://github.com/istr/ngx_postgres/commit/71b47117f2280c827189150e86de7c3035f8782e#diff-d41d8cd98f00b204e9800998ecf8427e.

I tend not to dereference what is already an address (uchar[...]) so I always prefer to write foo->bar instead of &foo->bar if feasible, even if it may be semantically equivalent.

[agentzh]

@istr I do understand your reasoning regarding &foo->bar. But I'd rather follow the nginx core's corresponding code as much as possible. Also it makes the patch smaller ;)

@PiotrSikora What's your opinion on this? I don't really mind actually :)

[agentzh]

@istr Yes, the pending issues are just on special code paths. You need some luck to trigger them but you still could (and some users indeed did in the past and that was exactly how they were discovered in the first place) ;)

[istr]

Closing, as there is a fix for the (more common) IPv6 part of it.