[postgis-tickets] r17621 - Support for srid reprojection when using -D switch

[Sandro Santilli]

Great to see this coming Regina, just some old-style peer-review :)

On Sun, Jul 28, 2019 at 02:14:48AM +0000, Regina Obe wrote:
> Author: robe
> Date: 2019-07-28 14:14:48 -0700 (Sun, 28 Jul 2019)
> New Revision: 17621

[...]


> @@ -1090,10 +1091,11 @@
>  	state->precisions = malloc(state->num_fields * sizeof(int));
>  	state->pgfieldtypes = malloc(state->num_fields * sizeof(char *));
>  	state->col_names = malloc((state->num_fields + 2) * sizeof(char) * MAXFIELDNAMELEN);
> +	state->col_names_no_paren = malloc((state->num_fields + 2) * sizeof(char) * MAXFIELDNAMELEN);

The size above is suspicious.
We're not accounting for the size of parens.
I understand what you're using is smaller (no paren)
but I noticed the discrepancy of having the same size.
Maybe using a "size" variable to hold that info ?
Or, even better, using a single "col_names" variable,
without the parens, and use the parens only when
needed ?

> +	strcpy(state->col_names_no_paren, "" );

I'm afraid the above is undefined behavior, when
state->col_names_no_paren was just allocated and
never initialized. This was the case before this
change as well, but worth mentioning.

> +	/* Generate a string of comma separated column names of the form "col1, col2 ... colN" for the SQL
>  	   insertion string */

Could we please try to stick within 80 cols ?

> +	/**If we are in dump mode and a transform was asked for need to create a temp table to store original data
> +	 You may ask, why don't we go straight into the main table and then do an alter table alter column afterwards
> +	 Main reason is so we don't incur the penalty of WAL logging when we change the typmod in final run. **/

Could we please try to stick within 80 cols ?

> +	stringbuffer_t *sb;
> +	sb = stringbuffer_create();
> +	stringbuffer_clear(sb);

Is "_clear" really needed right after "_create" ?

> +		/* Copy the string buffer into a new string, destroying the string buffer */
> +		ret = (char *)malloc(strlen((char *)stringbuffer_getstring(sb)) + 1);
> +		strcpy(ret, (char *)stringbuffer_getstring(sb));

Maybe you want to use this instead ?

 char *stringbuffer_getstringcopy(stringbuffer_t *sb);

> +		stringbuffer_aprintf(sb, "\"%s\" %s ", state->config->table, state->col_names);
> +		stringbuffer_aprintf(sb, "SELECT %s FROM \"pgis_tmp_%s\";\n", state->col_names_no_paren, state->config->table );

Note you can 

> Modified: trunk/loader/shp2pgsql-core.h
> ===================================================================
> --- trunk/loader/shp2pgsql-core.h	2019-07-22 13:27:46 UTC (rev 17620)
> +++ trunk/loader/shp2pgsql-core.h	2019-07-28 21:14:48 UTC (rev 17621)
> @@ -192,6 +192,9 @@
>  	/* String containing colume name list in the form "(col1, col2, col3 ... , colN)" */
>  	char *col_names;
>  
> +	/* String containing colume name list in the form "col1, col2, col3 ... , colN" */
> +	char *col_names_no_paren;
> +

Ideally we'd drop "col_names_no_paren" completely, but I guess
those variables do not really need to be in the header file
and could instead be declared module-static  in the .c file
(static char *col_names)

--strk;