ST_AsMVT crash with OVER(ORDER BY random())

[robe]

SELECT ST_AsMVT(foo1)OVER(ORDER BY random()) As result FROM ((SELECT ST_SetSRID(ST_Point(i,j),4326) As the_geom FROM (SELECT a*1.11111111 FROM generate_series(-10,50,2) As a) As i(i) CROSS JOIN generate_series(40,70, 5) j ORDER BY i,j )) As foo1 LIMIT 10;

note that this doesn't crash:

SELECT ST_AsMVT(foo1)OVER() As result FROM ((SELECT ST_SetSRID(ST_Point(i,j),4326) As the_geom FROM (SELECT a*1.11111111 FROM generate_series(-10,50,2) As a) As i(i) CROSS JOIN generate_series(40,70, 5) j ORDER BY i,j )) As foo1 LIMIT 10;

[pramsey]

So, smaller more concrete example.

DROP TABLE foo;
CREATE TABLE foo AS
SELECT *
FROM (
VALUES 
 ('POINT(0 0)'::geometry, 'one', 1)
,('POINT(1 1)'::geometry, 'two', 2)
) AS v(g,b,i);

-- These all crash
SELECT ST_AsMVT(foo) OVER(ORDER BY random() DESC) AS result FROM foo;
SELECT ST_AsMVT(foo) OVER(ORDER BY i DESC) AS result FROM foo;
SELECT ST_AsMVT(foo) OVER(ORDER BY i) AS result FROM foo;

[pramsey]

Trying to take a few notes. I do not have it 100% pinned down, but it comes down to this:

The code is written with an expectation of an aggregate function, which is that when the finalizefn is called you are DONE. But that expectation is actually WRONG. When called in a window context, a function is expected to finalize and emit its CURRENT STATE but remain available to keep adding to that state. So finalizefn can be called on a given context MULTIPLE TIMES.

The MVT code currently takes a TupleDesc from PgSQL, which it needs to release to avoid leaking into the PgSQL cache. However, simply keeping that TupleDesc around does not seem sufficient to avoid the windowing problem, since we get a different crash if we just keep it around. So the finalize logic seems to depend to some degree on cleaning up the context. This is the unknown part of the problem.

Unfortunately this is all made even more complex by the fact that the ST_AsMVT function is parallelizeable so it can convert a context into an intermediate serialization (an MVT, as it happens) which can then be combined before the final final finalization. So the lifecycle of this stuff is really complex.

[robe]

sounds icky. I wonder if bjorn's attempt at fixing the window crash for ST_AsFlatGeoBuf triggered the crash in #5005 and testing your example it doesn't crash but gives a strange error.

[pramsey]

RhodiumToad (of course) notes that we have an easy out: FINALFUNC_MODIFY = READ_WRITE on our aggregate will disallow this multiple-finalize behaviour, at the price of not being able to use the aggregate in a window context.

Strk notes that we can add this clause in the upgrade script as well.

[pramsey]

Should be fixed (aka windowing disabled) by using the finalfunc_modify = read_only setting, as of b02b7bf7b64337665e2e824d15e71f2d2c02734e