Bug Fix for reading the options from conf file and addition of one option recovery-till-timestamp

[himanchali]

Added module for delayed replication to restore till a given timestamp with new option recovery-till-timestamp .
Fixed the bug for reading options from conf file.
Added one script to update timestamp in conf file for recovery-till-timestamp option so that dbserver doesnt need to be restarted.

[ghost]

As is - it's not going to fly. Problems that I see:

1. I don't fully understand what this is about. It seems to be about recovery-till-timestamp. But if so - why can't recovery_target_time be used instead?
2. It adds dependency on non-core perl module. OmniPITR is built specifically so that it will not require any cpan modules to be installed - only core perl.
3. Commit himanchali@32f556c seems to contain some commented out debug things
4. Commit himanchali@9f62090 doesn't have any real explanation on what it is, and what for (and I don't understand what is the reason it does what it does)
5. script from commit himanchali@f9dc627 - i understand what it does but I don't understand why would we need it. What is the point of having such functionality scripted? Not to mention that it has issues like hardcoded path to bash, and requirement of particular version of (at least) sed - "sed -i ..." doesn't work in on solaris 10.

I think I understand what it's for, but I don't see why recovery.conf option of "recovery_target_time" is not sufficient. Plus - as it is, I can't merge it.

[PSUdaemon]

I've added some comments to specific pieces of code. I'll let Depesz's comments stand as I feel they have merit.

Also, even if you did plan to add this option, it seems like relying on timestamps of files is a flawed design. Using the option that Depesz suggested in recovery.conf will actually be based on the transaction timestamp.

[himanchali]

Answers inline for depesz comment:

I don't fully understand what this is about. It seems to be about recovery-till-timestamp. But if so - why can't recovery_target_time be used instead?
Ans: recovery_target_time can not be used as it recovers till a particular timestamp and then brings slave out of recovery. I needed some option where we can use both recovery_target_time and standby_mode = on , which was not possible in this case.
We had the requirement where we wanted replication done on slave everyday for a particular timestamp given,and leave it in recovery mode, like everyday till 10:00 PM and after that no change for whole day. In that case this new feature is required.

It adds dependency on non-core perl module. OmniPITR is built specifically so that it will not require any cpan modules to be installed - only core perl.
Ans: I have not installed any separate perl module to make it work on our system.

Commit himanchali/omnipitr@32f556c seems to contain some commented out debug things
Ans : Have fixed it in new commit.

Commit himanchali/omnipitr@9f62090 doesn't have any real explanation on what it is, and what for (and I don't understand what is the reason it does what it does)

Ans : This is a bug fix. Without this line , your read from config file for options doesn't work. If you want to read from config file for all the options instead of direct command line argument its needed. Reason, it was returing the value before '=' but was not returning after '=' .

script from commit himanchali/omnipitr@f9dc627 - i understand what it does but I don't understand why would we need it. What is the point of having such functionality scripted? Not to mention that it has issues like hardcoded path to bash, and requirement of particular version of (at least) sed - "sed -i ..." doesn't work in on solaris 10

Ans : As I told you the reuirement for this new option recovery_till_timestamp , So everytime you have to change the timestamp to a particular time in the conf file, so again it starts applies till there. You also dont need to restart db server in this case. We are using CentOS.

The option i introduced to fulfil our requirement and its working fine.

[himanchali]

Reply inline for PSUdaemon ;
Also, even if you did plan to add this option, it seems like relying on timestamps of files is a flawed design. Using the option that Depesz suggested in recovery.conf will actually be based on the transaction timestamp.

Ans : The option that Depesz suggested that just gives you a delayed replication always moving , it doesn't give you the option if someone wants the data till for a particular timestamp only everyday and use that remaining time. I agree that to use this option you have to make sure that the timestamp of log is exactly same as on source master where it got generated and it is not tough to achieve this.
Thx.

[PSUdaemon]

I think you are confusing the --recovery-delay option of omnipitr with http://www.postgresql.org/docs/9.1/static/recovery-target-settings.html#RECOVERY-TARGET-TIME .

