Subject: Updated packaging for MobilityDB 1.1.0~beta1

From: Bradford Boyle <[email-masked]>
Date: Sat, 4 Nov 2023 16:19:05 -0700

Hello,

I have just pushed an update to the Debian packaging of MobilityDB to
salsa.debian.org [1] that addresses the feedback [2] from the first
attempt at packaging this extension. The build passes the GitLab CI
pipeline and I have verified that the package builds and autopkgtests
pass on arm64 (via sbuild). Below is a summary of how I have addressed
the comments/questions raised in the previous review. As time permits, I
would appreciate another review.

* I have updated debian/copyright to more accurately reflect the
  copyrights found in the source. I bootstrapped this file using cme[3]
  and then manually addressed all of the TODOs. I've also ensured that
  lintian doesn't raise any errors or warnings related to d/copyright.
* I've updated d/tests/installcheck to avoid the use of
  shared_preload_libraries when creating the test cluster
    * Tests instead set session_preload_libraries via PGOPTIONS to load
      the postgis extension. I don't think it is possible to link
      against postgis since it is not a shared library (no SONAME).
* I've updated d/control.in so that PGVERSION is no longer used --
  declaring a dependency on postgresql-postgis is sufficient

Thanks,

-- Bradford

[1]: https://salsa.debian.org/postgresql/mobilitydb/-/commit/9dfff5eee4eab17b8d20ecd408ed10b830750244
[2]: https://www.postgresql.org/message-id/ZMfMWcLtZIVxbDYj%40msg.df7cb.de
[3]: https://wiki.debian.org/CopyrightReviewTools#cme

---

From: Christoph Berg <[email-masked]>
Date: Sun, 5 Nov 2023 15:04:08 +0100

Re: Bradford Boyle
> attempt at packaging this extension.

Thanks!

>   declaring a dependency on postgresql-postgis is sufficient

In postgresql-XX-mobility, we need the XX version of
postgresql-XX-postgis-3, not just any version, so this change isn't
correct.

The usage of "postgresql-postgis" in the Build-Depends is also fishy,
but the "update debian/control from control.in" machinery doesn't
support using PGVERSION in the Source section yet. I tried
implementing that the other week, but didn't get to finish it yet,
hopefully that will happen over the next days.

I haven't investigated the cause yet, but the pgdg build is failing:

https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/2/architecture=amd64,distribution=sid/console

14:56:48 dh_missing: warning: usr/lib/postgresql/16/lib/libMobilityDB-1.1.so exists in debian/tmp but is not installed to anywhere
14:56:48 dh_missing: warning: usr/share/postgresql/16/extension/mobilitydb--1.1.0.sql exists in debian/tmp but is not installed to anywhere
14:56:48 dh_missing: warning: usr/share/postgresql/16/extension/mobilitydb.control exists in debian/tmp but is not installed to anywhere
14:56:48 dh_missing: error: missing files, aborting

Christoph

---

From: Christoph Berg <[email-masked]>
Date: Mon, 6 Nov 2023 17:25:42 +0100

Re: To Bradford Boyle
> hopefully that will happen over the next days.

Hi Bradford,

I've done that now, it will be available in postgresql-common 256.

I pushed a change that uses it and also loops over the versions that
should be targeted for cmake.

Ideally, testing the extension could also use this from
debian/tests/installcheck:

pg_buildext run_installed /usr/bin/cmake -Btest-%v
pg_buildext run_installed make -C test-% test

... but that starts by recompiling the package instead of just running
the tests.

Perhaps you have an idea how to make it work? The existing
debian/tests/regress.sh file looks very complex and might break with
every little upstream change. (Of course if there's no easier way to
get the upstream testsuite to run, we have to go the complex route.)

Christoph

---

From: Bradford Boyle <[email-masked]>
Date: Tue, 7 Nov 2023 23:19:06 -0800

Hi Christoph,

> I've done that now, it will be available in postgresql-common 256.

Thank you!

> get the upstream testsuite to run, we have to go the complex route.)

I agree with your assessment of d/tests/regress.sh. The reason I went
that route initially was because running cmake followed by make test was
rebuilding the package. I did see that autopkgtest has the
'build-needed' restriction [1] but its use seems to be discouraged. I
took another look at the upstream testsuite to see if we can (easily)
run its testsuite without require a rebuild and I think I have found a
way.

In short, upstream was defining a test called 'build' that would rebuild
the extension. We can instruct ctest to exclude this test by specifying
a regex. I was able to drop regress.sh altogether and have
d/tests/installcheck run

cmake -Btest-%v
make -C test-%v ARGS='-E build' test

