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

Read PostgreSQL port from PGDATA folder #340

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@PierreDucroquet

PierreDucroquet commented Jun 2, 2017

Hi

During a routine check of my backups, I found out that wal-e does its base backup using psql with no parameter, thus using only the default cluster. Since I have several PostgreSQL versions on the same server, it means that the backups are complete only for the default one.
That behaviour is not documented and easy to fix by reading postmaster.pid and extracting the cluster port from that file.
This patch implements this behaviour.

Ref #326

@fdr

Clever idea, all in all. What do you think about checking show data_directory to find and/or use a matching cluster?

Show outdated Hide outdated wal_e/operator/backup.py Outdated
@fdr

This comment has been minimized.

Show comment
Hide comment
@fdr

fdr Jun 6, 2017

Member

This should be overriden by a PGPORT environment variable. That's how people normally handle multiple/non-default clusters.

Member

fdr commented Jun 6, 2017

This should be overriden by a PGPORT environment variable. That's how people normally handle multiple/non-default clusters.

@PierreDucroquet

This comment has been minimized.

Show comment
Hide comment
@PierreDucroquet

PierreDucroquet Jun 8, 2017

Using a PGPORT environment variable is the usual way, but since the documentation never mentions it and the information is in the PGDATA folder, one could (wrongly) assume it was not required.

Do you suggest I add a check that psql connects to the server that has the right data_directory ?
Aka. something like

if psql_csv_run('show data_directory')[0] != args.data_directory:
    raise InvalidArgument()
elif get_port_from_pgdata(args.data_directory) != env['PGPORT']:
    raise InvalidArgument()

And if PGPORT is not specified in environment, continue extracting it from data directory and put it in env ?

PierreDucroquet commented Jun 8, 2017

Using a PGPORT environment variable is the usual way, but since the documentation never mentions it and the information is in the PGDATA folder, one could (wrongly) assume it was not required.

Do you suggest I add a check that psql connects to the server that has the right data_directory ?
Aka. something like

if psql_csv_run('show data_directory')[0] != args.data_directory:
    raise InvalidArgument()
elif get_port_from_pgdata(args.data_directory) != env['PGPORT']:
    raise InvalidArgument()

And if PGPORT is not specified in environment, continue extracting it from data directory and put it in env ?

@fdr

This comment has been minimized.

Show comment
Hide comment
@fdr

fdr Jun 19, 2017

Member

I do suggest such check. Sorry for the delay.

Member

fdr commented Jun 19, 2017

I do suggest such check. Sorry for the delay.

@zvolsky

This comment has been minimized.

Show comment
Hide comment
@zvolsky

zvolsky Jun 30, 2017

Mr. PierreDucroquet, could you mention in your blog http://www.pinaraf.info/2017/06/wal-e-and-the-gotcha-how-i-nearly-lost-50-of-my-backups/,
that the standard way how to do things is /etc/wal-e.d/env/PGPORT ?
It is not the best idea to let some misleading information (about a relative special topic) in an "IT blog", but with closed comment possibility.
Thank you.

zvolsky commented Jun 30, 2017

Mr. PierreDucroquet, could you mention in your blog http://www.pinaraf.info/2017/06/wal-e-and-the-gotcha-how-i-nearly-lost-50-of-my-backups/,
that the standard way how to do things is /etc/wal-e.d/env/PGPORT ?
It is not the best idea to let some misleading information (about a relative special topic) in an "IT blog", but with closed comment possibility.
Thank you.

@PierreDucroquet

This comment has been minimized.

Show comment
Hide comment
@PierreDucroquet

PierreDucroquet Jun 30, 2017

@zvolsky No offense, but I do mention that you can put the PGPORT in the environment variables. I do not say that wal-e has a bug (grep for that word, you will not find it) but instead a "gotcha", a tiny trap that is never mentioned anywhere in the documentation nor the code and that is misleading since the port can be found in the PGDATA folder. Please point out specifically what bother you in that blog post which main aim was to be informative and point out a serious trap that at least some of my DBA friends were not aware of.
(BTW, comments are closed because we are past x days after the post and I found it's the easiest way to stop spam... sorry about that)

PierreDucroquet commented Jun 30, 2017

@zvolsky No offense, but I do mention that you can put the PGPORT in the environment variables. I do not say that wal-e has a bug (grep for that word, you will not find it) but instead a "gotcha", a tiny trap that is never mentioned anywhere in the documentation nor the code and that is misleading since the port can be found in the PGDATA folder. Please point out specifically what bother you in that blog post which main aim was to be informative and point out a serious trap that at least some of my DBA friends were not aware of.
(BTW, comments are closed because we are past x days after the post and I found it's the easiest way to stop spam... sorry about that)

Extract port from PGDATA/postmaster.pid and check it
This checks the PGPORT environment variable for consistency
with the PGPORT reported by the PostgreSQL cluster.
@PierreDucroquet

This comment has been minimized.

Show comment
Hide comment
@PierreDucroquet

PierreDucroquet Jun 30, 2017

@fdr I have rewritten the patch as you suggested, removing the extra parameter and instead just checking for consistency.
Do you require other changes ?

PierreDucroquet commented Jun 30, 2017

@fdr I have rewritten the patch as you suggested, removing the extra parameter and instead just checking for consistency.
Do you require other changes ?

@deverant

This comment has been minimized.

Show comment
Hide comment
@deverant

deverant Jun 6, 2018

Member

Reviving old threads. I think this looks good but fails some stylechecks, etc. @PierreDucroquet do you mind rebasing this and fixing the test errors? I am happy to merge this right after that. If not that's fine, I might just do it myself and give you the credit. :)

It seems like the postmaster.pid format is pretty stable so I think it's fine to read the data from there.

Member

deverant commented Jun 6, 2018

Reviving old threads. I think this looks good but fails some stylechecks, etc. @PierreDucroquet do you mind rebasing this and fixing the test errors? I am happy to merge this right after that. If not that's fine, I might just do it myself and give you the credit. :)

It seems like the postmaster.pid format is pretty stable so I think it's fine to read the data from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment