Show more information in get_git_commit #2468

mkindahl · 2020-09-30T08:09:02Z

Show more information in get_git_commit

The command get_git_commit shows quite a little information and
especially if mistakes are made it is hard to verify what the binary is
actually based on.

This commit extends the function to provide some more information:
specifically the SHA of the HEAD regardless of whether there is a tag
on it or not and the time for the HEAD commit.

postgres=# select * from _timescaledb_internal.get_git_commit();
        commit_tag        | commit_hash |      commit_time
--------------------------+-------------+------------------------
 1.7.4-10-g09b0b778-dirty | 09b0b778    | 2020-09-13 17:50:38+02
(1 row)

If git is not installed, the function get_git_commit will return an
error indicating that no git information is available. If some of the
fields are available, they will be emitted and the remaining fields
will be NULL.

Fixes #2457

pmwkaa

Looks good. Slightly concerned about commit_sha name maybe use commit_id or commit_hash name instead?

erimatnor · 2020-10-01T09:25:41Z

src/version.c

@@ -20,21 +20,39 @@
 #define STR_EXPAND(x) #x
 #define STR(x) STR_EXPAND(x)

-static const char *git_commit = STR(EXT_GIT_COMMIT);
+static const char commit_sha[] = EXT_GIT_COMMIT_SHA;


Do we want to make this an exported string, e.g., TSDLLEXPORT and name it ts_commit_sha, so that one can use strings and grep directly on the binary?

I was considering this, but we might want to have some sort of tag so that we can easily find it, e.g., sha:aec070645fe53ee3b3763059376134f058cc337247c978add178b6ccdfb0019f and the same for the tag.

I created three tags commit-sha:, commit-tag:, and commit-time: and added them. I do not think we need to export them since they are visible in the SO anyway as long as the link-time optimizer does not remove them. The prefix is removed before added to the result.

mkindahl · 2020-10-01T09:32:17Z

Looks good. Slightly concerned about commit_sha name maybe use commit_id or commit_hash name instead?

It is referred to in the documentation as sha1 or "SHA-1 checksum" so I think commit_sha is fine.

codecov · 2020-10-02T09:17:12Z

Codecov Report

Merging #2468 (453ee02) into master (37cef82) will increase coverage by 0.14%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master    #2468      +/-   ##
==========================================
+ Coverage   89.97%   90.11%   +0.14%     
==========================================
  Files         212      212              
  Lines       34496    34451      -45     
==========================================
+ Hits        31037    31046       +9     
+ Misses       3459     3405      -54

