Avoid lost work/credits when an exception happens

[rafatower]

The scenario we want to improve is:

1. An analysis is added to a dashboard
2. That triggers a query on all the rows of a dataset through the Batch SQL API. E.g: https://github.com/CartoDB/camshaft/blob/master/lib/node/nodes/georeference-street-address.js#L59-L64
3. Calls are executing one by one on the Dataservices API server side, because of the way pl/proxy works
4. Some of those calls can fail. There are a few causes for that: quota exhausted, programming error, network issues.
5. That produces an exception/error that rollbacks the whole transaction in the user's DB, wasting any work done and potentially consuming credits.

Possible solutions discussed internally:

a. Wrap all public functions in an _exception_safe version that returns NULL instead of bubbling the exception to the client.
b. Write table-level functions for services consuming quota: isolines, hires_geocoder, routing, observatory.

cc'ing @rochoa for discussion.

[rochoa]

I think we already talk about scenario b. a while ago, however, we didn't dig deeper into it.

About option a., I guess it depends if you, as an end user, want to avoid null values, and don't want a do an analysis with half of your dataset as null entries.

[rafatower]

(cc'ing @javisantana to let him know about your concerns, Raul)

Another option is to return warnings instead of errors, which can later be checked.

Anyway, any option different from just returning null, such as adding an extra column status or something like that, would put the responsibility of managing that information in the client (i.e. do some "error handling" with the status or the warnings).

IMHO we should seek for a cost-effective solution right now and discuss what we'd like to have in the interfaces in the near future.

[rafatower]

We're going for (a) and make all the necessary changes, as it is the fastest path. If that doesn't work or is not enough, and since we're keeping existing functions and adding new ones, we can always rollback or implement (b).

[javisantana]

There were some corncerns from @ethervoid about being about to implement the "exception safe" version of geocoder (and other DS) so, did we already research if that's possible?

[rafatower]

I'll write a PoC (which shall be quick) and ask Mario about his concerns.

In pseudo-code, it should be something like this:

BEGIN
    RETURN original_function(params);
EXCEPTION
    WHEN OTHERS THEN
        RAISE WARNING 'whatever';
        RETURN NULL;
END;

and we already got the template engine to produce those functions out of the description of current API:
https://github.com/CartoDB/dataservices-api/blob/development/client/renderer/interface.yaml
https://github.com/CartoDB/dataservices-api/tree/development/client/renderer/templates

For the reference: https://www.postgresql.org/docs/9.5/static/plpgsql-control-structures.html#PLPGSQL-ERROR-TRAPPING

[rafatower]

I wrote a small PoC based on cdb_geocode_street_point_exception_safe (which is actually generated out of this template:

CREATE OR REPLACE FUNCTION cdb_dataservices_client.cdb_geocode_street_point_exception_safe(searchtext text, city text DEFAULT NULL::text, state_province text DEFAULT NULL::text, country text DEFAULT NULL::text)
RETURNS geometry
AS $$
DECLARE
  ret Geometry;
  username text;
  orgname text;
BEGIN
  IF session_user = 'publicuser' OR session_user ~ 'cartodb_publicuser_*' THEN
    RAISE EXCEPTION 'The api_key must be provided';
  END IF;
  SELECT u, o INTO username, orgname FROM cdb_dataservices_client._cdb_entity_config() AS (u text, o text);
  -- JSON value stored "" is taken as literal
  IF username IS NULL OR username = '' OR username = '""' THEN
    RAISE EXCEPTION 'Username is a mandatory argument, check it out';
  END IF;

  BEGIN
    SELECT cdb_dataservices_client._cdb_geocode_street_point(username, orgname, searchtext, city, state_province, country) INTO ret;
    RETURN ret;
  EXCEPTION
    WHEN OTHERS THEN
      RAISE WARNING 'an exception happened';
      RETURN NULL;
  END;

END;
$$ LANGUAGE plpgsql SECURITY DEFINER;

and it seems to work ok:

SELECT * from  cdb_dataservices_client.cdb_service_quota_info() WHERE service = 'hires_geocoder';
    service     | monthly_quota | used_quota | soft_limit | provider 
----------------+---------------+------------+------------+----------
 hires_geocoder |             5 |          5 | f          | mapzen
(1 row)

-- An exception is to be expected here
SELECT cdb_geocode_street_point('651 Lombard Street', 'San Francisco', 'California', 'United States');
ERROR:  cdb_dataservices_client._cdb_geocode_street_point(6): [server] REMOTE ERROR: spiexceptions.ExternalRoutineException: Exception: You have reached the limit of your quota
CONTEXT:  Remote context: Traceback (most recent call last):
  PL/Python function "cdb_geocode_street_point", line 21, in <module>
    return plpy.execute(mapzen_plan, [username, orgname, searchtext, city, state_province, country], 1)[0]['point']
PL/Python function "cdb_geocode_street_point"
SQL statement "SELECT cdb_dataservices_client._cdb_geocode_street_point(username, orgname, searchtext, city, state_province, country)"
PL/pgSQL function cdb_geocode_street_point(text,text,text,text) line 16 at SQL statement

-- But here we get a NULL in it's _exception_safe counterpart
SELECT cdb_geocode_street_point_exception_safe('651 Lombard Street', 'San Francisco', 'California', 'United States');
WARNING:  an exception happened
 cdb_geocode_street_point_exception_safe 
-----------------------------------------
 
(1 row)

What I'm going to do:

• run a few more tests to make sure the approach works
• try to gather a bit more information for the warning (GET STACKED DIAGNOSTICS) and add it if it makes sense.
• write the corresponding templates to generate these functions automatically.

[rafatower]

I made some more tests and found out that returning NULL can be troublesome in some cases (particularly when returning SETOF type):

WARNING:  whatever
ERROR:  structure of query does not match function result type
DETAIL:  Returned type record does not match expected type geometry(Geometry,4326) in column 1.
CONTEXT:  PL/pgSQL function cdb_isochrone(geometry,text,integer[],text[]) line 22 at RETURN QUERY

In that case I found it more practical to just not return anything explicitly. E.g: for isochrones:

CREATE OR REPLACE FUNCTION cdb_dataservices_client.cdb_isochrone(source geometry, mode text, range integer[], options text[] DEFAULT ARRAY[]::text[])
 RETURNS SETOF cdb_dataservices_client.isoline

--...

  BEGIN
    RETURN QUERY
    SELECT * FROM cdb_dataservices_client._cdb_isochrone(username, orgname, source, mode, range, options);
  EXCEPTION
    WHEN OTHERS THEN
      RAISE WARNING 'whatever';
  END;