Added drizzle_return_error_code and drizzle_no_log_error_codes directives

[ghost]

Hi,

I've added the mentioned two directives and comments to the module. These allow you to return the drizzle error code rather than 500 when there are errors (e.g. so you can process them inside ngx_lua), and allows you to suppress logging for specific error codes (e.g. 1062 for duplicate keys on inserts).

Cheers,

Marcus.

[PiotrSikora]

Hey Marcus,
I've got mixed feelings about this:

1. MySQL error codes (1000+) are outside of the HTTP error codes space (100-5xx), so this might caught people off-guard,
2. Why do you need to return them as HTTP status codes in the first place? Database error codes are already returned in the RDS and JSON in the appropriate fields, so you should be able to extract them on the client side without much trouble.

[ghost]

Hi,

In response, I'll do them in reverse order :

2. (i.e. 1[2]) I agree that returning the response in JSON is much better with error code and error message. However, in my tests (using ngx_lua), I got 500 reponses for all errors, including the one I'm interested in (duplicate keys), with an empty body. This is useless. I was surprised, because I did expect the JSON error message and code to be returned.

If I missed something, then great. Do I need to change a setting somewhere to get it to return the error code and message?

Also, importantly though, is the logging issue. It appears that errors will be logged regardless. For most errors, of course you'd probably want this, but for duplicate keys I don't. I want to know why it fails, but not log the fact. If there's another way to turn off the logging of errors, too, then please let me know, but I didn't see anything in the code.

1. It is precisely because the error codes for databases fall outside the HTTP range that I did it that way. Anyone who's familiar with database codes and HTTP codes shouldn't be confused by the matter, and Nginx is already set up for defining custom error codes anyway. It's an option that is not enabled by default, so you'd have to turn it on (and if you do so, then you should be relatively unlikely to be caught off guard).

Overall, I would personally like the option of returning the error codes / messages as JSON (with the default being on, probably), with the option to also just return the code.

Marcus.

[PiotrSikora]

Hi,
it seems that I should have read whole test suite (ngx_rds_json/t/openresty.t) before commenting. It turns out that the error codes that I was referring to were returned using rds_json_ret in the nginx.conf and not by ngx_drizzle itself.

However, I still don't like returning values outside of the usual HTTP response codes range. There are at least few ways to solve this in more elegant way:

1. add rds_json_error_codes on|off to the ngx_rds_json module, so that it would extract error codes from the RDS (both: error code and error string are already there),
2. add $drizzle_error variable, so that you could use something like add_header X-Drizzle-Error $drizzle_error,
3. map specific errors to existing HTTP error codes in a way that makes sense (see: ngx_postgres/t/rewrites.t).

btw: I don't have anything against your logging changes.

[ghost]

Hi ya,

Yes - I'd skim-read the rds_json_ret stuff too, hence my surprise when I got no return at all from them.

1. I definitely agree with this one, but considering that ngx_drizzle, ngx_posgres etc may be used without ngx_rds_json (I know they're probably not now, but that may well change at some point), I feel that a per-rdbms option is also needed.

2. Yes, this makes good sense, $drizzle_error_code too, and putting the information in headers can certain work well.

3. Personally, I think this is crazy! Either leave the HTTP codes intact (as the default would be, which would return just 500 on error and universally understood), or return a number that has more meaning to drizzle (which the database-specific error codes do). There are simply far too many database error codes to map to HTTP codes, and it adds an extra level of complexity, and I think would make it even more confusing for developers.

I understand your point of not wanting to send responses outside the normal HTTP responses. I think it's inappropriate to send it out over the Internet with a non-standard code. However, I'm proposing the use of DB error codes soley for 'internal' use, and I'm thinking primarily with ngx_lua, but it could also with other internal solutions.

Given that there most definitely wouldn't be a conflict in the response codes with those of HTTP (and given that anyone that knows anything about the HTTP protocol is going to know that a response code of e.g. 1062 has nothing to do with HTTP), I don't really think that it'd be very confusing or a bad thing to do. It's efficient, and would need to be turned on by the developer anyway.

Cheers,

Marcus.

[PiotrSikora]

1. But the database-specific solution already exists... You're simply losing this information while converting RDS to JSON.
2. What exactly are you trying to achieve? You're talking only about duplicate keys here and I believe that returning 409 Conflict for such errors would be perfectly acceptable.

To summarize, I would prefer to see solution 1) or 2) instead of the proposed one.

[PiotrSikora]

btw: 410 Gone is currently returned for MYSQL_ER_NO_SUCH_TABLE errors ;)

[ghost]

1. The database-specific solution does not exist - at least not for drizzle. It just returns 500, regardless, and gives no error information. What I'm saying is that you should not assume that RDS JSON will be used, and so there should be a mechanism for providing the necessary information back to the client without relying on it.

2. I am only talking about duplicate keys so far, primarily because I've not yet looked through all the possible error codes to consider whether it might be useful to give them their own separate code to be handled in the client somewhere. 409 is a sensible code to return for duplicate keys, but it's quite possible that there may be other situations where knowing the specific return code is useful, and I don't see a problem with returning that code rather than a sort-of-comparable HTTP code (at least for internal use).

I think I would prefer to have the option of both 1, 2 and just returning the code as I've done (perhaps with the addition of putting the error message as the status message too - something I'm sure you don't find appealing :-P).