With recovery_target_time and pause_at_recovery_target I think you can do what you want, however you will have to stop/start the standby DB daily.

[himanchali]

No I am not confusing with that, I had already tried that. What you suggested will work there , but as you pointed its needs a dbserver restart. and for a server running 24*7 it was not accepted. The main purpose of this option was to do the delayed replication for a given timestamp without affecting the dbserver.

[PSUdaemon]

Not having to restart the database seems like a reasonable requirement.

The timestamp seems kind of clunky. Is it possible to do a more cron style interface? Like stop at 10am every day? Also maybe use pg_last_xact_replay_timestamp() to determine how far you have replayed instead of file timestamps? That would require a connection to the DB and a newer (9.1+) version of postgres, however.

[himanchali]

As --recovery-delay also works on file timestamp, so had used same. It is also good approach as xlog timestamp on master gives you exact time and with rsync you can achieve same on slave host as well. Some cron style thing wont solve it as it will work on slave hosts timestamp , not on master's timestamp.

Using pg_last_xact_replay_timestamp() is better way obviously but before applying the changes on slave it will always require to hit to master, that is not good performance wise. In xlog file tiemstamp , master server not gets affected.

[PSUdaemon]

You should not hit the master for pg_last_xact_replay_timestamp(). It's for what has been replayed on the standby.

[ghost]

Well, let me understand what you need:
You need a way to make replication "pause" after reaching some time, but in a way that is "unpause-able".
This I understand, and it's not impossible to make. But:

1. Requirement to use non-core perl module is a show stopper immediataly. It's nice that you didn't have to install anything extra, but quick check in docs ( http://search.cpan.org/~rjbs/perl-5.16.3/pod/perlmodlib.PL ) shows that Date::Parse is not a part of even newest 5.16.3, not to mention earlier Perl releases (we supoprt since 5.8)
2. Addition of a script that is intended to change a line in a file, and in not-portable way, is definition not going to happen. This can be done using perl -pi '...', or any text editor. Adding specialized script to change single value in config file is impossible.
3. I see that you're using config files, which is great, but please note that this is not released code. Newest released version is 1.0.0, and it doesn't support config files. What's more - I did test the code that handles config files, and it did work fine for me, without the change that you suggested, but will look into it closely to see if I missed some edge case.

Summary:
I am perfectly open to add a patch that adds option to pause restoration after reaching given xlog mtime. Not in current form, though. Such feature, although definitely not common, is valuable, and if you wouldn't be willing to rewrite the code so that I could use it, I might write it myself (though not in short time frame).

[himanchali]

@depesz

I will rewrite the things as suggested. Will remove use of Date::Parse and Add a perl script to dynamic change any option in the conf file, not only single option.
Thanks.

[ghost]

Great. Please think whether you really think we need a script to do it (change values in config file). Maybe it's just overkill? Doing it by hand is simple. Or perhaps - since this is basically only for your option, think about providing another way of setting the border time.

For example, with the option "--recovery-till-timestamp" make it accept timestamp, or string "@filename" where timestamp would be read from mtime of given file? In such case, you could simply "touch" the file to make omnipitr proceed with recovery.

[himanchali]

Yea , using @filename and touch it when required also sounds a good idea, and it will work for my option. But I was thinking to have a separate script cause I can change other option as well dynamically, ( like recovery-delay might be on regular basis ). Also want to understand how it will be a overkill as there also i ll need to maintain another file.

[ghost]

It's overkill in terms of: it's a script to change a text value to another text value in text file. There are already many tools to do so: sed and perl are just the first two that come to my mind. I am not saying that it's bad idea. It's just that I have problem imagining what would such script bring to the table, that simple perl -pi -e 's/.../.../' wouldn't do.

[ghost]

In other news - I checked the fix for conflig files parsing that you sent, and you were 100% correct - there was a bug in OmniPITR core. Fixed - a bit differently than you suggested, but it does the job. Commit: 069e076

[ghost]

It was fixed some time ago.