sql: st_simplifypreservetopology can be non-deterministic

[cockroach-teamcity]

roachtest.costfuzz failed with artifacts on release-22.2 @ 9ec2ddb8dc118ce91bf806612bc4c0f86dd7b32b:

		  | 	github.com/cockroachdb/cockroach/pkg/cmd/roachtest/tests/query_comparison_util.go:66
		  | github.com/cockroachdb/cockroach/pkg/cmd/roachtest/tests.runCostFuzz
		  | 	github.com/cockroachdb/cockroach/pkg/cmd/roachtest/tests/costfuzz.go:44
		  | main.(*testRunner).runTest.func2
		  | 	main/pkg/cmd/roachtest/test_runner.go:930
		  | runtime.goexit
		  | 	GOROOT/src/runtime/asm_amd64.s:1594
		Wraps: (4) expected unperturbed and perturbed results to be equal
		  |   []string{
		  |   	... // 96 identical elements
		  |   	"010500000004000000010200000002000000A07E449FBAC0C2C1E009A140AE76"...,
		  |   	"0105000000090000000102000000050000007CC973D413E4FA41BED86CBEB219"...,
		  |   	strings.Join({
		  |   		... // 79 identical bytes
		  |   		"9EC41A424DD14DC9BFCC10811B9C4198CE44114BE4EBE679BE0C1CE1FB5F3C77",
		  |   		"5F84120B7A978CD3ABD411480B73AF4B401425EACD09D4F11F74101020000000",
		  | - 		"2",
		  | + 		"3",
		  |   		"000000584195279D91E941B2853B8E7F1AF9",
		  | + 		"41081ED0F323A7E3C148BE72AF33E7EF",
		  |   		"41899EC705D124F5C1C0D65A2F29C4F441010200000002000000C646DA0F2EBC",
		  |   		"F241823084D429B4F2C14C721FA9B7FBEB4150EA2CA4DBBAEDC1010200000002",
		  |   		... // 729 identical bytes
		  |   	}, ""),
		  |   	"010500008002000000010200008002000000781D9D718DE5D5C1359BDDBEEA30"...,
		  |   	"010500008003000000010200008003000000EA8DC4A142C4F3C120D843E8A651"...,
		  |   	... // 82 identical elements
		  |   }
		  | sql: SELECT
		  | 	st_simplifypreservetopology(tab_503.col2_4::GEOMETRY, 1.7976931348623157e+308:::FLOAT8::FLOAT8)::GEOMETRY
		  | 		AS col_1785,
		  | 	'8cae:cc90:fa16:b854:2fc0:7213:7f18:dde6/103':::INET AS col_1786,
		  | 	tab_503.col2_7 AS col_1787,
		  | 	tab_503.col2_10 AS col_1788,
		  | 	tab_503.col2_5 AS col_1789,
		  | 	tab_503.col2_5 AS col_1790,
		  | 	'\x27':::BYTES AS col_1791
		  | FROM
		  | 	defaultdb.public.table2@[0] AS tab_503
		  | WHERE
		  | 	EXISTS(
		  | 		SELECT
		  | 			'77 years 6 mons 999 days 19:46:11.170129':::INTERVAL AS col_1783,
		  | 			'1993-01-09 07:47:12.000519':::TIMESTAMP AS col_1784
		  | 		FROM
		  | 			defaultdb.public.table4@[0] AS tab_504
		  | 	)
		  | ORDER BY
		  | 	tab_503.col2_11 ASC
		Error types: (1) *withstack.withStack (2) *errutil.withPrefix (3) *withstack.withStack (4) *errutil.leafError

Parameters: ROACHTEST_cloud=gce , ROACHTEST_cpu=4 , ROACHTEST_encrypted=false , ROACHTEST_ssd=0

Help

See: roachtest README

See: How To Investigate (internal)

Same failure on other branches

• roachtest: costfuzz/seed-multi-region failed #90683 roachtest: costfuzz/seed-multi-region failed [C-test-failure O-roachtest O-robot T-sql-queries branch-master]

/cc @cockroachdb/sql-queries

This test on roachdash | Improve this report!

Jira issue: CRDB-21436

[rytaft]

This is very strange. I can reproduce as follows on master with cockroach demo:

CREATE TABLE table2 (
  col2_4  GEOMETRY
);

