Skip to content

Conversation

@mkgrgis
Copy link

@mkgrgis mkgrgis commented Sep 23, 2025

Refactor testing system and Makefile for multi-versional CI tests. Extensions are declared directly in tests, not in testing options. Redis scripts was moved from sql to common testing directory.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage , any questions?

Copy link

@dpage dpage left a comment

Choose a reason for hiding this comment

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

The tests are tied to specific minor versions of PostgreSQL, which makes running them quite impractical:

make USE_PGXS=1 installcheck
echo "# +++ regress install-check in  +++" && /opt/homebrew/lib/postgresql@17/pgxs/src/makefiles/../../src/test/regress/pg_regress --inputdir=./ --bindir='/opt/homebrew/Cellar/postgresql@17/17.4_1/bin'    --encoding=utf8 --inputdir=test --outputdir=test --dbname=contrib_regression 17.4/redis_fdw 17.4/nogis
# +++ regress install-check in  +++
# using postmaster on Unix socket, default port
/bin/sh: /Users/dpage/git/redis_fdw/test/sql/17.4/redis_fdw.sql: No such file or directory
diff: /Users/dpage/git/redis_fdw/test/expected/17.4/redis_fdw.out: No such file or directory
# diff command failed with status 512: diff  "/Users/dpage/git/redis_fdw/test/expected/17.4/redis_fdw.out" "/Users/dpage/git/redis_fdw/test/results/17.4/redis_fdw.out" > "/Users/dpage/git/redis_fdw/test/results/17.4/redis_fdw.out.diff"
Bail out!make: *** [installcheck] Error 2

Please update so they're only tied to the major version of the database server, and therefore won't break following package updates.

There are no specific TCs for pg minor versions
@mkgrgis mkgrgis requested a review from dpage October 1, 2025 10:00
@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

No problems with 17.4, @dpage . 18.0 also updated here because was released during PR waiting.

Copy link

@dpage dpage left a comment

Choose a reason for hiding this comment

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

Your change simply makes the tests work with the current minor releases of PostgreSQL. We need to remove any hard-coded minor release versions entirely, otherwise we'll have to update the code every time there's a minor release, and ensure that any systems on which tests are run are updated to the new releases at the same time. That's just not practical.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage , how should CI scripts determine exact PostgreSQL git branch for downloading and testing? Does any other FDW use not exact pg 17.4, but "some of pg 17 branch"?

we'll have to update the code every time there's a minor release, and ensure that any systems on which tests are run are updated to the new releases at the same time. That's just not practical.

Yes, this is not very practical, but @pgspider team experience allows us to ensure, there are no breakable changes between minor pg updates. This not very practical algorithm is a cost of any in-time PostgreSQL development.

UPD. We can automate minor version changes/releases from git.postgrsql.org.

@mkgrgis mkgrgis mentioned this pull request Oct 1, 2025
@dpage
Copy link

dpage commented Oct 1, 2025

@dpage , how should CI scripts determine exact PostgreSQL git branch for downloading and testing? Does any other FDW use not exact pg 17.4, but "some of pg 17 branch"?

I don't know what other FDWs or extensions do - I've not previously had to deal with different output for different server versions - in fact, is that actually an issue here anyway? If so, why?

we'll have to update the code every time there's a minor release, and ensure that any systems on which tests are run are updated to the new releases at the same time. That's just not practical.

Yes, this is not very practical, but @pgspider team experience allows us to ensure, there are no breakable changes between minor pg updates. This not very practical algorithm is a cost of any in-time PostgreSQL development.

Not at all - the fact that PostgreSQL goes to such lengths to ensure compatibility is always maintained in minor releases makes this into a non-problem. We don't concern ourselves with minor versions at all typically; our CI runners can just install "postgresql18" and everything will work with whatever the current minor version is. Look at the pgAdmin regression tests for example - where different platforms/server versions are relevant, they care only about the major PostgreSQL version, e.g.

jobs:
  run-python-tests-pg:
    strategy:
      fail-fast: false
      matrix:
        os: [macos-latest, ubuntu-22.04, windows-latest]
        pgver: [13, 14, 15, 16, 17, 18]

    runs-on: ${{ matrix.os }}

    steps:

<snip>

      - name: Setup the PGDG APT repo on Linux
        if: ${{ matrix.os == 'ubuntu-22.04' }}
        run: |
          sudo sh -c 'echo "deb https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list'
          wget --quiet -O - https://www.postgresql.org/media/keys/ACCC4CF8.asc | sudo apt-key add -

<snip>

      - name: Install platform dependencies on Linux
        if: ${{ matrix.os == 'ubuntu-22.04' }}
        run: |
          sudo apt update
          sudo apt install -y libpq-dev libffi-dev libssl-dev libkrb5-dev zlib1g-dev postgresql-${{ matrix.pgver }} postgresql-${{ matrix.pgver }}-pldebugger pgagent postgresql-${{ matrix.pgver }}-postgis-3

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

... is that actually an issue here anyway? If so, why?

This can be a problem after some minor-specific additions to out FDW. In Redis FDW there are no such features, but this is possible.

Not at all - the fact that PostgreSQL goes to such lengths to ensure compatibility is always maintained in minor releases makes this into a non-problem.
...
our CI runners can just install "postgresql18"

This is package installation, but proposed CI use PostgreSQL C compilation against some version with possible user's C experiments (this is used by companies like EDB).
Package CI and C code CI is not the same if some base PostgreSQL versions are (still) not prepared as .deb. For example, we can test release-candidate only as compilable C code, not as package. In our case releases-candidates testing is useful for preparing to a new major PosrgreSQL release.

@dpage
Copy link

dpage commented Oct 1, 2025

... is that actually an issue here anyway? If so, why?

This can be a problem after some minor-specific additions to out FDW. In Redis FDW there are no such features, but this is possible.

It should never be a problem.

Not at all - the fact that PostgreSQL goes to such lengths to ensure compatibility is always maintained in minor releases makes this into a non-problem.
...
our CI runners can just install "postgresql18"

This is package installation, but proposed CI use PostgreSQL C compilation against some version with possible user's C experiments (this is used by companies like EDB). Package CI and C code CI is not the same if some base PostgreSQL versions are (still) not prepared as .deb. For example, we can test release-candidate only as compilable C code, not as package. In our case releases-candidates testing is useful for preparing to a new major PosrgreSQL release.

I'm quite familiar with EDB's processes for building Postgres and related code, having been responsible for them for many years. Their processes are different because they are building a suite of product components that are distributed together.

This project is not doing that - we are simply concerned with testing our own code against whatever major version of PostgreSQL is installed on our development machines or gets installed onto a CI runner.

I will not be merging any PRs that require specific minor versions of PostgreSQL, or require PostgreSQL or other components that are not built from the code in this repo to be compiled at test time.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

Ok, @dpage. I'll try to change testing version selecting here based on major version latest release. Maybe REL_**_STABLE will be useful. Look like this touch testing folders names only.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage , will this stable branches from https://github.com/postgres/postgres/branches enough for multi-versional testing?

@dpage
Copy link

dpage commented Oct 1, 2025

@dpage , will this stable branches from https://github.com/postgres/postgres/branches enough for multi-versional testing?

That's probably fine, though I'd just hard-code that list of versions myself. It'll be less code, and will only need to be updated annually (when we're ready to do so).

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage, CI was adopted to git clone after REL_**_SATBLE git branch in complex with f3fb59e

@dpage
Copy link

dpage commented Oct 1, 2025

@dpage, CI was adopted to git clone after REL_**_SATBLE git branch in complex with f3fb59e

As I've said in two PRs now, please just install the release packages that end users would be using for anything other than redis_fdw. Building from source overcomplicates things, will significantly increase the runtime, and means we're not testing against any of the same builds that end users are likely to be using.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage, do you want CI only for Ubuntu and hard-adoptable to other Unix environments? What about future PostGIS tests in this case? Package CI will absolutely not flexible. Spatial testing between PostGIS, Redis FDW, hiredis and Redis will absolutely not flexible in this case and determined by Ubuntu package combinations, but in other distributives and Unix environments can be other combinations of versions.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage , I have understood usually CI for you. Let's I share my thought.

There is such CI kind as:

  1. Pre-package CI for a distributive with all pre-compiled .deb, .rpm etc. dependencies. This CI confirms "if this code is compilable with this environment". This is your choice.
  2. Industrial contributor's CI. This CI confirms if is compilable some complex of a software products. For example for SQLite FDW this compilable complex includes such C software products as (1) PostgreSQL, (2) FDW as SQLite FDW themself, (3) foreign client as SQLite and (4) spatial processor as PostGIS. Please note, actual SQLite version for testing in current SQLite FDW CI is 3.49.**, but Ubuntu repos contains only 3.47 and lower versions.

In experimental contributor's branches for touching version support limits pre-package CI for a distributive is absolutely unusable, because tested version combinations of dependencies of the code cannot (yet) be presented in repos. Industrial CI is better for everyday using, but Pre-package CI for a distributive is usefully only for releases.

What do you think about described CI difference?

@dpage
Copy link

dpage commented Oct 2, 2025

@dpage , I have understood usually CI for you. Let's I share my thought.

There is such CI kind as:

  1. Pre-package CI for a distributive with all pre-compiled .deb, .rpm etc. dependencies. This CI confirms "if this code is compilable with this environment". This is your choice.
  2. Industrial contributor's CI. This CI confirms if is compilable some complex of a software products. For example for SQLite FDW this compilable complex includes such C software products as (1) PostgreSQL, (2) FDW as SQLite FDW themself, (3) foreign client as SQLite and (4) spatial processor as PostGIS. Please note, actual SQLite version for testing in current SQLite FDW CI is 3.49.**, but Ubuntu repos contains only 3.47 and lower versions.

In experimental contributor's branches for touching version support limits pre-package CI for a distributive is absolutely unusable, because tested version combinations of dependencies of the code cannot (yet) be presented in repos. Industrial CI is better for everyday using, but Pre-package CI for a distributive is usefully only for releases.

What do you think about described CI difference?

I have made it clear I believe it is most important to test against the package builds and combinations that end users are most likely to be running. If a patch is submitted to implement CI tests along those lines, I will happily review it when I have time. I will not review or commit a patch that builds dependencies from source unnecessarily, nor am I going to debate the pros and cons of doing so further.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 2, 2025

@dpage , ok. What about this tests without any CI? Here (in the PR) are no CI scripts, only test .sql and expected results. What about content of this files?

@mkgrgis
Copy link
Author

mkgrgis commented Oct 3, 2025

@dpage , look like I have example of universal tests. This PR can be closed after I'll get CI success with universal tests.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 7, 2025

This PR is closed because it's still possible to implement identical tests for all PG versions without big Makefile changes. Discussion about CI will be continued in #47.

@mkgrgis mkgrgis closed this Oct 7, 2025
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