Yes, I'd noticed that 410 had already gone (no pun intended).

[PiotrSikora]

1. Hmm... I was under the impression that (at least in the native format) error information would be returned in the response body (in RDS format), but I didn't touch this code in over half year, so I might be wrong.

Anyway, since you clearly want to use this only internally, why are you insisting on sending this as the HTTP response code (which is breaking format of HTTP response defined in RFC2616)?

[ghost]

1. I've only started to look at the code for ngx_drizzle, so I may have missed something, but there is very little code after the checking of the error status (in the send_request() func), and it returned 500 immediately without sending back error info.

I think the error info should be returned in the response body somehow, perhaps emitting JSON directly or as plain text.

I am not insisting on sending it as an HTTP response code, but I am proposing that it be an optional way of retrieving the response code. I personally like the idea of sending it as an HTTP response code because (a) it's efficient, and (b) I think that given that the HTTP request is basically a wrapper for the database call, that it doesn't matter too much and has a certain elegance. The most important thing for me is to be able to retrieve the response code from Drizzle, and it seems to me that this is the most efficient way of doing so (because there's no overhead of creating then parsing JSON or text or whatever).

Oh, and I like to break rules when I see a good reason for it. ;-)

[agentzh]

I prefer an nginx output variable for this purpose :)

Now there's already an undocumented nginx variable $drizzle_thread_id for outputing the mysql thread id. And we usually use a custom response header to pass this information out.

[ghost]

Are you ok with the logging changes?

[agentzh]

I'm fine with the capability to suppress logging the drizzle error codes that the user specify :)

[agentzh]

Suppressing logging may not be sufficient for your purpose though. Maybe you'd also want those errors to be discarded in context of keepalive database connections? That is, for those drizzle error codes, also leave the connection intact such that they can be reused by other requests later. ngx_drizzle now already does something similar for the "table not exist" error: it will use the info log level for the error message and do not abandon the current mysql connection.

[ghost]

Ok. I hadn't thought about that yet. I've only done ngx_lua subrequests to MySQL thus far, but I will almost certainly have AJAX -> MySQL requests at some stage.

Do you think the return code should be 200, and then just leave it up to the client to parse the headers to check for errors?

[agentzh]

I think we should follow the traditional definition of HTTP status code, just as what Piotr says earlier :) We've already extended it to 410 Gone to mean the mysql table missing error and we can go a bit further with that but no amazing magic ;)

[ghost]

Well, the thing is that if the user has specified that certain error codes are to be ignored, then should we return an HTTP error code, or success code?

I have two thoughts on this:

1. Downgrade the 'server error' to a 'client error', and return a 4xx code
2. Return some sort of 'success' code (200 or otherwise) for server errors that are caught / ignored

Personally I think that option 1 is probably better here.

Looking through the MySQL error codes, though (I didn't get through all of them, just the first few hundred or so), I don't think there will be many of them that users will be interested in individually on a regular basis, so it might be OK to 'map' a few common DB error codes that users might want to ignore to HTTP ones.

If we just choose an HTTP code for key-already-exists situations, then I'm ok with that (see suggestions below). I think also by default, that key-already-exists errors shouldn't be logged. I think that in most situations where INSERTs fail because of this, it's because the user is choosing something like a name that already exists, and in general, you probably wouldn't want to log this.

We'll make the error code and message both available as variables.

Btw, I don't think that 410 is the best code to use for a MySQL table not existing. According to RFC 2616, the HTTP code means that a resource is no longer available (i.e. that it was available in the past, which isn't necessarily the case), and won't be in the future (which also may not be the case).

I think 404 makes much more sense, since it may or may not have existed in the past and may exist in the future. Even having a different 4xx number to existing ones, with no established meaning, makes more sense than 410 (if you're wanting to follow the meaning of HTTP codes).

To me what would make more sense would be to use the following return codes:

404 (Table does not exist)
409 (Conflict) - for errors that are 'ignored' or caught
410 (Gone) - key-already-exists situations, since the key was available in the past, but it isn't now and won't be in the future
500 - for uncaught server errors

I am, however, guessing though that you won't want to change the usage of 410 so as not to break existing setups. Assuming that this is the case, how about this:

409 (Conflict) - key-already-exists situations, since the key was available in the past, but it isn't now and won't be in the future
410 (Gone) - for table doesn't exist errors
430 (Input error) - for 'caught' errors
500 - for uncaught server errors

And we change the 'drizzle_ignore_error_codes' directive to something that indicates that it's providing an input error, e.g. 'drizzle_input_error_codes'.

Does that work for you guys?

[agentzh]

I do like your second suggestion for the status code mapping ;)

[ghost]

Ok.

I'll get that done soonish.

[lizhitao67116961]

nginx的drizzle中，获取(查询)多个列值时报错如下
*** glibc detected *** /root/work/nginx: double free or corruption (!prev): 0x000000000543a9e0 ***
ngx_http_upstream_drizzle_send_query--》ngx_http_upstream_drizzle_recv_cols

在ngx_http_upstream_drizzle_recv_cols函数中调用drizzle_column_free报错！两次释放同一地址。

[agentzh]

@lizhitao67116961 please do not post Chinese text on all my GitHub issues boards. I'd make GitHub a pure English place for my projects.

You can join the openresty (Chinese) mailing list and discuss the issue in Chinese there: https://groups.google.com/group/openresty

Thank you for your understanding and cooperation.