INSERT
  INTO table2 (col2_4)
VALUES (
'010500000003000000010200000006000000A895D1A400D5ECC1609C0989FD52DBC112D755C6B9A2F3C165ACC4C3BCA1F9C18082CD60BE54DAC15E9394D5D967F3C1B06A77C8AE1EF741868350996E4401C27049540680A0F441187733D39EB9FEC17CAC0CBDBAE0FD419E13116D07D7FAC1010200000005000000E2F7F28788E0F3C1AC2D2649F364FAC130416E4540CEC0C13C4C7A8A1A19DAC1EC0E3FC5DA45D6C180BADD00827C9C41D05C1423CE3BC7C1D08416A2067BD941D268575B9AADEFC10C074B65FBACFC41010200000004000000943129AA8569F24148930F3C642EF7C19AF0B939B3DBF541A08B753C8E7FB5414A80AD2B3A180042DA7C995C9FC5F941E071BC67B24ADCC1309CCE2B59F2C141':::GEOMETRY
       );


  SELECT st_simplifypreservetopology(
          col2_4,
          1.7976931348623157e+308
         )
    FROM table2;

Most of the time, the st_simplifypreservetopology function returns:

010500000003000000010200000003000000A895D1A400D5ECC1609C0989FD52DBC112D755C6B9A2F3C165ACC4C3BCA1F9C17CAC0CBDBAE0FD419E13116D07D7FAC1010200000002000000E2F7F28788E0F3C1AC2D2649F364FAC1D268575B9AADEFC10C074B65FBACFC41010200000003000000943129AA8569F24148930F3C642EF7C14A80AD2B3A180042DA7C995C9FC5F941E071BC67B24ADCC1309CCE2B59F2C141

But sometimes it returns:

010500000003000000010200000003000000A895D1A400D5ECC1609C0989FD52DBC112D755C6B9A2F3C165ACC4C3BCA1F9C17CAC0CBDBAE0FD419E13116D07D7FAC1010200000002000000E2F7F28788E0F3C1AC2D2649F364FAC1D268575B9AADEFC10C074B65FBACFC41010200000002000000943129AA8569F24148930F3C642EF7C1E071BC67B24ADCC1309CCE2B59F2C141

I can reproduce reliably by running the last command over and over again. Usually I see both results with no more than 10 runs in a row.

@otan, do you have any theories about why st_simplifypreservetopology might be non-deterministic here?

[rytaft]

(Note that st_simplifypreservetopology is immutable, so it should be deterministic)

[otan]

this is entirely using a third party library :(

looks like this was recently fixed to be deterministic -- libgeos/geos@e1b10c7. we can apply this patch to our version of GEOS if we wanted to.

one day we should also consider updating GEOS / PostGIS.

[msirek]

To fix this, would we need to create a new fork of geos, similar to https://github.com/cockroachdb/geos or would it suffice to sync that fork with the latest code? The latter might impact builds of older CRDB releases.

References to the libgeos version would need to be updated from 3.8.1 to 3.12.0 as well.

c-deps/BUILD.bazel
184:            "cp -L lib/libgeos.3.8.1.dylib $INSTALLDIR/lib/libgeos.dylib\n" +
186:            "$CMAKE_INSTALL_NAME_TOOL -id @rpath/libgeos.3.8.1.dylib $INSTALLDIR/lib/libgeos.dylib\n" +
188:            "$CMAKE_INSTALL_NAME_TOOL -change $PREFIX/libgeos.3.8.1.dylib @rpath/libgeos.3.8.1.dylib $INSTALLDIR/lib/libgeos_c.dylib\n"
190:        "@io_bazel_rules_go//go/platform:darwin": "cp -L lib/libgeos.3.8.1.dylib $INSTALLDIR/lib/libgeos.dylib",

Maybe SQL Queries shouldn't be making these changes, but another team?

[otan]

libgeos is due for an upgrade for sure, i think the master branch has everything we needed since we made the fork.
as for who owns it ... good question ^_^

[msirek]

@cockroachdb/dev-inf We may need some help upgrading libgeos

[otan]

let's see how #106642 goes

[otan]

need dev-inf's help upgrading the archived_cdep_repository with c-deps - can't find any instructions for it...

[yuzefovich]

@otan can this now be closed given #106642 merged?

[otan]

yep