Merge branch 'master' into 103-Extension_Group_API

Makefile

@@ -1,7 +1,7 @@
 # cartodb/Makefile
 
 EXTENSION = cartodb
-EXTVERSION = 0.10.0
+EXTVERSION = 0.10.1
 
 SED = sed
 
@@ -48,6 +48,7 @@ UPGRADABLE = \
   0.9.3 \
   0.9.4 \
   0.10.0 \
+  0.10.1 \
   $(EXTVERSION)dev \
   $(EXTVERSION)next \
   $(END)

NEWS.md

@@ -2,6 +2,14 @@ next (2015-mm-dd)
 -----------------
 * Groups API
 
+0.10.1 (2015-09-16)
+-------------------
+* Get back the `update_updated_at` function (still used by old tables) [#143](https://github.com/CartoDB/cartodb-postgresql/pull/143)
+* Fix for CDB_StatsTest.sql test failing randomly [#144](https://github.com/CartoDB/cartodb-postgresql/issues/144)
+* Fix for table cartodbfy'ed without default seq value [#138](https://github.com/CartoDB/cartodb-postgresql/issues/138)
+* Fix for cartodbfy error column `the_geom` already exists [#141](https://github.com/CartoDB/cartodb-postgresql/issues/141)
+* Fix for columns with geometry cartodbfy'ed without SRID [#154](https://github.com/CartoDB/cartodb-postgresql/issues/154)
+
 0.10.0 (2015-09-07)
 -----------------
 * Quote schema and table names returned by CDB_QueryTables [#134](https://github.com/CartoDB/cartodb-postgresql/pull/134). Use quote_ident to quote schema and table names when necessary.

expected/test_ddl_triggers.out

@@ -91,6 +91,7 @@ select pg_sleep(.1);
 (1 row)
 
 alter table c.t3 rename column the_geom_webmercator to webmerc;
+NOTICE:  column "the_geom_webmercator" of relation "t3" does not exist, skipping
 NOTICE:  event trigger "cdb_on_relation_create" does not exist, skipping
 NOTICE:  event trigger "cdb_on_relation_drop" does not exist, skipping
 NOTICE:  event trigger "cdb_on_alter_column" does not exist, skipping
@@ -115,6 +116,7 @@ select pg_sleep(.1);
 (1 row)
 
 alter table c.t3 rename column the_geom_webmercator to webmerc2;
+NOTICE:  column "the_geom_webmercator" of relation "t3" does not exist, skipping
 NOTICE:  event trigger "cdb_on_relation_create" does not exist, skipping
 NOTICE:  event trigger "cdb_on_relation_drop" does not exist, skipping
 NOTICE:  event trigger "cdb_on_alter_column" does not exist, skipping

scripts-available/CDB_CartodbfyTable.sql

@@ -280,6 +280,16 @@ BEGIN
 END;
 $$ LANGUAGE plpgsql VOLATILE;
 
+--- Trigger to update the updated_at column. No longer added by default
+--- but kept here for compatibility with old tables which still have this behavior
+--- and have it added
+CREATE OR REPLACE FUNCTION _CDB_update_updated_at()
+  RETURNS TRIGGER AS $$
+BEGIN
+   NEW.updated_at := now();
+   RETURN NEW;
+END;
+$$ LANGUAGE plpgsql VOLATILE;
 
 -- Auxiliary function
 CREATE OR REPLACE FUNCTION cartodb._CDB_is_raster_table(schema_name TEXT, reloid REGCLASS)
@@ -499,30 +509,6 @@ END;
 $$ LANGUAGE 'plpgsql';
 
 
--- Return the geometry SRID from the column metadata or
--- the geometry of the very first entry in a given column.
-CREATE OR REPLACE FUNCTION _CDB_Geometry_SRID(reloid REGCLASS, columnname TEXT)
-RETURNS INTEGER
-AS $$
-DECLARE
-  rec RECORD;
-BEGIN
-
-  RAISE DEBUG 'CDB(%): %', '_CDB_Geometry_SRID', 'entered function';
-  
-  EXECUTE Format('SELECT ST_SRID(%I) AS srid FROM %s LIMIT 1', columnname, reloid::text)
-  INTO rec;
-
-  IF FOUND THEN 
-    RETURN rec.srid;
-  END IF;
-
-  RETURN 0;
-  
-END;
-$$ LANGUAGE 'plpgsql';
-
-
 -- Find out if the table already has a usable primary key
 -- If the table has both a usable key and usable geometry
 -- we can no-op on the table copy and just ensure that the 
@@ -654,9 +640,68 @@ END;
 $$ LANGUAGE 'plpgsql';
 
 
-DROP FUNCTION IF EXISTS _CDB_Has_Usable_Geom(regclass);
+CREATE OR REPLACE FUNCTION _CDB_Has_Usable_PK_Sequence(reloid REGCLASS)
+RETURNS BOOLEAN
+AS $$
+DECLARE
+  seq TEXT;
+  const RECORD;
+  has_sequence BOOLEAN = false;
+BEGIN
+
+  const := _CDB_Columns();
+
+  SELECT pg_get_serial_sequence(reloid::text, const.pkey)
+  INTO STRICT seq;
+  has_sequence := seq IS NOT NULL;
+
+  RETURN has_sequence;
+END;
+$$ LANGUAGE 'plpgsql';
+
+-- Return a set of columns that can be candidates to be the_geom[webmercator]
+-- with some extra information to analyze them.
+CREATE OR REPLACE FUNCTION _cdb_geom_candidate_columns(reloid REGCLASS)
+RETURNS TABLE (attname name, srid integer, typname name, desired_attname text, desired_srid integer)
+AS $$
+DECLARE
+  const RECORD;
+BEGIN
+
+  const := _CDB_Columns();
+
+  RETURN QUERY
+    SELECT
+    a.attname,
+    CASE WHEN t.typname = 'geometry' THEN postgis_typmod_srid(a.atttypmod) ELSE NULL END AS srid,
+    t.typname,
+    f.desired_attname, f.desired_srid
+    FROM pg_class c
+    JOIN pg_attribute a ON a.attrelid = c.oid
+    JOIN pg_type t ON a.atttypid = t.oid,
+    (VALUES (const.geomcol, 4326), (const.mercgeomcol, 3857) ) as f(desired_attname, desired_srid)
+    WHERE c.oid = reloid
+    AND a.attnum > 0
+    AND NOT a.attisdropped
+    AND postgis_typmod_srid(a.atttypmod) IN (4326, 3857, 0)
+    ORDER BY t.oid ASC;
+END;
+$$ LANGUAGE 'plpgsql';
+
+
+CREATE TYPE _cdb_has_usable_geom_record
+  AS (has_usable_geoms boolean,
+      text_geom_column boolean,
+      text_geom_column_name text,
+      text_geom_column_srid boolean,
+      has_geom boolean,
+      has_geom_name text,
+      has_mercgeom boolean,
+      has_mercgeom_name text);
+
+DROP FUNCTION IF EXISTS _CDB_Has_Usable_Geom(REGCLASS);
 CREATE OR REPLACE FUNCTION _CDB_Has_Usable_Geom(reloid REGCLASS)
-RETURNS RECORD
+RETURNS _cdb_has_usable_geom_record
 AS $$
 DECLARE
   r1 RECORD;
@@ -688,20 +733,7 @@ BEGIN
 
   -- Do we have a column we can use?
   FOR r1 IN
-    SELECT 
+    SELECT * FROM _cdb_geom_candidate_columns(reloid)
-    a.attname, 
-    CASE WHEN t.typname = 'geometry' THEN postgis_typmod_srid(a.atttypmod) ELSE NULL END AS srid,
-    t.typname,
-    f.desired_attname, f.desired_srid
-    FROM pg_class c 
-    JOIN pg_attribute a ON a.attrelid = c.oid 
-    JOIN pg_type t ON a.atttypid = t.oid,
-    (VALUES (const.geomcol, 4326), (const.mercgeomcol, 3857) ) as f(desired_attname, desired_srid)
-    WHERE c.oid = reloid
-    AND a.attnum > 0
-    AND NOT a.attisdropped
-    AND postgis_typmod_srid(a.atttypmod) IN (4326, 3857, 0)
-    ORDER BY t.oid ASC
   LOOP
   
     RAISE DEBUG 'CDB(_CDB_Has_Usable_Geom): %', Format('checking column ''%s''', r1.attname);
@@ -756,7 +788,7 @@ BEGIN
 
       -- If it's the right SRID, we can use it in place without
       -- transforming it!
-      IF r1.srid = r1.desired_srid OR _CDB_Geometry_SRID(reloid, r1.attname) = r1.desired_srid THEN
+      IF r1.srid = r1.desired_srid THEN
 
         RAISE DEBUG 'CDB(_CDB_Has_Usable_Geom): %', Format('found acceptable ''%s''', r1.attname);
 
@@ -769,7 +801,7 @@ BEGIN
         END IF;
         
       -- If it's an unknown SRID, we need to know that too
-      ELSIF r1.srid = 0 OR _CDB_Geometry_SRID(reloid, r1.attname) = 0 THEN
+      ELSIF r1.srid = 0 THEN
         
         -- Unknown SRID, we'll have to fill it in later
         text_geom_column_srid := true;
@@ -780,23 +812,13 @@ BEGIN
     
   END LOOP;
 
-  -- If geom is the wrong name, just rename it.
-  IF has_geom AND has_geom_name != const.geomcol THEN  
-    sql := Format('ALTER TABLE %s RENAME COLUMN %s TO %s', reloid::text, has_geom_name, const.geomcol);
-    PERFORM _CDB_SQL(sql,'_CDB_Has_Usable_Geom');
-  END IF;
-
-  -- If mercgeom is the wrong name, just rename it.
-  IF has_mercgeom AND has_mercgeom_name != const.mercgeomcol THEN  
-    sql := Format('ALTER TABLE %s RENAME COLUMN %s TO %s', reloid::text, has_mercgeom_name, const.mercgeomcol);
-    PERFORM _CDB_SQL(sql,'_CDB_Has_Usable_Geom');
-  END IF;
-  
   SELECT 
     -- If table is perfect (no transforms required), return TRUE!
     has_geom AND has_mercgeom AS has_usable_geoms,
     -- If the geometry column is hiding in a text field, return enough info to deal w/ it.
-    text_geom_column, text_geom_column_name, text_geom_column_srid
+    text_geom_column, text_geom_column_name, text_geom_column_srid,
+    -- Return enough info to rename geom columns if needed
+    has_geom, has_geom_name, has_mercgeom, has_mercgeom_name
     INTO rv;
 
   RAISE DEBUG 'CDB(_CDB_Has_Usable_Geom): %', Format('returning %s', rv);
@@ -806,6 +828,7 @@ BEGIN
 END;
 $$ LANGUAGE 'plpgsql';
 
+
 -- Create a copy of the table. Assumes that the "Has usable" functions
 -- have already been run, so that if there is a 'cartodb_id' column, it is
 -- a "good" one, and the same for the geometry columns. If all the required
@@ -840,6 +863,7 @@ DECLARE
   geom_srid INTEGER;
   
   has_usable_primary_key BOOLEAN;
+  has_usable_pk_sequence BOOLEAN;
   
 BEGIN
 
@@ -867,18 +891,39 @@ BEGIN
 
   RAISE DEBUG 'CDB(_CDB_Rewrite_Table): has_usable_primary_key %', has_usable_primary_key;
 
+  -- See if the candidate primary key column has a sequence for default
+  -- values. No usable pk implies has_usable_pk_sequence = false.
+  has_usable_pk_sequence := false;
+  IF has_usable_primary_key THEN
+    SELECT _CDB_Has_Usable_PK_Sequence(reloid)
+    INTO STRICT has_usable_pk_sequence;
+  END IF;
+
   -- See if the geometry columns we need are already available
   -- on the table. If they are, we don't need to do any bulk
   -- transformation of the table, we can just ensure proper
   -- indexes are in place and apply a rename
   SELECT *
   FROM _CDB_Has_Usable_Geom(reloid) 
-  AS (has_usable_geoms boolean, 
-      text_geom_column boolean, 
-      text_geom_column_name text, 
-      text_geom_column_srid boolean)
   INTO STRICT gc;
 
+  -- If geom is the wrong name, just rename it.
+  IF gc.has_geom AND gc.has_geom_name != const.geomcol THEN
+    sql := Format('ALTER TABLE %s DROP COLUMN IF EXISTS %I', reloid::text, const.geomcol);
+    PERFORM _CDB_SQL(sql,'_CDB_Rewrite_Table');
+    sql := Format('ALTER TABLE %s RENAME COLUMN %I TO %I', reloid::text, gc.has_geom_name, const.geomcol);
+    PERFORM _CDB_SQL(sql,'_CDB_Rewrite_Table');
+  END IF;
+
+  -- If mercgeom is the wrong name, just rename it.
+  IF gc.has_mercgeom AND gc.has_mercgeom_name != const.mercgeomcol THEN
+    sql := Format('ALTER TABLE %s DROP COLUMN IF EXISTS %I', reloid::text, const.mercgeomcol);
+    PERFORM _CDB_SQL(sql,'_CDB_Rewrite_Table');
+    sql := Format('ALTER TABLE %s RENAME COLUMN %I TO %I', reloid::text, gc.has_mercgeom_name, const.mercgeomcol);
+    PERFORM _CDB_SQL(sql,'_CDB_Rewrite_Table');
+  END IF;
+
+
   RAISE DEBUG 'CDB(_CDB_Rewrite_Table): has_usable_geoms %', gc.has_usable_geoms;
 
   -- We can only avoid a rewrite if both the key and 
@@ -886,16 +931,18 @@ BEGIN
 
   -- No table re-write is required, BUT a rename is required to
   -- a destination schema, so do that now
-  IF has_usable_primary_key AND gc.has_usable_geoms AND destschema != relschema THEN
+  IF has_usable_primary_key AND has_usable_pk_sequence AND gc.has_usable_geoms THEN
+    IF  destschema != relschema THEN
 
-    RAISE DEBUG 'CDB(_CDB_Rewrite_Table): perfect table needs to be moved to schema (%)', destschema;
+      RAISE DEBUG 'CDB(_CDB_Rewrite_Table): perfect table needs to be moved to schema (%)', destschema;
-    PERFORM _CDB_SQL(Format('ALTER TABLE %s SET SCHEMA %I', reloid::text, destschema), '_CDB_Rewrite_Table');
+      PERFORM _CDB_SQL(Format('ALTER TABLE %s SET SCHEMA %I', reloid::text, destschema), '_CDB_Rewrite_Table');
-    RETURN true;
 
-  -- Don't move anything, just make sure our destination information is set right
+    ELSE
-  ELSIF has_usable_primary_key AND gc.has_usable_geoms AND destschema = relschema THEN
+
+      RAISE DEBUG 'CDB(_CDB_Rewrite_Table): perfect table in the perfect place';
+
+    END IF;
 
-    RAISE DEBUG 'CDB(_CDB_Rewrite_Table): perfect table in the perfect place';
     RETURN true;
 
   END IF;
@@ -1029,15 +1076,11 @@ BEGIN
       )
       SELECT ', ST_Transform(' 
             || t.missing_srid_start || t.attname || t.missing_srid_end
-            || ',4326)::Geometry(' 
+            || ',4326)::Geometry(GEOMETRY,4326) AS '
-            || t.geomtype 
-            || ',4326) AS '
             || const.geomcol
             || ', cartodb.CDB_TransformToWebmercator(' 
             || t.missing_srid_start || t.attname || t.missing_srid_end
-            || ')::Geometry('
+            || ')::Geometry(GEOMETRY,3857) AS '
-            || t.geomtype
-            || ',3857) AS '
             || const.mercgeomcol,
             t.attname
       INTO geom_transform_sql, geom_column_source
@@ -1094,19 +1137,15 @@ BEGIN
   -- Set up the primary key sequence
   -- If we copied the primary key from the original data, we need
   -- to set the sequence to the maximum value of that key
-  IF has_usable_primary_key THEN
+  EXECUTE Format('SELECT max(%s) FROM %s',
-
+          const.pkey, copyname)
-    EXECUTE Format('SELECT max(%s) FROM %s',
+     INTO destseqmax;
-            const.pkey, copyname)
-       INTO destseqmax;
-
-    IF FOUND AND destseqmax IS NOT NULL THEN
-      PERFORM _CDB_SQL(Format('SELECT setval(''%s'', %s)', destseq, destseqmax), '_CDB_Rewrite_Table');
-    END IF;
 
+  IF destseqmax IS NOT NULL THEN
+    PERFORM _CDB_SQL(Format('SELECT setval(''%s'', %s)', destseq, destseqmax), '_CDB_Rewrite_Table');
   END IF;
 
-  -- Make the primary key use the sequence as its default value
+   -- Make the primary key use the sequence as its default value
   sql := Format('ALTER TABLE %s ALTER COLUMN %s SET DEFAULT nextval(''%s'')',
           copyname, const.pkey, destseq);
   PERFORM _CDB_SQL(sql, '_CDB_Rewrite_Table');
@@ -1116,6 +1155,7 @@ BEGIN
   sql := Format('ALTER SEQUENCE %s OWNED BY %s.%s', destseq, copyname, const.pkey);
   PERFORM _CDB_SQL(sql,'_CDB_Rewrite_Table');
 
+  
   -- We just made a copy, so we can drop the original now
   sql := Format('DROP TABLE %s', reloid::text);
   PERFORM _CDB_SQL(sql, '_CDB_Rewrite_Table');

test/CDB_CartodbfyTableTest.sql

@@ -225,6 +225,63 @@ SELECT CDB_CartodbfyTable('original');
 DROP TABLE original_renamed;
 DROP TABLE original;
 
+-- Table always have a default seq value after cartodbfy #138
+CREATE TABLE bug_empty_table_no_seq (
+  cartodb_id integer,
+  the_geom geometry(Geometry,4326),
+  the_geom_webmercator geometry(Geometry,3857),
+  name text,
+  description text
+);
+SELECT CDB_CartodbfyTableCheck('bug_empty_table_no_seq', 'Table always have a default seq value after cartodbfy #138');
+INSERT INTO bug_empty_table_no_seq DEFAULT VALUES;
+DROP TABLE bug_empty_table_no_seq;
+
+-- Existing cartodb_id values are respected
+CREATE table existing_cartodb_id (
+  cartodb_id integer,
+  the_geom geometry(Geometry,4326),
+  the_geom_webmercator geometry(Geometry,3857),
+  name text,
+  description text
+);
+INSERT INTO existing_cartodb_id (cartodb_id, description) VALUES
+       (10, 'a'),
+       (20, 'b'),
+       (30, 'c');
+SELECT CDB_CartodbfyTableCheck('existing_cartodb_id', 'Existing cartodb_id values are respected #138');
+SELECT * from existing_cartodb_id;
+DROP TABLE existing_cartodb_id;
+
+-- Table with both the_geom and wkb_geometry
+CREATE TABLE many_geometry_columns (
+       the_geom geometry,
+       wkb_geometry geometry(MultiPoint,4326),
+       description varchar
+);
+INSERT INTO many_geometry_columns (the_geom, wkb_geometry) VALUES
+       ('0104000020E61000000100000001010000007108B023698052C03CEEA53A2E5D4440', '0104000020E61000000100000001010000007108B023698052C03CEEA53A2E5D4440'),
+       ('0104000020E6100000010000000101000000864C9E57618052C0994F0C7F3C5B4440', '0104000020E6100000010000000101000000864C9E57618052C0994F0C7F3C5B4440');
+SELECT CDB_CartodbfyTableCheck('many_geometry_columns', 'Table with both the_geom and wkb_geometry #141');
+SELECT * FROM many_geometry_columns;
+DROP TABLE many_geometry_columns;
+
+-- Many colliding geom columns
+CREATE TABLE many_colliding_columns (
+  the_geom varchar,
+  the_geom_webmercator varchar,
+  my_geom geometry,
+  my_mercgeom geometry(Point, 3857),
+  cartodb_id varchar,
+  my_pk integer primary key
+);
+INSERT INTO many_colliding_columns VALUES (
+ 'foo', 'bar', 'SRID=4326;POINT(0 0)', 'SRID=3857;POINT(0 0)', 'nerf', 1
+);
+SELECT CDB_CartodbfyTableCheck('many_colliding_columns', 'Many colliding columns #141');
+DROP TABLE many_colliding_columns;
+
+
 -- TODO: table with existing custom-triggered the_geom
 
 DROP FUNCTION CDB_CartodbfyTableCheck(regclass, text);

test/CDB_CartodbfyTableTest_expect

@@ -58,5 +58,26 @@ CREATE TABLE
 original
 DROP TABLE
 DROP TABLE
+CREATE TABLE
+Table always have a default seq value after cartodbfy #138 cartodbfied fine
+INSERT 0 1
+DROP TABLE
+CREATE TABLE
+INSERT 0 3
+Existing cartodb_id values are respected #138 cartodbfied fine
+10|||a|
+20|||b|
+30|||c|
+DROP TABLE
+CREATE TABLE
+INSERT 0 2
+Table with both the_geom and wkb_geometry #141 cartodbfied fine
+1|0104000020E61000000100000001010000007108B023698052C03CEEA53A2E5D4440|0104000020110F00000100000001010000004A9F662B456D5FC11392690DC3F75241|
+2|0104000020E6100000010000000101000000864C9E57618052C0994F0C7F3C5B4440|0104000020110F00000100000001010000002858E0EC376D5FC1CAE8DB4B95F55241|
+DROP TABLE
+CREATE TABLE
+INSERT 0 1
+Many colliding columns #141 cartodbfied fine
+DROP TABLE
 DROP FUNCTION
 DROP FUNCTION

test/CDB_StatsTest.sql

@@ -2,15 +2,12 @@
 -- http://mathworld.wolfram.com/UniformDistribution.html
 set client_min_messages to ERROR;
 
-With dist As (
+WITH dist AS (
-  SELECT random()::numeric As val
+  SELECT generate_series(0,10000)::numeric / 10000.0 i
-  FROM generate_series(1,50000) t
 )
-
 SELECT
-  -- does random dist values match within 1% of known values
+  abs(CDB_Kurtosis(array_agg(i)) + 1.2) < 1e-3 AS kurtosis,
-  abs(CDB_Kurtosis(array_agg(val)) + 1.20) < 1e-2 As kurtosis,
+  abs(CDB_Skewness(array_agg(i))) < 1e-3 AS skewness
-  abs(CDB_Skewness(array_agg(val)) - 0) < 1e-2 As skewness
 FROM dist;
 
 set client_min_messages to NOTICE;

test/extension/test.sh

@@ -123,7 +123,7 @@ function create_role_and_schema() {
 
 function drop_role_and_schema() {
     local ROLE=$1
-    sql "DROP SCHEMA \"${ROLE}\";"
+    sql "DROP SCHEMA \"${ROLE}\" CASCADE;"
     sql "REVOKE CONNECT ON DATABASE \"${DATABASE}\" FROM \"${ROLE}\";"
     sql "DROP ROLE \"${ROLE}\";"
 }
@@ -210,8 +210,8 @@ function tear_down() {
     sql "DROP SCHEMA cartodb CASCADE"
 
     log_info "########################### TEAR DOWN ###########################"
-    sql 'DROP SCHEMA cdb_testmember_1;'
+    sql 'DROP SCHEMA cdb_testmember_1 CASCADE;'
-    sql 'DROP SCHEMA cdb_testmember_2;'
+    sql 'DROP SCHEMA cdb_testmember_2 CASCADE;'
 
     sql "REVOKE CONNECT ON DATABASE \"${DATABASE}\" FROM cdb_testmember_1;"
     sql "REVOKE CONNECT ON DATABASE \"${DATABASE}\" FROM cdb_testmember_2;"