Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds pg 15 beta support #956

Closed
wants to merge 10 commits into from
Closed

Adds pg 15 beta support #956

wants to merge 10 commits into from

Conversation

gurkanindibay
Copy link
Contributor

@gurkanindibay gurkanindibay commented Aug 5, 2022

This branch is a temporary branch to be able to handle pg 15 temporary changes especially --without-pg-version-check flag
Since it includes temporary flag, it will not be merged into all-citus

@@ -21,6 +21,7 @@ base:
- ".*: warning: 'RSA_generate_key_ex' is deprecated \\[-Wdeprecated-declarations\\]"
- ".*: warning: 'EVP_PKEY_assign' is deprecated.*"
- "\\d+ warnings generated."
- "configure: WARNING: unrecognized options: --without-pg-version-check"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this line? We added support for this in citusdata/citus#6072 that is now in main branch.

If this option is not recognized, we will fail to build Citus on PG15 and we will have errors in later steps of our builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are adding this warning temporarily. @onderkalaci told me to use this flag in the builds. You can see that I added that flag into both deb rule file and rpm spec file to be able to build pg 15.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have support for this option. If we observe such a warning there is something wrong with our builds. I agree that we should update debian/rules and citus.spec files. However I do not think that we should ever allow configure to throw this warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actualy does not understand what you mean. We do not throw an error specifically for this warning. We have a warning detector machanism which throws error other than items in packaging-ignore file. Therefore since this warning appears in the logs because we add this flag into our configuration, without adding this warning into ignore file, warning detector throws error.

- 11.0:
postgres_versions: [13,14]
postgres_versions: [ 13, 14, 15 ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe 11.1 will be the first version to support PG15.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this for nightlies. After 11.1 release I will change it. This is for beta process

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use 11.1devel version on the main branch.

  1. Why do you need to update entries for 11.0 for nightlies? Should not that be 11.1?
  2. Would not this change cause problems if we want to release a patch version for 11.0 that does not and will not support PG15.

I suggest we add 11.1 here and make sure nightlies actually use the values for that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current structure does not work like that. Nightlies are being processed seperately and evaluated with the current branch. Actually we do not make a version check since it is on main branch.

Comment on lines +1 to +5
exclude:
nightly:
ol/7: [15]
release:
all: [15]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving this file and merging it with some of the current files that we already have? See citusdata/tools#238 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we will not need this file quite common so I prefer not adding this option into postgres_matrix file. Additionally, postgres matrix includes all versions which is some sort of a citus postgres version support matrix file. This file will be very long and adding additional things will make the file more difficult to read.
However, it is not a bad idea. If it is needed, I need to open a PR about that and merge them in the future since there are more prioritized tasks waiting my attention in my work queue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it is not too much of a hassle to concatenate the two files. I do not think that you need to update the structure.

How about the following suggestion for postgres-matrix.yml:

name: Postgres Version Matrix
project_name: citus

version_matrix:
  - 8.0:
      postgres_versions: [ 10, 11 ]
  - 9.0:
      postgres_versions: [ 11, 12 ]
  - 9.5:
      postgres_versions: [ 11, 12, 13 ]
  - 10.1:
      postgres_versions: [ 12, 13 ]
  - 10.2:
      postgres_versions: [ 12, 13, 14 ]
  - 11.0:
      postgres_versions: [ 13, 14 ]
  - 11.1:
      postgres_versions: [ 13, 14, 15 ]

exclude:
  nightly:
    ol/7: [ 15 ]
  release:
    all: [ 15 ]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it can be like that but I wanted to keep postgres-matrix.yml for only versions.
I opened an issue about that. I could start this issue after my prioritized tasks
#958

citus.spec Outdated
@@ -44,7 +44,7 @@ if [ "$(printf '%s\n' "$requiredgccver" "$currentgccver" | sort -V | head -n1)"
exit 1
fi

%configure PG_CONFIG=%{pginstdir}/bin/pg_config --with-extra-version="%{?conf_extra_version}" --with-security-flags CC=$(command -v gcc)
%configure PG_CONFIG=%{pginstdir}/bin/pg_config --with-extra-version="%{?conf_extra_version}" --with-security-flags CC=$(command -v gcc) --without-pg-version-check
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this check with the direction of @onderkalaci to be able to build nightlies for pg15

debian/rules Outdated
@@ -15,7 +15,7 @@ override_dh_auto_test:
# nothing to do here, see debian/tests/* instead

override_dh_auto_configure:
+pg_buildext configure build-%v --with-extra-version="$${CONF_EXTRA_VERSION:-}" --with-security-flags
+pg_buildext configure build-%v --without-pg-version-check --with-extra-version="$${CONF_EXTRA_VERSION:-}" --with-security-flags
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this check with the direction of @onderkalaci to be able to build nightlies for pg15

@gurkanindibay
Copy link
Contributor Author

@hanefi I addressed all reviews. Could you check the PR again?

hanefi
hanefi previously approved these changes Aug 15, 2022
Copy link
Member

@hanefi hanefi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like the current design, but I will approve this in the hopes that we fix it soon. The main problem is captured in #958

@gurkanindibay gurkanindibay marked this pull request as draft August 15, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants