lwcollection_extract return object possibly referencing portions of argument

[strk]

Like with #3245, lwcollection_extract is another function that might return objects that depend on memory owned by its arguments. It uses lwgeom_clone over the input components, rather than lwgeom_clone_deep.

Probably since 2.0.

[pramsey]

So, the question is kind of whether we feel OK having those references or not. It's certainly a lot lighter weight to do them, and I confirmed that, in ordinary usage, shallow cloning works OK:

static void test_clone(void)
{
	static char *wkt = "GEOMETRYCOLLECTION(MULTIPOLYGON(((0 0, 10 0, 10 10, 0 10, 0 0))),POINT(1 1),LINESTRING(2 3,4 5))";
	LWGEOM *geom1 = lwgeom_from_wkt(wkt, LW_PARSER_CHECK_ALL);
	LWGEOM *geom2;
	
	/* Free in "backwards" order */
	geom2 = lwgeom_clone(geom1);
	lwgeom_free(geom1);
	lwgeom_free(geom2);

	/* Free in "forewards" order */
	geom1 = lwgeom_from_wkt(wkt, LW_PARSER_CHECK_ALL);
	geom2 = lwgeom_clone(geom1);
	lwgeom_free(geom2);
	lwgeom_free(geom1);
}

Mind you, freeing the input and then *using* the output would fail, but in terms of freeing, the read-only guard works fine, there's no doublefree condition. Question is basically if we want to apply a "mutation implies copy" policy over the whole codebase or not, even when the mutation doesn't actually affect the coordinates (the biggest overhead of our memory use).

[robe]

Definitely not something we want to see a change in 2.1.

FWIW: I'm all for keeping shallow cloning particularly where changing to deep could cause a performance degradation.

[dbaston]

Resolve this by adding a comment to the source explaining that return references portions of argument.

[pramsey]

I've noted in the code that the return value references portions of the argument. r15643

[robe]

not to be picky here but it looks like you did a little more than just put a comment in here. You changed code. Not sure what to say in the NEWS about this but feels like something needs to be said.

[strk]

Yup, sounds like a leak was added ?

[pramsey]

Well yes, looking at the function, once you start having an eye for the kinds of leaks that hang around the whole lwgeom_release() pattern, you see some others, so I removed them since I was there. Like most of our leaks, they are leaking into the pgsql bathtub, which is drained at the end of the function call anyways It's not something users would ever perceive.

[strk]

Chances are the memory will be released at the end of *statment*, not function call. May grow quick.