This does require repeating the list of -dev packages in d/tests/control
so that cmake can generate the Makefiles. If this sounds like a good
approach to you, I can push the commit to s.d.o.

--Bradford

[1]: https://people.debian.org/~eriberto/README.package-tests.html

---

From: Christoph Berg <[email-masked]>
Date: Wed, 8 Nov 2023 09:13:45 +0100

Re: Bradford Boyle
> approach to you, I can push the commit to s.d.o.

Sure, that sounds sane.

Christoph

---

From: Christoph Berg <[email-masked]>
Date: Thu, 4 Apr 2024 14:12:40 +0200

Hi Bradford,

where are we with MobilityDB? Are the remaining test failures a
problem in the source, or just artifacts of the way we try to run the
tests?

Christoph

---

From: Esteban Zimanyi <[email-masked]>
Date: Thu, 4 Apr 2024 14:24:12 +0200

Dear all (cc MobilityDB Team and Regina Obe)

The MobilityDB 1.1 version was released two weeks ago.
See: https://github.com/MobilityDB/MobilityDB/releases/tag/v1.1.0

However, as this release was not running on ARM, we were waiting to produce
a v1.1.1 release with small bug fixes that allows MobilityDB to run on the
ARM architecture. This is now almost complete and we are thus planning to
release and announce MobilityDB 1.1.1 either at the end of this week or
next week.

Regards

Esteban
------------------------------------------------------------
Prof. Esteban Zimanyi
Department of Computer & Decision Engineering  (CoDE) CP 165/15
Universite Libre de Bruxelles
Avenue F. D. Roosevelt 50
B-1050 Brussels, Belgium
fax: + 32.2.650.47.13
tel: + 32.2.650.31.85
e-mail: [email-masked]
Internet: http://cs.ulb.ac.be/members/esteban/
------------------------------------------------------------

---

From: Bradford Boyle <[email-masked]>
Date: Thu, 4 Apr 2024 23:06:13 -0700

Hi Christoph,

> problem in the source, or just artifacts of the way we try to run the

I pushed a commit to s.d.o this past weekend updating the version from
1.1.0~rc.1 to 1.1.0. Looking at the latest build on pgdgbuild [1], I see
that there is a failure for sid amd64. I've only looked at the build
output briefly but it looks like the failure is building against
postgres 12:

    meos/src/general/tsequence.c:3116:19: error: implicit declaration
of function ‘hash_bytes_uint32’;

There have been some recent commits on this file (tsequence.c) between
1.1.0~rc.1 and 1.1.0 but I'll need to investigate more to figure out a
fix.

--Bradford

[1]: https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/8/

---

From: Christoph Berg <[email-masked]>
Date: Fri, 5 Apr 2024 09:38:30 +0200

Re: Bradford Boyle
> 1.1.0~rc.1 and 1.1.0 but I'll need to investigate more to figure out a

If it's not trivial to fix, there is always the option to exclude the
older PG versions from the build. 12 is going to be EOL soonish
anyway.

Christoph

---

From: Bradford Boyle <[email-masked]>
Date: Sun, 7 Apr 2024 19:25:59 -0700

Hi Chistoph,

I pushed a commit to s.d.o dropping support for Postgres 12 for
MobilityDB. Can you trigger another build on pgdgbuild?

Thanks,
--Bradford

---

From: SCHOEMANS Maxime <[email-masked]>
Date: Mon, 8 Apr 2024 15:45:24 +0000

Hi Christoph, Bradford,

Re: Christoph Berg

Re: Bradford Boyle
 > MobilityDB. Can you trigger another build on pgdgbuild?

In case you still want to keep support for Postgres 12, we just 
published a bugfix release of MobilityDB (v1.1.1) which contains the fix 
needed to run on pg12.
Let us know if there is any way we can help in this packaging effort.

Best,
Maxime

---

From: Bradford Boyle <[email-masked]>
Date: Mon, 8 Apr 2024 22:55:06 -0700

I've updated the Debian package to build MobilityDB v1.1.1 and added
back Postgres 12 support.

-- Bradford

---

From: Bradford Boyle <[email-masked]>
Date: Tue, 9 Apr 2024 23:44:03 -0700

Hi Christop,

I was reviewing the latest builds for MobilityDB on pgdgbuild and it
looks like autopkgtest is still failing. The issue is that the cmake
build assumes the source tree is writable and the source tree is not
checked out in the same directory as AUTOPKGTEST_TMP. Here are two lines
from one of the failed autopkgtest runs:

    chmod -R go+rwX /tmp/autopkgtest.R3ZFXt/autopkgtest_tmp
    ...
    CMake Error: Could not open file for write in copy operation
/tmp/autopkgtest.R3ZFXt/tree/postgis/postgis_config.h.tmp

This is why recursively chmod-ing the AUTOPKGTEST_TMP didn't work. I
have a fix in mind but need to find some time to verify it.

--Bradford

---

From: Bradford Boyle <[email-masked]>
Date: Sat, 13 Apr 2024 17:58:57 -0700

Hello Maxime,

I have a development branch [1] for the Debian packaging for MobilityDB
1.1.1 that contains some fixes for the build that allow running the
autopkgtest suite for MobilityDB on pgdgbuild [2]. I needed to update a
couple of CMakeLists.txt to write generated configuration files to the
build directory instead of back to the source directory.

The linked job builds one Debian package per supported Postgres version
and then runs MobilityDB's tests against the installed package. It looks
like there are four failing tests for Postgres 12, 13, 14, 15, and 16:

* 053_tpoint_inout_tbl (Failed)
  - problem in alloc set ExprContext: detected write past chunk end in
    block 0x560f423d2de0, chunk 0x560f423d2e08
* 070_tpoint_spatialrels (Failed)
* 091_tnpoint_routeops_gin_tbl (Failed)
  - invalid memory alloc request size 2139062143
* 091_tnpoint_routeops_tbl (Failed)
  - invalid memory alloc request size 2139062143

You can view the full console output (which includes both building and
running the tests) here [3].

Regards,
-- Bradford

[1]: https://salsa.debian.org/postgresql/mobilitydb/-/tree/dev-pgdg-autopkgtest?ref_type=heads
[2]: https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/
[3]: https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/16/architecture=amd64,distribution=sid/consoleText

---

From: SCHOEMANS Maxime <[email-masked]>
Date: Thu, 18 Apr 2024 15:08:11 +0000

Hi Bradford,

We have been wanting to avoid writing the config files in the source 
tree for some time now, so the patch with the fixed for the build is 
very much appreciated.
You are welcome to send a PR to the develop branch [1]on the mobilitydb 
repository with these changes.

Concerning the failing tests, we indeed did not catch these as they only 
appear when running postgres with debug enabled.
We will update our tests to avoid missing these in the future.
In the mean time, I have fixed the issues on our develop branch [2], so 
we could produce a new bugfix release with these fixes and the build 
patch if this is the only remaining blocker.
I am attaching the patch with the fixes so you can test it out before we 
produce the release.

Let us know if there are any other issues or if we can assist you in 
producing the PR with the build patch.

Best,
Maxime

[1]: https://github.com/MobilityDB/MobilityDB/tree/develop
[2]: 
https://github.com/mobilityDB/MobilityDB/compare/942b10c47e7966f3d0b61a1d3a66e0400ee4769d...6e0e6572072c90dade2148d87ecaba6f5f072ddb

diff --git a/meos/src/general/type_out.c b/meos/src/general/type_out.c
index c3cadb057..350ca30fe 100644
--- a/meos/src/general/type_out.c
+++ b/meos/src/general/type_out.c
@@ -848,7 +848,7 @@ tsequenceset_mfjson_size(const TSequenceSet *ss, bool isgeo, bool hasz,
   size += ( isgeo ? sizeof("{'coordinates':[],") : sizeof("{'values':[],") ) * ss->count;
   size += sizeof("'datetimes':[],'lower_inc':false,'upper_inc':false},") * ss->count;
   if (isgeo)
-    size = coordinates_mfjson_size(ss->totalcount, hasz, precision);
+    size += coordinates_mfjson_size(ss->totalcount, hasz, precision);
   else
   {
     for (int i = 0; i < ss->count; i++)
diff --git a/mobilitydb/src/npoint/tnpoint_gin.c b/mobilitydb/src/npoint/tnpoint_gin.c
index d011aa535..de9441447 100644
--- a/mobilitydb/src/npoint/tnpoint_gin.c
+++ b/mobilitydb/src/npoint/tnpoint_gin.c
@@ -75,9 +75,9 @@ Tnpoint_gin_extract_value(PG_FUNCTION_ARGS)
   Datum *elems = palloc(sizeof(Datum) * routes->count);
   for (int i = 0; i < routes->count; i++)
     elems[i] = Int64GetDatum(SET_VAL_N(routes, i));
-  pfree(routes);
   *nkeys = routes->count;
   *nullFlags = NULL;
+  pfree(routes);
   PG_FREE_IF_COPY(temp, 0);
   PG_RETURN_POINTER(elems);
 }
@@ -127,9 +127,9 @@ Tnpoint_gin_extract_query(PG_FUNCTION_ARGS)
       elems = palloc(sizeof(Datum) * routes->count);
       for (int i = 0; i < routes->count; i++)
         elems[i] = Int64GetDatum(SET_VAL_N(routes, i));
-      pfree(routes);
       *nkeys = routes->count;
       *searchMode = GIN_SEARCH_MODE_DEFAULT;
+      pfree(routes);
       PG_FREE_IF_COPY(temp, 0);
       break;
     default:
diff --git a/postgis/liblwgeom/lwhomogenize.c b/postgis/liblwgeom/lwhomogenize.c
index 70c39e9ee..ac20ab7a7 100644
--- a/postgis/liblwgeom/lwhomogenize.c
+++ b/postgis/liblwgeom/lwhomogenize.c
@@ -101,7 +101,7 @@ lwcollection_build_buffer(const LWCOLLECTION *col, HomogenizeBuffer *buffer)
 				buffer->buf[geom->type] = bufcol;
 			}
 			/* Add sub-geom to buffer */
