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

DBZ-445 Make postGIS support optional. #9

Merged
merged 1 commit into from Nov 15, 2017

Conversation

QazerLab
Copy link

@QazerLab QazerLab commented Nov 8, 2017

Given that postGIS is an optional extension, it may be absent in some postgres installations. Having postGIS dependency mandatory seems strange, as it requires pretty enough transitive dependencies (something across 200MB).

This pull request adds ability to exclude postGIS support during build for systems without postGIS:

make still builds decoderbufs as before;
make USE_POSTGIS=false builds decoderbufs without postGIS support.

@gunnarmorling
Copy link
Member

Hi, thanks a lot for this contribution!

Two small remarks:

  • Could you also add an environment variable for the Postgres Docker images we provide and document that in the README.md for 9.6 and 10.0?
  • Could you reword the commit message so it starts with "DBZ-445 Made ..."

Thanks!

@QazerLab QazerLab changed the title Make postGIS support optional. DBZ-445 Make postGIS support optional. Nov 14, 2017
@QazerLab
Copy link
Author

Hi @gunnarmorling,

I've renamed both the commit and the PR to associate them with the ticket.

Regarding the changes in the Dockerfiles - I agree that the Dockerfiles should be modified to support this feature (and ready to create the appropriate PR). However, I'm not sure that doing this before the next release of the postgresql-decoderbufs is correct, because you specify the release tag in the environment (https://github.com/debezium/docker-images/blob/master/postgres/9.6/Dockerfile#L4) and use it during the build. What do you think about it?

@gunnarmorling
Copy link
Member

gunnarmorling commented Nov 14, 2017 via email

@rhauch
Copy link
Member

rhauch commented Nov 14, 2017 via email

@gunnarmorling
Copy link
Member

I don't think Debezium's PostgreSQL Docker image needs to be modified

Yeah, I was a bit confused about build time of the image and enabling/disabling extensions when running the database. Perhaps it'd be possible to optionally not load postGIS when using our image, but I'm not sure whether that's needed.

@QazerLab, when you said

it requires pretty enough transitive dependencies (something across 200MB).

does this refer to disk usage of the PG installation or memory usage of the process when running the database? In case of the former I think we can ignore that for our image, in case of the latter it may indeed make sense to have an option for not loading the GIS extension if not needed.

@rhauch
Copy link
Member

rhauch commented Nov 14, 2017

The Debezium PostgreSQL Docker image is really not for production, right? So we shouldn't really care too much about it's size. And TBH, I don't think it's worth the complication of trying not to load it.

Having said that, I think this PR is great and absolutely worthwhile for those that want to use it in their local PG installations without PostGIS.

@gunnarmorling
Copy link
Member

Yes, as said I wouldn't care about the image size. But memory consumption is something to keep an eye on.

But independently from that and as you say, this change is good, going to merge it now.

@gunnarmorling gunnarmorling merged commit 6c1723a into debezium:master Nov 15, 2017
@gunnarmorling
Copy link
Member

Rebased and applied. Thanks, @QazerLab!

@QazerLab
Copy link
Author

QazerLab commented Nov 15, 2017

@gunnarmorling, I was talking about the disk usage. However, my statement about 200 MB of transitive dependencies was based on the personal experience - I build CentOS-based image with decoderbufs, and in CentOS postGIS seems to have much more transitive dependencies than in Debian (these include even some GTK libs). I've checked Debian-based postgres:9.6 - here postGIS installation takes only 76 MB.

@gunnarmorling
Copy link
Member

Ok, thanks for reporting back then. Seems we're done then. Thanks again for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants