Skip to content

Conversation

@mkgrgis
Copy link

@mkgrgis mkgrgis commented Sep 17, 2025

Hello, Dave @dpage !
Hello, Andrew @adunstan !

Thanks for this very good FDW! I want to made this FDW better and introduce best industrial CI practice here.

This PR contains:

  1. github full automated multiversional CI based on Ubuntu for PostgreSQL 14..18
  2. New diagnostic helper SQL functions redis_fdw_hiredis_version() and redis_fdw_version() for debug and bugreports.
  3. Unified FDW code across PostgreSQL 14..18. Note: Between Pg 13 and Pg 14 there was a big function refactoring, hence there is reason for separate unified branch for older versions.
  4. Testing environment getting scripts in test/local multiversional tests with README instruction
  5. Refactored Makefile based on CI and common FDW samples such as SQLite FDW, InfluxDB FDW, Oracle FDW, Firebird FWD etc.
  6. Unified PostgreSQL license for automated github parsing, not only string in code files.

CI process based on made by Toshiba corporation and used for SQLite FDW.

CI tests covers PostGIS and "no gis" test cases because Redis implements spatial functions and use WKT spatial format. Hence integration with PostGIS is not hard in future PRs.

Test set is equal for different PostgreSQL versions, but test output can be different. For example, there is no PostGIS support for PostgreSQL 18RC1.

I think this CI can allow speed up PR merging and backporting without any code problems. For example I am preparing encoding support PR and there is some other useful PRs like #41.

@dpage
Copy link

dpage commented Sep 22, 2025

Hi,

This PR includes changes that are logically separate from one another. Please break it up into individual PRs each addressing a single change/feature/improvement.

In a very quick look at the changes I did notice that the licence file attributes copyright to Andrew and I. Please use "The redis-fdw Development Team" instead, as that will cover all contributors including yourself.

Thanks.

@dpage dpage closed this Sep 22, 2025
@mkgrgis
Copy link
Author

mkgrgis commented Sep 23, 2025

@dpage, please give me your point of view about logically separated parts of this PR.
My view is 1 is impossible without 5 and unusual without results of 2. Also 3 (unified FDW code like for other FDWs) is the only possible base of CI testing. Hence I can separate 6 and 4 without any problems. Will this enough?

@dpage
Copy link

dpage commented Sep 23, 2025

That just means we need to handle PRs in order. I don't see any reason why a PR can't note that it's dependent on another one being applied first.

@mkgrgis
Copy link
Author

mkgrgis commented Sep 23, 2025

OK. Here will be a group of small but not functionally PRs. 3 -> 2 -> 1+5 -> 6 -> 4.

@mkgrgis
Copy link
Author

mkgrgis commented Sep 23, 2025

@dpage , there are #43 as 3 and #44 as 2. Let's review.

@mkgrgis
Copy link
Author

mkgrgis commented Oct 1, 2025

@dpage , 6 is done as #46.

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