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

Replication slots #41

Merged
merged 6 commits into from Mar 21, 2018
Merged

Replication slots #41

merged 6 commits into from Mar 21, 2018

Conversation

@zilder
Copy link
Collaborator

@zilder zilder commented Mar 13, 2018

No description provided.

zilder added 2 commits Mar 13, 2018
@codecov-io
Copy link

@codecov-io codecov-io commented Mar 13, 2018

Codecov Report

Merging #41 into master will increase coverage by 0.24%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   97.02%   97.27%   +0.24%     
==========================================
  Files          16       16              
  Lines        1245     1356     +111     
==========================================
+ Hits         1208     1319     +111     
  Misses         37       37
Impacted Files Coverage Δ
testgres/backup.py 96.92% <100%> (ø) ⬆️
tests/test_simple.py 99.77% <100%> (+0.02%) ⬆️
testgres/consts.py 100% <100%> (ø) ⬆️
testgres/node.py 97.55% <84.21%> (+0.28%) ⬆️
testgres/enums.py 100% <0%> (ø) ⬆️
testgres/connection.py 95% <0%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bc608e...59dbe42. Read the comment docs.

@funbringer funbringer requested review from ildus and funbringer Mar 14, 2018
).format(slot_name)

self.execute(query=query,
dbname=dbname or default_dbname(),

This comment has been minimized.

@funbringer

funbringer Mar 14, 2018
Collaborator

You don't have to explicitly use default_dbname() and such since execute() method is able to handle None args properly.

This comment has been minimized.

@zilder

zilder Mar 20, 2018
Author Collaborator

Agree

u"log_statement = {}\n"
u"listen_addresses = '{}'\n"
u"port = {}\n"
u"max_replication_slots = {}\n".format(log_statement,

This comment has been minimized.

@funbringer

funbringer Mar 14, 2018
Collaborator

IMHO max_replication_slots should be placed under allow_streaming=True, next to max_wal_senders etc.

This comment has been minimized.

@zilder

zilder Mar 20, 2018
Author Collaborator

Agree as well

@@ -856,7 +863,24 @@ def backup(self, **kwargs):

return NodeBackup(node=self, **kwargs)

def replicate(self, name=None, **kwargs):
def create_replication_slot(self, slot_name, dbname=None, username=None):

This comment has been minimized.

@funbringer

funbringer Mar 14, 2018
Collaborator

Should we allow user to choose his own slot_name? If not, it's better to inline this method.

This comment has been minimized.

@zilder

zilder Mar 20, 2018
Author Collaborator

If we don't allow this, then it could contradict with logical replication interface (#42) which allows user to choose the subscriber name (which implicitly creates logical replication slot with the same name) voluntarily.

Speaking of inlining, I can call create_replication_slot() from replicate() if slot_name != None so that user won't need to call it explicitly. It would be more convenient for user.

This comment has been minimized.

@funbringer

funbringer Mar 20, 2018
Collaborator

@zilder You're right, of course. Maybe we'll add a default name generator later.

Speaking of inlining, I can call create_replication_slot() from replicate() if slot_name != None

Sure, that would be nice. It's better to check if slot exists, though.

This comment has been minimized.

@zilder

zilder Mar 20, 2018
Author Collaborator

It's better to check if slot exists, though.

True

@funbringer
Copy link
Collaborator

@funbringer funbringer commented Mar 21, 2018

Please make sure that code won't be rewritten by yapf.

@funbringer funbringer merged commit b4051cf into postgrespro:master Mar 21, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.