Impacted Files	Coverage Δ
src/version.c	`86.88% <90.00%> (-0.62%)`	⬇️
src/loader/bgw_message_queue.c	`87.74% <0.00%> (+3.22%)`	⬆️
src/import/planner.c	`70.30% <0.00%> (+11.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37cef82...453ee02. Read the comment docs.

svenklemm · 2020-10-02T12:16:23Z

Looks good. Slightly concerned about commit_sha name maybe use commit_id or commit_hash name instead?

I agree i think this should be named commit_hash

svenklemm · 2020-10-02T14:13:46Z

Looks good. Slightly concerned about commit_sha name maybe use commit_id or commit_hash name instead?

It is referred to in the documentation as sha1 or "SHA-1 checksum" so I think commit_sha is fine.

git is planning to move away from sha1 so a more generic name makes sense https://git-scm.com/docs/hash-function-transition/

mkindahl · 2020-10-05T12:36:34Z

Looks good. Slightly concerned about commit_sha name maybe use commit_id or commit_hash name instead?

It is referred to in the documentation as sha1 or "SHA-1 checksum" so I think commit_sha is fine.

git is planning to move away from sha1 so a more generic name makes sense https://git-scm.com/docs/hash-function-transition/

Wasn't aware of that. Makes sense to call it hash then.

erimatnor

Approving, but there are some things that I believe needs an extra look before merging.

erimatnor · 2020-11-25T09:34:27Z

.github/workflows/update-test.yaml

@@ -29,7 +29,7 @@ jobs:

    - name: Update tests ${{ matrix.pg }}
      run: |
-        ./scripts/test_updates_pg${{ matrix.pg_major}}.sh
+        bash -x ./scripts/test_updates_pg${{ matrix.pg_major}}.sh


Nit, use sh instead since the shell might not be bash.

It was only used for debugging. Removed.

erimatnor · 2020-11-25T09:35:33Z

.github/workflows/alpine-32bit-build-and-test.yaml

@@ -30,7 +30,7 @@ jobs:

    - name: Install build dependencies
      run: |
-        apk add --no-cache --virtual .build-deps coreutils dpkg-dev findutils gcc libc-dev make util-linux-dev diffutils cmake openssl-dev sudo flex bison
+        apk add --no-cache --virtual .build-deps coreutils dpkg-dev findutils gcc libc-dev make util-linux-dev diffutils cmake openssl-dev sudo flex bison git


Do we need this dependency now that Git is still optional? Isn't it good to also run some tests where we don't have Git installed and build the dummy version of the get_git_commit function?

I think we should require git also for this case. Before, the tests was designed to not fail when Git was not available, but that is not the case right now and it will fail if git is not available (because the function generate an error rather than some output).

Since this is an internal function intended for debugging, I do not think a full test is necessary, but if you feel strongly about testing this, we can create a separate test that is only added if Git is absent.

erimatnor · 2020-11-25T09:36:10Z

scripts/docker-build.sh

@@ -54,7 +54,7 @@ create_postgres_build_image() {
    docker run -d --name ${BUILD_CONTAINER_NAME} --env POSTGRES_HOST_AUTH_METHOD=trust -v ${BASE_DIR}:/src postgres:${PG_IMAGE_TAG}

    # Install build dependencies
-    docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "apk add --no-cache --virtual .build-deps gdb git coreutils dpkg-dev gcc libc-dev make cmake util-linux-dev diffutils openssl-dev && mkdir -p /build/debug"
+    docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "apk add --no-cache --virtual .build-deps gdb coreutils dpkg-dev gcc git libc-dev make cmake util-linux-dev diffutils openssl-dev && mkdir -p /build"


Same question as above, regarding need to install git.

Git is installed both before and after this change, it's just in a different place.

erimatnor · 2020-11-25T09:38:32Z

scripts/docker-build.sh

    # Build and install the extension with debug symbols and assertions
-    tar -c -C ${BASE_DIR} {src,sql,test,scripts,tsl,version.config,CMakeLists.txt,timescaledb.control.in} | docker cp - ${BUILD_CONTAINER_NAME}:/build/
-    docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "cd /build/debug && cmake -DUSE_OPENSSL=${USE_OPENSSL} -DREGRESS_CHECKS=OFF -DCMAKE_BUILD_TYPE=${BUILD_TYPE} .. && make && make install && echo \"shared_preload_libraries = 'timescaledb'\" >> /usr/local/share/postgresql/postgresql.conf.sample && echo \"timescaledb.telemetry_level=off\" >> /usr/local/share/postgresql/postgresql.conf.sample && cd / && rm -rf /build"
+    docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "cd /build && cmake -DUSE_OPENSSL=${USE_OPENSSL} -DREGRESS_CHECKS=OFF -DCMAKE_BUILD_TYPE=${BUILD_TYPE} /src && make && make install && echo \"shared_preload_libraries = 'timescaledb'\" >> /usr/local/share/postgresql/postgresql.conf.sample && echo \"timescaledb.telemetry_level=off\" >> /usr/local/share/postgresql/postgresql.conf.sample && cd / && rm -rf /build"


I don't understand this change, in particular the removal the removal of tar and copy. This seems unrelated to the main changes in this PR. At least an explanation is in order.

erimatnor · 2020-11-25T09:43:54Z

src/version.c

+/* Export the strings to that we can read them using strings(1). We add a
+ * prefix so that we can easily find it using grep(1). We only bother about
+ * generating them if the relevant symbol is defined. */
+#ifdef EXT_GIT_COMMIT_HASH


Will this work as expected?

Even if @EXT_GIT_COMMIT_TAG@ is empty in gitcommit.h.in, I suspect EXT_GIT_COMMIT_TAG will still be defined (albeit empty)?

Not according to the documentation: https://cmake.org/cmake/help/latest/command/configure_file.html

Still unclear what happens if EXT_GIT_COMMIT_HASH is defined like this:

#define EXT_GIT_COMMIT_HASH

But it is not defined like that in the generated file, it is either

#define EXT_GIT_COMMIT_HASH "2beeb3fc"

or

/* #undef EXT_GIT_COMMIT_HASH */

If a builder decides on editing the file and putting anything else there, they can break the build whatever we do, so there is no reason to handle that case.

That's what I wanted to know.

erimatnor · 2020-11-25T09:50:21Z

test/expected/version.out

@@ -5,7 +5,7 @@
 select pg_typeof(git) from _timescaledb_internal.get_git_commit() AS git;


I suggest changing this to typeof each returned column. Or at least add an additional line to the test to do that, similar to get_os_info below. Will it work for NULL values?

Calling pg_typeof on each column. Should work for NULL.

mats=# select * from foo; x | second --------+-------- 1 | 2 [NULL] | [NULL] (2 rows) mats=# select pg_typeof(second) from foo; pg_typeof ------------------ double precision double precision (2 rows)

erimatnor · 2020-11-25T09:51:36Z

src/version.c

+#endif
+
+#ifdef EXT_GIT_COMMIT_TIME
+	values[2] = DirectFunctionCall3(timestamptz_in,


Do we know that the the Git timestamp is always a parsable TimestampTz string?

AIUI, yes. The default is RFC2822, which is parseable by the timestamptz function: https://www.postgresql.org/message-id/32c009ea0510121301y6da7d43eq@mail.gmail.com

Documentation is vague, but says:

Date and time input is accepted in almost any reasonable format, including ISO 8601, SQL-compatible, traditional POSTGRES, and others.

erimatnor · 2020-11-25T16:49:12Z

scripts/docker-build.sh

@@ -54,7 +54,7 @@ create_postgres_build_image() {
    docker run -d --name ${BUILD_CONTAINER_NAME} --env POSTGRES_HOST_AUTH_METHOD=trust -v ${BASE_DIR}:/src postgres:${PG_IMAGE_TAG}

    # Install build dependencies
-    docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "apk add --no-cache --virtual .build-deps gdb git coreutils dpkg-dev gcc libc-dev make cmake util-linux-dev diffutils openssl-dev && mkdir -p /build/debug"
+    docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "apk add --no-cache --virtual .build-deps gdb coreutils dpkg-dev gcc git libc-dev make cmake util-linux-dev diffutils openssl-dev && mkdir -p /build/debug"


Looks like git is just moved to later in the list. Looks like this change isn't strictly needed in that case.

Correct. I'll remove it if I make more changes to the PR.

erimatnor · 2020-11-25T16:51:26Z

src/version.c

+/* Export the strings to that we can read them using strings(1). We add a
+ * prefix so that we can easily find it using grep(1). We only bother about
+ * generating them if the relevant symbol is defined. */
+#ifdef EXT_GIT_COMMIT_HASH


Still unclear what happens if EXT_GIT_COMMIT_HASH is defined like this:

#define EXT_GIT_COMMIT_HASH

The command `get_git_commit` shows quite a little information and especially if mistakes are made it is hard to verify what the binary is actually based on. This commit extends the function to provide some more information: specifically the SHA of the HEAD regardless of whether there is a tag on it or not and the time for the HEAD commit. ``` postgres=# select * from _timescaledb_internal.get_git_commit(); commit_tag | commit_hash | commit_time --------------------------+-------------+------------------------ 1.7.4-10-g09b0b778-dirty | 09b0b77 | 2020-09-13 17:50:38+02 (1 row) ``` If git is not installed, the function `get_git_commit` will return an error indicating that no git information is available. If some of the fields are available, they will be emitted and the remaining fields will be NULL. Fixes timescale#2457

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It adds checks of configuration, adds support for gapfill on distributed tables, and improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, and contains improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view * timescale#2662 Save compression metadata settings on access node * timescale#2707 Introduce additional db for data node bootstrapping **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval * timescale#2698 Improve memory handling for remote COPY * timescale#2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view * timescale#2662 Save compression metadata settings on access node * timescale#2707 Introduce additional db for data node bootstrapping **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval * timescale#2698 Improve memory handling for remote COPY * timescale#2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * #2689 Check configuration in alter_job and add_job * #2696 Support gapfill on distributed hypertable * #2468 Show more information in get_git_commit * #2678 Include user actions into job stats view * #2664 Fix support for complex aggregate expression * #2672 Add hypertable to continuous aggregates view * #2662 Save compression metadata settings on access node * #2707 Introduce additional db for data node bootstrapping **Bugfixes** * #2688 Fix crash for concurrent drop and compress chunk * #2666 Fix timeout handling in async library * #2683 Fix crash in add_job when given NULL interval * #2698 Improve memory handling for remote COPY * #2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * timescale#2689 Check configuration in alter_job and add_job * timescale#2696 Support gapfill on distributed hypertable * timescale#2468 Show more information in get_git_commit * timescale#2678 Include user actions into job stats view * timescale#2664 Fix support for complex aggregate expression * timescale#2672 Add hypertable to continuous aggregates view * timescale#2662 Save compression metadata settings on access node * timescale#2707 Introduce additional db for data node bootstrapping **Bugfixes** * timescale#2688 Fix crash for concurrent drop and compress chunk * timescale#2666 Fix timeout handling in async library * timescale#2683 Fix crash in add_job when given NULL interval * timescale#2698 Improve memory handling for remote COPY * timescale#2555 Set metadata for chunks compressed before 2.0

This release candidate contains bugfixes since the previous release candidate, as well as additional minor features. It improves validation of configuration changes for background jobs, adds support for gapfill on distributed tables, contains improvements to the memory handling for large COPY, and contains improvements to compression for distributed hypertables. **Minor Features** * #2689 Check configuration in alter_job and add_job * #2696 Support gapfill on distributed hypertable * #2468 Show more information in get_git_commit * #2678 Include user actions into job stats view * #2664 Fix support for complex aggregate expression * #2672 Add hypertable to continuous aggregates view * #2662 Save compression metadata settings on access node * #2707 Introduce additional db for data node bootstrapping **Bugfixes** * #2688 Fix crash for concurrent drop and compress chunk * #2666 Fix timeout handling in async library * #2683 Fix crash in add_job when given NULL interval * #2698 Improve memory handling for remote COPY * #2555 Set metadata for chunks compressed before 2.0

mkindahl requested a review from feikesteenbergen September 30, 2020 08:09

mkindahl requested a review from a team as a code owner September 30, 2020 08:09

mkindahl requested review from pmwkaa, k-rus and WireBaron and removed request for a team September 30, 2020 08:09

pmwkaa approved these changes Sep 30, 2020

View reviewed changes

mkindahl marked this pull request as draft September 30, 2020 12:27