-			lwcollection_add_lwgeom(buffer->buf[geom->type], lwgeom_clone(geom));
+			lwcollection_add_lwgeom(buffer->buf[geom->type], lwgeom_clone_deep(geom));
 			/* Increment count for this singleton type */
 			buffer->cnt[geom->type]++;
 			break;
@@ -217,7 +217,7 @@ lwgeom_homogenize(const LWGEOM *geom)
 			return lwcollection_as_lwgeom(lwcollection_construct_empty(geom->type, geom->srid, lwgeom_has_z(geom), lwgeom_has_m(geom)));
 		}
 
-		return lwgeom_clone(geom);
+		return lwgeom_clone_deep(geom);
 	}
 
 	switch (geom->type)
@@ -231,7 +231,7 @@ lwgeom_homogenize(const LWGEOM *geom)
 		case TRIANGLETYPE:
 		case CURVEPOLYTYPE:
 		case POLYGONTYPE:
-			return lwgeom_clone(geom);
+			return lwgeom_clone_deep(geom);
 
 		/* Process homogeneous geometries lightly */
 		case MULTIPOINTTYPE:
@@ -247,7 +247,6 @@ lwgeom_homogenize(const LWGEOM *geom)
 			/* Strip single-entry multi-geometries down to singletons */
 			if ( col->ngeoms == 1 )
 			{
-				// hgeom = lwgeom_clone((LWGEOM*)(col->geoms[0])); // MobilityDB changed
 				hgeom = lwgeom_clone_deep((LWGEOM*)(col->geoms[0]));
 				hgeom->srid = geom->srid;
 				if (geom->bbox)
@@ -256,7 +255,7 @@ lwgeom_homogenize(const LWGEOM *geom)
 			}
 
 			/* Return proper multigeometry untouched */
-			return lwgeom_clone(geom);
+			return lwgeom_clone_deep(geom);
 		}
 
 		/* Work on anonymous collections separately */

---

From: Bradford Boyle <[email-masked]>
Date: Sun, 21 Apr 2024 21:39:13 -0700

Hello,

I’ve updated the packaging for MobilityDB 1.1.1 to address the
autopkgtest issues when building on pgdg. MobilityDB does not build on
buster becuase of the old version of libproj; the package also fails to
build on i386. Additionally there are test failures on s390x. Attached
is a patch for pgapt to exclude these (buster, i386, and s390z) from the
build.

--Bradford

---

From: Christoph Berg <[email-masked]>
Date: Tue, 7 May 2024 17:13:24 +0200

Re: SCHOEMANS Maxime
> producing the PR with the build patch.

Hi,

one more thing that would be nice: Every time you invoke `psql` during
testing, it should really be `psql -X` so it doesn't read the user's
~/.psqlrc file. In my case, the `\pset linestyle unicode` and
`\timing` in there cause a lot of noise in the test results.

Re: Bradford Boyle
> is a patch for pgapt to exclude these (buster, i386, and s390z) from the

I've finally found the time to get back to this; the build is running.

Sorry, sometimes I'm overwhelmed by TODO items and then simply drop
some of them indefinitely.

Cheers,
Christoph

---

From: SCHOEMANS Maxime <[email-masked]>
Date: Wed, 15 May 2024 15:42:02 +0000

Hi,

Thanks a lot for your work.

 > `\timing` in there cause a lot of noise in the test results.

Indeed, this is something we did not think of.
I changed this on both master and stable-1.1, so it will be fixed in all 
upcoming releases.

Best,
Maxime