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.