erimatnor reviewed Oct 1, 2020

View reviewed changes

mkindahl force-pushed the extend_git_commit branch 4 times, most recently from eb9b143 to 35f9f93 Compare October 2, 2020 08:29

mkindahl marked this pull request as ready for review October 2, 2020 08:29

mkindahl requested a review from erimatnor October 2, 2020 08:49

mkindahl force-pushed the extend_git_commit branch from 35f9f93 to 83e7bba Compare October 2, 2020 08:59

mkindahl changed the base branch from 1.7.x to master October 2, 2020 09:00

mkindahl force-pushed the extend_git_commit branch from 83e7bba to d0b9686 Compare October 2, 2020 09:01

mkindahl force-pushed the extend_git_commit branch 6 times, most recently from 7977c64 to 970d7dd Compare October 5, 2020 12:22

mkindahl marked this pull request as draft October 5, 2020 13:29

mkindahl force-pushed the extend_git_commit branch 2 times, most recently from d45e1d5 to 3e2e7cc Compare November 20, 2020 08:22

mkindahl requested a review from erimatnor November 20, 2020 08:32

erimatnor approved these changes Nov 25, 2020

View reviewed changes

mkindahl force-pushed the extend_git_commit branch 4 times, most recently from 85e9b2a to 5e0910a Compare November 25, 2020 14:26

erimatnor reviewed Nov 25, 2020

View reviewed changes

mkindahl force-pushed the extend_git_commit branch from 5e0910a to 453ee02 Compare November 26, 2020 20:19

mkindahl merged commit 1e7f961 into timescale:master Nov 26, 2020

mkindahl deleted the extend_git_commit branch November 26, 2020 20:43

mkindahl mentioned this pull request Dec 2, 2020

Release 2.0.0-rc4 #2702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show more information in get_git_commit #2468

Show more information in get_git_commit #2468

mkindahl commented Sep 30, 2020 •

edited

pmwkaa left a comment •

edited

erimatnor Oct 1, 2020 •

edited

mkindahl Oct 1, 2020

mkindahl Oct 2, 2020

mkindahl commented Oct 1, 2020

codecov bot commented Oct 2, 2020 •

edited

svenklemm commented Oct 2, 2020

svenklemm commented Oct 2, 2020

mkindahl commented Oct 5, 2020

erimatnor left a comment

erimatnor Nov 25, 2020

mkindahl Nov 25, 2020

erimatnor Nov 25, 2020

mkindahl Nov 26, 2020

erimatnor Nov 25, 2020

mkindahl Nov 25, 2020

erimatnor Nov 25, 2020

erimatnor Nov 25, 2020

mkindahl Nov 25, 2020

erimatnor Nov 25, 2020

mkindahl Nov 26, 2020

erimatnor Nov 26, 2020

erimatnor Nov 25, 2020

mkindahl Nov 25, 2020

erimatnor Nov 25, 2020

mkindahl Nov 25, 2020

erimatnor Nov 25, 2020

mkindahl Nov 26, 2020

erimatnor Nov 25, 2020

		@@ -5,7 +5,7 @@
		select pg_typeof(git) from _timescaledb_internal.get_git_commit() AS git;

Show more information in get_git_commit #2468

Show more information in get_git_commit #2468

Conversation

mkindahl commented Sep 30, 2020 • edited

pmwkaa left a comment • edited

Choose a reason for hiding this comment

erimatnor Oct 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkindahl commented Oct 1, 2020

codecov bot commented Oct 2, 2020 • edited

Codecov Report

svenklemm commented Oct 2, 2020

svenklemm commented Oct 2, 2020

mkindahl commented Oct 5, 2020

erimatnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkindahl commented Sep 30, 2020 •

edited

pmwkaa left a comment •

edited

erimatnor Oct 1, 2020 •

edited

codecov bot commented Oct 2, 2020 •

edited