Skip to content

Conversation

@easel
Copy link
Contributor

@easel easel commented Oct 28, 2020

No description provided.

Copy link
Owner

@hartmut-co-uk hartmut-co-uk left a comment

Choose a reason for hiding this comment

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

Many thanks, looks good to me - though I'm currently not actively using this anywhere so I can't actually test E2E.

Please have a look at the inline comment.

README.md Outdated
### run (example)

docker run -e PGPASSWORD=supersecure -it --rm hartmut-co-uk/pg-repack-docker pg_repack -h localhost -U dbroot --dbname=dbname --dry-run --table=table1 --only-indexes --no-superuser-check
docker run -it --rm hartmut-co-uk/pg-repack-docker -h localhost -U dbroot --dbname=dbname --dry-run --table=table1 --only-indexes --no-superuser-check
Copy link
Owner

Choose a reason for hiding this comment

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

Why remove the example on how to set the password?
Any particular reason or better alternative?

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 dropped the example because the change to the entrypoint avoids the containers default postgres startup which fails for me without the password set. This allows pg_repack to prompt for the password. It occurs to me that pg_repack might pick up the password from the environment variable though, so I could go either way.

Copy link
Owner

Choose a reason for hiding this comment

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

Would you also be able to test by any chance?
The use case I've had back then for a client was 'scripted' with a cron job - so it was quite essential to not have a prompt.. so I'd like to keep the example for reference.

What do you think?

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 went ahead and adjusted the README to include the password-via-environment and also added --network=host on the assumption that one is probably trying to connect to a database on the local development machine when referencing localhost. It does work for me. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invariably, my local db has a difference version pf pg_repack and I get this:

ERROR: pg_repack failed with error: pg_repack 1.4.3 is not installed in the database

In my opinion the primary utility of this container is that pg_repack is very very picky about alignment between postgres and pg_repack versions, and on a normal client machine it can be pretty tricky to get all the right versions worked out to be able to call pg_repack on a server. The docker container locks all that down so it works consistently.

@hartmut-co-uk
Copy link
Owner

Quick note: after further consideration I've rolled back the ENTRYPOINT ["pg_repack"] - so the default behaviour is to actually run postgres (with pg_repack extension) with the container. See updated README.

@hartmut-co-uk
Copy link
Owner

Many thanks for the contribution!

@hartmut-co-uk hartmut-co-uk merged commit e39e54d into hartmut-co-uk:master Nov 1, 2020
@easel easel deleted the entrypoint branch November 1, 2020 23:23
@easel
Copy link
Contributor Author

easel commented Nov 1, 2020

Looks good. The issue I ran into is that even when running pg_repack it will try to start postgres, since it's in the entrypoint. This will fail if you don't have PGPASSWORD set because it can't initialize the postgres server. Perhaps the solution is to add a note to the README indicating the necessity?

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