matthewfranglen / postgres-elasticsearch-fdw

Postgres to Elastic Search Foreign Data Wrapper
MIT License
111 stars 32 forks source link

Add table option "es_version". #40

Closed LouisFunmula closed 5 months ago

LouisFunmula commented 5 months ago

This pull request has the following modifications:

  1. Support ES8 #35
  2. Add es_version option. When we have different versions of ES clusters, we can set the es_version option in the foreign table option.
  3. Add es_version_option to tests/run.py, to test table with option
  4. In the ES_PIP_VERSIONS images, the elasticsearch module does not match the server version.
  5. Updated postgres images. I failed to build the images of old PG versions and I can't update them. Therefore I removed them from Makefile.
matthewfranglen commented 5 months ago

Hi.

I'm sorry but I do not agree with the intent of this PR. You are installing all client ES versions and relying on this new option to differentiate between them, which would likely break this fdw for every existing user.

Furthermore this is not required. The PR from yosva added the scheme parameter and the ES8 client works with that https://github.com/matthewfranglen/postgres-elasticsearch-fdw/pull/38 which they confirm works here (on the very issue that you link).

The installation instructions for this explicitly state that the correct version of the elasticsearch client must be installed. This is reasonable as the postgres fdw relies upon globally installed packages which mean the database environment should be isolated (e.g. in docker or by having specific dedicated database machines).

Thank you for your interest in this project. I don't really maintain this any more so if you wish to fork it and maintain it yourself then I fully encourage you to do so.

LouisFunmula commented 5 months ago

Hi,

Sorry I didn't explain it clearly. If the table option dosen't has "es_version" option, it loads elasticsearch module in the original way. (If it dosen't, there is something wrong in my code. I should fix it).

This option is required in our environment. Because we have multiple elasticsearch clusters in different versions, include very old one. This option allow us to setup fdw connecting to them in one PG server.

BTW, Update query is also fixed in this PR, in only two lines. Shall I create another PR for it?

matthewfranglen commented 5 months ago

Hey, is the update fix this: https://github.com/matthewfranglen/postgres-elasticsearch-fdw/pull/40/files#diff-59661246e5cf9359efe962d490c85cc43198dc07e2bf49175c88840a118219caR130-R143 where the kwarg changes from doc to body?

I now better understand your requirements. Unfortunately I don't think that is behaviour that I want to integrate into the base fdw as it adds quite a lot of complexity to the code.

For your own implementation you may benefit from implementing es client specific versions of the fdw which each just change the specific part that they need to. Then the multicorn wrapper class can just delegate all operations to the es specific code. Something like:

class ElasticsearchFDW(ForeignDataWrapper):
    def __init__(...):
        ...
        if elasticsearch_version == 8:
            self.implementation = Elasticsearch8Wrapper(...)
        elif elasticsearch_version == 7:
            self.implementation = Elasticsearch7Wrapper(...)
        ...
    def execute(...):
        return self.implementation.execute(...)
matthewfranglen commented 5 months ago

oof did I really use index instead of update in the update method :facepalm:

matthewfranglen commented 5 months ago

Right I've updated the update method and published a new version, 0.12.0. This version properly supports Elasticsearch 8. Good luck with implementing your overrides, and thanks for taking the time to open this PR.

matthewfranglen commented 5 months ago

oh and FYI if you make a GET request to the root of the elasticsearch server then you get back a response that includes the version number. I do that here as elasticsearch 8 introduced a breaking change to the bulk actions I use as part of the tests. You may be able to use this to validate / select the elasticsearch client version to use.