Update extension version checking logic by var77 · Pull Request #325 · lanterndata/lantern

var77 · 2024-07-24T08:58:20Z

Generate static array from CMake for compatible version list based on update file names. Use that array to check if current catalog version of extension is compatible with current binary version

greptile-apps

PR Summary

The pull request updates the extension version checking logic to ensure compatibility between binary and SQL versions, adding new helper functions for semantic version comparison.

sql/lantern.sql: Introduced _lantern_internal functions for version checking and comparison.
src/hnsw.c: Added lantern_internal_get_binary_version function for retrieving binary version.
src/hnsw/build.c: Replaced VersionsMatch() with CmpExtensionVersions() for stricter version checks.
src/hnsw/utils.c: Renamed and updated VersionsMatch to CmpExtensionVersions with enhanced logic.
test/expected/ext_relocation.out: Added tests for new internal version checking functions.

_{10 file(s) reviewed, 18 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-24T08:58:54Z

sql/lantern.sql

+CREATE FUNCTION _lantern_internal.get_binary_version() RETURNS TEXT
+	AS 'MODULE_PATHNAME', 'lantern_internal_get_binary_version' LANGUAGE C STABLE STRICT PARALLEL SAFE;


Info: Added function to get the binary version of the Lantern extension.

greptile-apps · 2024-07-24T08:58:55Z

sql/lantern.sql

+CREATE OR REPLACE FUNCTION _lantern_internal.get_catalog_version() RETURNS TEXT as $$
+DECLARE
+  cat_version TEXT;
+BEGIN
+   SELECT extversion INTO cat_version FROM pg_extension WHERE extname = 'lantern';
+   RETURN cat_version;
+END;
+$$ LANGUAGE plpgsql;


Info: Added function to get the catalog version of the Lantern extension.

greptile-apps · 2024-07-24T08:58:56Z

sql/lantern.sql

+END;
+$$ LANGUAGE plpgsql;
+
+CREATE OR REPLACE FUNCTION _lantern_internal.parse_semver(semver text)


Info: Added function to parse semantic versioning (semver) strings.

greptile-apps · 2024-07-24T08:58:57Z

sql/lantern.sql

+--    0  if versions are matching
+--    1  if binary version is bigger  than catalog version
+--    -1 if binary version is smaller than catalog version
+CREATE OR REPLACE FUNCTION _lantern_internal.compare_extension_versions(catalog_version TEXT, binary_version TEXT) RETURNS INT as $$


Info: Added function to compare the binary and catalog versions of the Lantern extension.

greptile-apps · 2024-07-24T08:59:23Z

src/hnsw.c

 #include <utils/selfuncs.h>
 #include <utils/spccache.h>

+#include "version.h"


Info: Included version.h to access version information.

greptile-apps · 2024-07-24T09:02:39Z

src/hnsw/utils.h

 void                  label2ItemPointer(usearch_label_t label, ItemPointer itemPtr);
 float4               *ToFloat4Array(ArrayType *arr, int *dim_out);
-bool                  VersionsMatch();
+int                   CmpExtensionVersions();


Info: Renamed VersionsMatch to CmpExtensionVersions to implement stricter version checking.

greptile-apps · 2024-07-24T09:03:10Z

test/expected/ext_relocation.out

 extschema |            proname             |     proschema     
 -----------+--------------------------------+-------------------
 schema1   | _create_ldb_operator_classes   | _lantern_internal
+ schema1   | compare_extension_versions     | _lantern_internal


Info: Added compare_extension_versions function to compare extension versions.

greptile-apps · 2024-07-24T09:03:10Z

test/expected/ext_relocation.out

 schema1   | create_pq_codebook             | _lantern_internal
 schema1   | failure_point_enable           | _lantern_internal
 schema1   | forbid_table_change            | _lantern_internal
+ schema1   | get_binary_version             | _lantern_internal


Info: Added get_binary_version function to retrieve the binary version of the extension.

greptile-apps · 2024-07-24T09:03:11Z

test/expected/ext_relocation.out

 schema1   | failure_point_enable           | _lantern_internal
 schema1   | forbid_table_change            | _lantern_internal
+ schema1   | get_binary_version             | _lantern_internal
+ schema1   | get_catalog_version            | _lantern_internal


Info: Added get_catalog_version function to retrieve the catalog version of the extension.

greptile-apps · 2024-07-24T09:03:12Z

test/expected/ext_relocation.out

+ schema1   | get_catalog_version            | _lantern_internal
 schema1   | mask_arrays                    | _lantern_internal
 schema1   | mask_order_by_in_plan          | _lantern_internal
+ schema1   | parse_semver                   | _lantern_internal


Info: Added parse_semver function to parse semantic version strings.

github-actions · 2024-07-24T09:06:54Z

Benchmarks

metric	old	new	pct change
recall (after create)	0.951	0.955	+0.42%
recall (after insert)	0.000	0.000	-
select tps	25374.002	25470.947	+0.38%
select bulk(100) tps	35.720	35.393	-0.92%
select latency (ms)	0.745 ± 1.560𝜎	0.704 ± 1.548𝜎	-5.50%
select bulk(100) latency (ms)	885.606 ± 99.034𝜎	870.792 ± 131.370𝜎	-1.67%
create latency (ms)	396793.805	400092.278	+0.83%
insert tps	431.859	445.026	+3.05%
insert bulk(100) tps	4.802	4.645	-3.27%
insert latency (ms)	73.505 ± 15.007𝜎	71.161 ± 15.660𝜎	-3.19%
insert bulk(100) latency (ms)	6513.105 ± 135.856𝜎	6780.624 ± 88.691𝜎	+4.11%
disk usage (bytes)	8192008192.000	8192008192.000	-

…nd use it to check for incompatible sql and binary versions

codecov · 2024-07-25T13:02:50Z

Codecov Report

Attention: Patch coverage is 41.66667% with 21 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
src/hnsw/utils.c	34.37%	15 Missing and 6 partials ⚠️

📢 Thoughts on this report? Let us know!

Ngalstyan4 · 2024-08-06T12:08:41Z

src/hnsw/utils.c

+    version_text = DatumGetTextP(val);
+    catalog_version = text_to_cstring(version_text);
+
+    if(strlen(catalog_version) == 0) {


so, if catalog version has zero length, we assume it is always compatible with any binary version?
is it what we want? I would think we would consider this error in all cases.

Ngalstyan4 · 2024-08-06T12:11:08Z

src/hnsw/utils.c

+
+    // Grab the result and check that it matches the version in the generated header
+    version_text = DatumGetTextP(val);
+    catalog_version = text_to_cstring(version_text);


You are storing version_text in a global variable but I am pretty sure it gets free()ed after the current context is gone, while catalog_version remains available as a freed pointer in the global process scope.

Is catalog_version actually used anywhere/
if so, it should probably be a NAME-sized static array into which you can strncpy the value of version_text

Ngalstyan4 · 2024-08-06T12:11:48Z

src/hnsw/utils.c

-bool versions_match = false;
-bool version_checked = false;
+bool  version_checked = false;
+bool  versions_match = true;


why default versions_match to true here?

…ion string using strncopy instead of using pointer

var77 requested a review from Ngalstyan4 July 24, 2024 08:58

greptile-apps bot reviewed Jul 24, 2024

View reviewed changes

var77 force-pushed the varik/strict-version-checking branch from b43ace5 to 5474e79 Compare July 24, 2024 14:52

generate compatible versions array from cmake based on update files a…

fcfb4d9

…nd use it to check for incompatible sql and binary versions

var77 force-pushed the varik/strict-version-checking branch from 5474e79 to fcfb4d9 Compare July 25, 2024 12:49

do not call SPI_finish while the result is being used

1b7ffbe

var77 force-pushed the varik/strict-version-checking branch 3 times, most recently from bb8f4fe to 126cc3f Compare July 26, 2024 06:06

remove unnecessarry update SQL

0f8608f

var77 force-pushed the varik/strict-version-checking branch from 126cc3f to 0f8608f Compare July 30, 2024 08:17

Ngalstyan4 reviewed Aug 6, 2024

View reviewed changes

use CMake's join function for string concatenation, copy catalog vers…

087a52e

…ion string using strncopy instead of using pointer

var77 merged commit 39d6fac into main Aug 7, 2024

var77 deleted the varik/strict-version-checking branch August 7, 2024 08:48

		CREATE FUNCTION _lantern_internal.get_binary_version() RETURNS TEXT
		AS 'MODULE_PATHNAME', 'lantern_internal_get_binary_version' LANGUAGE C STABLE STRICT PARALLEL SAFE;

Conversation

var77 commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

codecov bot commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

var77 commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading