Re: [HACKERS] LSN as a recovery target
On Mon, Sep 5, 2016 at 4:02 PM, Simon Riggs wrote: > On 5 September 2016 at 06:55, Michael Paquier > wrote: >> On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs wrote: > >>> I noticed we don't mention what LSN is anywhere, so I'd like to apply >>> the following doc patch also. >> >> +1 for the idea. What do you think about adding a mention to the >> system data type pg_lsn? >> >> >> + WAL records are appended to the WAL >> + logs as each new record is written. The insert position is described by >> + a Log Sequence Number (LSN) that is a byte offset into >> + the logs, increasing monotonically with each new record. Two >> + LSNs can be compared to calculate the volume of >> + WAL data that separates them, so are used to measure >> + progress of WAL during replication and recovery. >> + >> Here we could for example append a sentence like "The system data type >> pg_lsn is an internal >> representation of the LSN". > > Good input, thanks. v2 attached. Looks good to me. Thanks for the wordsmithing. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 5 September 2016 at 06:55, Michael Paquier wrote: > On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs wrote: >> I noticed we don't mention what LSN is anywhere, so I'd like to apply >> the following doc patch also. > > +1 for the idea. What do you think about adding a mention to the > system data type pg_lsn? > > > + WAL records are appended to the WAL > + logs as each new record is written. The insert position is described by > + a Log Sequence Number (LSN) that is a byte offset into > + the logs, increasing monotonically with each new record. Two > + LSNs can be compared to calculate the volume of > + WAL data that separates them, so are used to measure > + progress of WAL during replication and recovery. > + > Here we could for example append a sentence like "The system data type > pg_lsn is an internal > representation of the LSN". Good input, thanks. v2 attached. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services doc_lsn.v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs wrote: > On 4 September 2016 at 14:32, Abhijit Menon-Sen wrote: >> At 2016-09-04 07:02:01 +0100, si...@2ndquadrant.com wrote: >>> >>> > By the way, what has been committed does not include the patch >>> > adding the parsing context in case of an error as wanted upthread. >>> > Perhaps that's not worth adding now as there is the GUC refactoring >>> > potentially happening for the recovery parameters, so I don't mind >>> > much. Just that's worth mentioning. >>> >>> Hmm, that was unintentional. If something stalls the recovery >>> parameter project, please remind me to commit that as well. >> >> I'm aware of this, and will adjust accordingly in the GUC patch. Thanks >> for the heads up. > > I noticed we don't mention what LSN is anywhere, so I'd like to apply > the following doc patch also. +1 for the idea. What do you think about adding a mention to the system data type pg_lsn? + WAL records are appended to the WAL + logs as each new record is written. The insert position is described by + a Log Sequence Number (LSN) that is a byte offset into + the logs, increasing monotonically with each new record. Two + LSNs can be compared to calculate the volume of + WAL data that separates them, so are used to measure + progress of WAL during replication and recovery. + Here we could for example append a sentence like "The system data type pg_lsn is an internal representation of the LSN". -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 4 September 2016 at 14:32, Abhijit Menon-Sen wrote: > At 2016-09-04 07:02:01 +0100, si...@2ndquadrant.com wrote: >> >> > By the way, what has been committed does not include the patch >> > adding the parsing context in case of an error as wanted upthread. >> > Perhaps that's not worth adding now as there is the GUC refactoring >> > potentially happening for the recovery parameters, so I don't mind >> > much. Just that's worth mentioning. >> >> Hmm, that was unintentional. If something stalls the recovery >> parameter project, please remind me to commit that as well. > > I'm aware of this, and will adjust accordingly in the GUC patch. Thanks > for the heads up. I noticed we don't mention what LSN is anywhere, so I'd like to apply the following doc patch also. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services doc_lsn.v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
At 2016-09-04 07:02:01 +0100, si...@2ndquadrant.com wrote: > > > By the way, what has been committed does not include the patch > > adding the parsing context in case of an error as wanted upthread. > > Perhaps that's not worth adding now as there is the GUC refactoring > > potentially happening for the recovery parameters, so I don't mind > > much. Just that's worth mentioning. > > Hmm, that was unintentional. If something stalls the recovery > parameter project, please remind me to commit that as well. I'm aware of this, and will adjust accordingly in the GUC patch. Thanks for the heads up. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Sun, Sep 4, 2016 at 3:02 PM, Simon Riggs wrote: > On 4 September 2016 at 04:50, Michael Paquier > wrote: >> On Sun, Sep 4, 2016 at 8:05 AM, Michael Paquier >> wrote: >>> On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs wrote: On 24 August 2016 at 05:50, Michael Paquier wrote: >>> Everything else looks in good order. Committed. Thanks. >>> >>> Thanks for the commit! > > No problem, it was a good patch. Since I moan to others about lack of > docs, tests etc, I'll do the same here and compliment you on providing > a well rounded patch with docs, tests that does what it says in a > clean way. Thanks a lot! That's really motivating! I don't think I deserve that. >> By the way, what has been committed does not include the patch adding >> the parsing context in case of an error as wanted upthread. Perhaps >> that's not worth adding now as there is the GUC refactoring >> potentially happening for the recovery parameters, so I don't mind >> much. Just that's worth mentioning. > > Hmm, that was unintentional. If something stalls the recovery > parameter project, please remind me to commit that as well. That's what is here as 0002: https://www.postgresql.org/message-id/CAB7nPqSG5adk7%3DnBgKqy8uQ1tXkeX212jboO_hJbZDVehD3w8Q%40mail.gmail.com This makes produce a context message should an error occur, like that: FATAL: invalid input syntax for type pg_lsn: "popo" CONTEXT: line 11 of configuration file "recovery.conf", parameter "recovery_target_lsn" And as outlined by reviewers upthread this is particularly useful for recovery_target_time and recovery_target_lsn because those would bump on a parsing error message from the data type, letting users no information that it came from recovery.conf :( -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 4 September 2016 at 04:50, Michael Paquier wrote: > On Sun, Sep 4, 2016 at 8:05 AM, Michael Paquier > wrote: >> On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs wrote: >>> On 24 August 2016 at 05:50, Michael Paquier >>> wrote: >>> >> Everything else looks in good order. >>> >>> Committed. Thanks. >> >> Thanks for the commit! No problem, it was a good patch. Since I moan to others about lack of docs, tests etc, I'll do the same here and compliment you on providing a well rounded patch with docs, tests that does what it says in a clean way. > By the way, what has been committed does not include the patch adding > the parsing context in case of an error as wanted upthread. Perhaps > that's not worth adding now as there is the GUC refactoring > potentially happening for the recovery parameters, so I don't mind > much. Just that's worth mentioning. Hmm, that was unintentional. If something stalls the recovery parameter project, please remind me to commit that as well. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Sun, Sep 4, 2016 at 8:05 AM, Michael Paquier wrote: > On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs wrote: >> On 24 August 2016 at 05:50, Michael Paquier >> wrote: >> > Everything else looks in good order. >> >> Committed. Thanks. > > Thanks for the commit! By the way, what has been committed does not include the patch adding the parsing context in case of an error as wanted upthread. Perhaps that's not worth adding now as there is the GUC refactoring potentially happening for the recovery parameters, so I don't mind much. Just that's worth mentioning. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs wrote: > On 24 August 2016 at 05:50, Michael Paquier wrote: > Everything else looks in good order. > > Committed. Thanks. Thanks for the commit! -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 24 August 2016 at 05:50, Michael Paquier wrote: >>> Everything else looks in good order. Committed. Thanks. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Tue, Aug 23, 2016 at 8:50 PM, Michael Paquier wrote: > On Tue, Aug 23, 2016 at 6:10 PM, Simon Riggs wrote: >> On 23 August 2016 at 09:39, Petr Jelinek wrote: >> >>> Looks very reasonable to me (both patches). Thanks for doing that. >>> >>> I am inclined to mark this as ready for committer. >> >> Looking at it now. >> >> The messages for recovery_target_lsn don't mention after or before, as >> do other targets... e.g. >> recoveryStopAfter ? "after" : "before", >> My understanding is that if you request an LSN that isn't the exact >> end point of a WAL record then it will either stop before or after the >> requested point, so that needs to be described in the docs and in the >> messages generated prior to starting to search. >> >> Everything else looks in good order. > > You are right, this message should be completed as such. Do you want > an updated patch? Well, I finished by updating the patch anyway. -- Michael 0001-Introduce-recovery_target_lsn-in-recovery.conf.patch Description: invalid/octet-stream 0002-Introduce-error-callback-when-parsing-recovery.conf.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Tue, Aug 23, 2016 at 6:10 PM, Simon Riggs wrote: > On 23 August 2016 at 09:39, Petr Jelinek wrote: > >> Looks very reasonable to me (both patches). Thanks for doing that. >> >> I am inclined to mark this as ready for committer. > > Looking at it now. > > The messages for recovery_target_lsn don't mention after or before, as > do other targets... e.g. > recoveryStopAfter ? "after" : "before", > My understanding is that if you request an LSN that isn't the exact > end point of a WAL record then it will either stop before or after the > requested point, so that needs to be described in the docs and in the > messages generated prior to starting to search. > > Everything else looks in good order. You are right, this message should be completed as such. Do you want an updated patch? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 23 August 2016 at 09:39, Petr Jelinek wrote: > Looks very reasonable to me (both patches). Thanks for doing that. > > I am inclined to mark this as ready for committer. Looking at it now. The messages for recovery_target_lsn don't mention after or before, as do other targets... e.g. recoveryStopAfter ? "after" : "before", My understanding is that if you request an LSN that isn't the exact end point of a WAL record then it will either stop before or after the requested point, so that needs to be described in the docs and in the messages generated prior to starting to search. Everything else looks in good order. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 08/23/2016 10:39 AM, Petr Jelinek wrote: > On 23/08/16 09:33, Michael Paquier wrote: >> On Tue, Aug 23, 2016 at 12:49 AM, Robert Haas >> wrote: >>> On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier >>> wrote: On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat wrote: > As Julien said, there is nothing to notice that error comes from > recovery.conf. > My fear would be that an user encounters an error like this. Il > will be > difficult to link to the recovery.conf. Thinking a bit wider than that, we may want to know such context for normal GUC parameters as well, and that's not the case now. Perhaps there is actually a reason why that's not done for GUCs, but it seems that it would be useful there as well. That would give another reason to move all that under the GUC umbrella. >>> >>> Maybe so, but that's been tried multiple times without success. If >>> you think an error context is useful here, and I bet it is, I'd say >>> just add it and be done with it. >> >> This has finished by being less ugly than I thought, so I implemented >> it as attached. Patch 0001 introduces recovery_target_lsn, and patch >> 0002 sets up an error context callback generating things like that on >> failure: >> FATAL: invalid input syntax for type pg_lsn: "popo" >> CONTEXT: line 11 of configuration file "recovery.conf", parameter >> "recovery_target_lsn" Good! Message is clear now for recovery_target_lsn and recovery_target_time. Thanks for your work. >> > > Looks very reasonable to me (both patches). Thanks for doing that. > > I am inclined to mark this as ready for committer. > +1 everything if fine for me. -- Adrien NAYRAT http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 23/08/16 09:33, Michael Paquier wrote: On Tue, Aug 23, 2016 at 12:49 AM, Robert Haas wrote: On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier wrote: On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat wrote: As Julien said, there is nothing to notice that error comes from recovery.conf. My fear would be that an user encounters an error like this. Il will be difficult to link to the recovery.conf. Thinking a bit wider than that, we may want to know such context for normal GUC parameters as well, and that's not the case now. Perhaps there is actually a reason why that's not done for GUCs, but it seems that it would be useful there as well. That would give another reason to move all that under the GUC umbrella. Maybe so, but that's been tried multiple times without success. If you think an error context is useful here, and I bet it is, I'd say just add it and be done with it. This has finished by being less ugly than I thought, so I implemented it as attached. Patch 0001 introduces recovery_target_lsn, and patch 0002 sets up an error context callback generating things like that on failure: FATAL: invalid input syntax for type pg_lsn: "popo" CONTEXT: line 11 of configuration file "recovery.conf", parameter "recovery_target_lsn" Looks very reasonable to me (both patches). Thanks for doing that. I am inclined to mark this as ready for committer. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Tue, Aug 23, 2016 at 12:49 AM, Robert Haas wrote: > On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier > wrote: >> On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat >> wrote: >>> As Julien said, there is nothing to notice that error comes from >>> recovery.conf. >>> My fear would be that an user encounters an error like this. Il will be >>> difficult to link to the recovery.conf. >> >> Thinking a bit wider than that, we may want to know such context for >> normal GUC parameters as well, and that's not the case now. Perhaps >> there is actually a reason why that's not done for GUCs, but it seems >> that it would be useful there as well. That would give another reason >> to move all that under the GUC umbrella. > > Maybe so, but that's been tried multiple times without success. If > you think an error context is useful here, and I bet it is, I'd say > just add it and be done with it. This has finished by being less ugly than I thought, so I implemented it as attached. Patch 0001 introduces recovery_target_lsn, and patch 0002 sets up an error context callback generating things like that on failure: FATAL: invalid input syntax for type pg_lsn: "popo" CONTEXT: line 11 of configuration file "recovery.conf", parameter "recovery_target_lsn" -- Michael From 6f73e9b02ee9806dd73571e1190983748fcad03d Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 23 Aug 2016 16:12:03 +0900 Subject: [PATCH 1/2] Introduce recovery_target_lsn in recovery.conf This new recovery target can be used to stop recovery once a given WAL position is reached (LSN). Tests and documentation are added. --- doc/src/sgml/recovery-config.sgml | 24 +++-- src/backend/access/transam/recovery.conf.sample | 6 ++- src/backend/access/transam/xlog.c | 69 + src/include/access/xlog.h | 1 + src/test/recovery/t/003_recovery_targets.pl | 28 -- 5 files changed, 119 insertions(+), 9 deletions(-) diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml index 26af221..ad977d3 100644 --- a/doc/src/sgml/recovery-config.sgml +++ b/doc/src/sgml/recovery-config.sgml @@ -157,9 +157,10 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows By default, recovery will recover to the end of the WAL log. The following parameters can be used to specify an earlier stopping point. At most one of recovery_target, - recovery_target_name, recovery_target_time, or - recovery_target_xid can be used; if more than one of these - is specified in the configuration file, the last entry will be used. + recovery_target_lsn, recovery_target_name, + recovery_target_time, or recovery_target_xid + can be used; if more than one of these is specified in the configuration + file, the last entry will be used. @@ -232,6 +233,23 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows + + + recovery_target_lsn (pg_lsn) + +recovery_target_lsn recovery parameter + + + + +This parameter specifies the LSN of the write-ahead log stream up to +which recovery will proceed. The precise stopping point is also +influenced by . This +parameter is parsed using the system data type +pg_lsn. + + + diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample index b777400..7a16751 100644 --- a/src/backend/access/transam/recovery.conf.sample +++ b/src/backend/access/transam/recovery.conf.sample @@ -67,8 +67,8 @@ # must set a recovery target. # # You may set a recovery target either by transactionId, by name, -# or by timestamp. Recovery may either include or exclude the -# transaction(s) with the recovery target value (ie, stop either +# by timestamp or by WAL position (LSN). Recovery may either include or +# exclude the transaction(s) with the recovery target value (ie, stop either # just after or just before the given target, respectively). # # @@ -78,6 +78,8 @@ # #recovery_target_xid = '' # +#recovery_target_lsn = '' # e.g. '0/70006B8' +# #recovery_target_inclusive = true # # diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index f13f9c1..e3985ee 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -67,6 +67,7 @@ #include "utils/builtins.h" #include "utils/guc.h" #include "utils/memutils.h" +#include "utils/pg_lsn.h" #include "utils/ps_status.h" #include "utils/relmapper.h" #include "utils/snapmgr.h" @@ -254,6 +255,7 @@ static RecoveryTargetAction recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE; static TransactionId recoveryTargetXid; static TimestampTz recoveryTargetTime; static char *recoveryTargetName; +static XLogRecPtr recoveryTargetLSN; static int recovery_min_app
Re: [HACKERS] LSN as a recovery target
On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier wrote: > On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat > wrote: >> As Julien said, there is nothing to notice that error comes from >> recovery.conf. >> My fear would be that an user encounters an error like this. Il will be >> difficult to link to the recovery.conf. > > Thinking a bit wider than that, we may want to know such context for > normal GUC parameters as well, and that's not the case now. Perhaps > there is actually a reason why that's not done for GUCs, but it seems > that it would be useful there as well. That would give another reason > to move all that under the GUC umbrella. Maybe so, but that's been tried multiple times without success. If you think an error context is useful here, and I bet it is, I'd say just add it and be done with it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 8/22/16 8:28 AM, Michael Paquier wrote: > Thinking a bit wider than that, we may want to know such context for > normal GUC parameters as well, and that's not the case now. Perhaps > there is actually a reason why that's not done for GUCs, but it seems > that it would be useful there as well. GUC parsing generally needs, or used to need, to work under more constrained circumstances, e.g., no full memory management. That's not a reason not to try this, but there might be non-obvious problems. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat wrote: > As Julien said, there is nothing to notice that error comes from > recovery.conf. > My fear would be that an user encounters an error like this. Il will be > difficult to link to the recovery.conf. Thinking a bit wider than that, we may want to know such context for normal GUC parameters as well, and that's not the case now. Perhaps there is actually a reason why that's not done for GUCs, but it seems that it would be useful there as well. That would give another reason to move all that under the GUC umbrella. > For the others settings (xid, timeline,name) there is an explicit > message that notice error is in recovery.conf. > > I see it is not the case for recovery_target_time. Yes, in this case the parameter value is parsed using an datatype _in function call. And the error message depends on that.. > Should we modify only the documentation or should we try to find a > solution to point the origin of error? The patch as proposed is complicated enough I think, and it would be good to keep things simple if we can. So having something in the docs looks fine to me, and that's actually the reference to pg_lsn used to parse the parameter value. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 08/20/2016 04:16 PM, Michael Paquier wrote: > On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek wrote: >> On 20/08/16 02:13, Michael Paquier wrote: >>> On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat >>> wrote: >>> Using a PG_TRY/CATCH block the way you do to show to user a different >>> error message while the original one is actually correct does not >>> sound like a good idea to me. It complicates the code and the original >>> pattern matches what is already done for timestamps, where in case of >>> error you'd get that: >>> FATAL: invalid input syntax for type timestamp with time zone: "aa" >>> >>> I think that a better solution would be to make clearer in the docs >>> that pg_lsn is used here. First, in recovery.conf.sample, let's use >>> pg_lsn and not LSN. Then, in the sgml docs, let's add a reference to >>> the page of pg_lsn as well: >>> https://www.postgresql.org/docs/devel/static/datatype-pg-lsn.html >>> >>> Now we have that: >>> + >>> +This parameter specifies the LSN of the write-ahead log stream up >>> to >>> +which recovery will proceed. The precise stopping point is also >>> +influenced by . >>> + >>> Perhaps we could just add an additional sentence: "This parameter >>> value is parsed using the system datatype ". Or something >>> like that. Thoughts? >>> >> >> +1 > > So here is the patch resulting in that. After thinking more about that > I have arrived at the conclusion to not use LSN in recovery.conf, but > "WAL position (LSN)", and also mention in the docs that pg_lsn is used > to parse the parameter value. A couple of log messages are changed > similarly. It definitely makes it more understandable for users to > speak of WAL positions. Good, I prefer this. > I also noticed that the top block of "Recovery Target Settings" needs > to mention recovery_target_lsn, aka when name, xid, lsn and time are > combined in the same recovery.conf, only the last value will be used. > Good catch! As Julien said, there is nothing to notice that error comes from recovery.conf. My fear would be that an user encounters an error like this. Il will be difficult to link to the recovery.conf. For the others settings (xid, timeline,name) there is an explicit message that notice error is in recovery.conf. I see it is not the case for recovery_target_time. Should we modify only the documentation or shoud we try to find a solution to point the origin of error? -- Adrien NAYRAT http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek wrote: > On 20/08/16 02:13, Michael Paquier wrote: >> On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat >> wrote: >> Using a PG_TRY/CATCH block the way you do to show to user a different >> error message while the original one is actually correct does not >> sound like a good idea to me. It complicates the code and the original >> pattern matches what is already done for timestamps, where in case of >> error you'd get that: >> FATAL: invalid input syntax for type timestamp with time zone: "aa" >> >> I think that a better solution would be to make clearer in the docs >> that pg_lsn is used here. First, in recovery.conf.sample, let's use >> pg_lsn and not LSN. Then, in the sgml docs, let's add a reference to >> the page of pg_lsn as well: >> https://www.postgresql.org/docs/devel/static/datatype-pg-lsn.html >> >> Now we have that: >> + >> +This parameter specifies the LSN of the write-ahead log stream up >> to >> +which recovery will proceed. The precise stopping point is also >> +influenced by . >> + >> Perhaps we could just add an additional sentence: "This parameter >> value is parsed using the system datatype ". Or something >> like that. Thoughts? >> > > +1 So here is the patch resulting in that. After thinking more about that I have arrived at the conclusion to not use LSN in recovery.conf, but "WAL position (LSN)", and also mention in the docs that pg_lsn is used to parse the parameter value. A couple of log messages are changed similarly. It definitely makes it more understandable for users to speak of WAL positions. I also noticed that the top block of "Recovery Target Settings" needs to mention recovery_target_lsn, aka when name, xid, lsn and time are combined in the same recovery.conf, only the last value will be used. -- Michael recovery-target-lsn-3.patch Description: application/download -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 20/08/2016 15:41, Michael Paquier wrote: > On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek wrote: >> If we want to specifically name the recovery_target_lsn in the message, we >> could probably do it using context. > > So that would be basically assigning error_context_stack for each item > parsed for recovery.conf? That seems a bit narrow as usually > recovery.conf is not that long.. > That'd still be an improvement, since the error message doesn't even mention that the error comes from recovery.conf. I agree it can't come from many places, but it may be useful for some people. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek wrote: > If we want to specifically name the recovery_target_lsn in the message, we > could probably do it using context. So that would be basically assigning error_context_stack for each item parsed for recovery.conf? That seems a bit narrow as usually recovery.conf is not that long.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 20/08/16 02:13, Michael Paquier wrote: On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat wrote: I reviewed this patch rebased to deal with f6ced51f9188ad5806219471a0b40a91dde923aa, and minor adjustment (see below) Thanks! It do the job. However if you use an incorrect recovery_target_lsn you get this message : "FATAL: invalid input syntax for type pg_lsn: "0RT5/4"" I added a PG_TRY/PG_CATCH section in order to get a more explicit message : FATAL: wrong recovery_target_lsn (must be pg_lsn type) I am not sure if it is the best solution? Using a PG_TRY/CATCH block the way you do to show to user a different error message while the original one is actually correct does not sound like a good idea to me. It complicates the code and the original pattern matches what is already done for timestamps, where in case of error you'd get that: FATAL: invalid input syntax for type timestamp with time zone: "aa" I think that a better solution would be to make clearer in the docs that pg_lsn is used here. First, in recovery.conf.sample, let's use pg_lsn and not LSN. Then, in the sgml docs, let's add a reference to the page of pg_lsn as well: https://www.postgresql.org/docs/devel/static/datatype-pg-lsn.html Now we have that: + +This parameter specifies the LSN of the write-ahead log stream up to +which recovery will proceed. The precise stopping point is also +influenced by . + Perhaps we could just add an additional sentence: "This parameter value is parsed using the system datatype ". Or something like that. Thoughts? +1 If we want to specifically name the recovery_target_lsn in the message, we could probably do it using context. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat wrote: > I reviewed this patch rebased to deal with > f6ced51f9188ad5806219471a0b40a91dde923aa, and minor adjustment (see below) Thanks! > It do the job. However if you use an incorrect recovery_target_lsn you > get this message : > "FATAL: invalid input syntax for type pg_lsn: "0RT5/4"" > > I added a PG_TRY/PG_CATCH section in order to get a more explicit message : > FATAL: wrong recovery_target_lsn (must be pg_lsn type) > > I am not sure if it is the best solution? Using a PG_TRY/CATCH block the way you do to show to user a different error message while the original one is actually correct does not sound like a good idea to me. It complicates the code and the original pattern matches what is already done for timestamps, where in case of error you'd get that: FATAL: invalid input syntax for type timestamp with time zone: "aa" I think that a better solution would be to make clearer in the docs that pg_lsn is used here. First, in recovery.conf.sample, let's use pg_lsn and not LSN. Then, in the sgml docs, let's add a reference to the page of pg_lsn as well: https://www.postgresql.org/docs/devel/static/datatype-pg-lsn.html Now we have that: + +This parameter specifies the LSN of the write-ahead log stream up to +which recovery will proceed. The precise stopping point is also +influenced by . + Perhaps we could just add an additional sentence: "This parameter value is parsed using the system datatype ". Or something like that. Thoughts? I'll think about that a bit more and send a new patch. I have switched the patch to "waiting on author" for now, just to not forget about that. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 06/09/2016 02:33 PM, Michael Paquier wrote:
> On Wed, May 25, 2016 at 1:32 AM, Michael Paquier
> wrote:
>> On Tue, May 24, 2016 at 9:29 AM, Alvaro Herrera
>> wrote:
>>> Christoph Berg wrote:
Re: Michael Paquier 2016-05-24
> Yeah, that's really something that covers only a narrow case, though
> if we don't have it when we need it we're limited to some hacks.
> Perhaps people who have the advanced level to use such a thing have
> the level to use hacks anyway..
I'd think recovery_target_lsn would be more useful in practice than
the existing recovery_target_xid. So I don't see why it shouldn't just
be added, also given it's likely very unobtrusive to do so.
>>>
>>> Also, see
>>> http://www.postgresql.org/message-id/56bd0e4e.5050...@2ndquadrant.com
>>
>> Looking at xlog.c it is not that complicated, and we could add tests
>> in 003_recovery_targets.pl at the same time. Perhaps somebody looking
>> for a first participation would be interested in this small project?
>
> Oh, well. I have implemented it as attached by introducing
> recovery_target_lsn as a new recovery parameter. This takes into
> account recovery_target_inclusive and stops at the precise point of a
> record without being influenced by the xact records, in a way similar
> recovery_target_name. Tests and documentation are added, and this is
> part of the next CF.
>
>
>
>
Hi,
I reviewed this patch rebased to deal with
f6ced51f9188ad5806219471a0b40a91dde923aa, and minor adjustment (see below)
It do the job. However if you use an incorrect recovery_target_lsn you
get this message :
"FATAL: invalid input syntax for type pg_lsn: "0RT5/4""
I added a PG_TRY/PG_CATCH section in order to get a more explicit message :
FATAL: wrong recovery_target_lsn (must be pg_lsn type)
I am not sure if it is the best solution?
Thanks to Julien Rouhaud for his help.
--
Adrien NAYRAT
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml
index 26af221..e131b7d 100644
--- a/doc/src/sgml/recovery-config.sgml
+++ b/doc/src/sgml/recovery-config.sgml
@@ -232,6 +232,21 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
+
+
+ recovery_target_lsn (pg_lsn)
+
+recovery_target_lsn recovery parameter
+
+
+
+
+This parameter specifies the LSN of the write-ahead log stream up to
+which recovery will proceed. The precise stopping point is also
+influenced by .
+
+
+
diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample
index b777400..32f6903 100644
--- a/src/backend/access/transam/recovery.conf.sample
+++ b/src/backend/access/transam/recovery.conf.sample
@@ -67,7 +67,7 @@
# must set a recovery target.
#
# You may set a recovery target either by transactionId, by name,
-# or by timestamp. Recovery may either include or exclude the
+# by timestamp or by LSN. Recovery may either include or exclude the
# transaction(s) with the recovery target value (ie, stop either
# just after or just before the given target, respectively).
#
@@ -78,6 +78,8 @@
#
#recovery_target_xid = ''
#
+#recovery_target_lsn = '' # e.g. '0/70006B8'
+#
#recovery_target_inclusive = true
#
#
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f13f9c1..762c7bb 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -67,6 +67,7 @@
#include "utils/builtins.h"
#include "utils/guc.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/ps_status.h"
#include "utils/relmapper.h"
#include "utils/snapmgr.h"
@@ -254,6 +255,7 @@ static RecoveryTargetAction recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
static TransactionId recoveryTargetXid;
static TimestampTz recoveryTargetTime;
static char *recoveryTargetName;
+static XLogRecPtr recoveryTargetLSN;
static int recovery_min_apply_delay = 0;
static TimestampTz recoveryDelayUntilTime;
@@ -275,6 +277,7 @@ static bool fast_promote = false;
*/
static TransactionId recoveryStopXid;
static TimestampTz recoveryStopTime;
+static XLogRecPtr recoveryStopLSN;
static char recoveryStopName[MAXFNAMELEN];
static bool recoveryStopAfter;
@@ -5078,6 +5081,34 @@ readRecoveryCommandFile(void)
(errmsg_internal("recovery_target_name = '%s'",
recoveryTargetName)));
}
+ else if (strcmp(item->name, "recovery_target_lsn") == 0)
+ {
+ recoveryTarget = RECOVERY_TARGET_LSN;
+
+ /*
+ * Convert the LSN string given by the user to XLogRecPtr form.
+ */
+ PG_TRY();
+ {
+recoveryTargetLSN =
+ DatumGetLSN(DirectFunctionCall3(pg_lsn_in,
+CStringGetDatum(item->value),
+ObjectIdGetDatum(InvalidOid),
+ Int32GetDatum(-1)));
+ }
+ PG_CATCH();
+ {
+ereport(FATAL,
+ (err
Re: [HACKERS] LSN as a recovery target
On Wed, May 25, 2016 at 1:32 AM, Michael Paquier wrote: > On Tue, May 24, 2016 at 9:29 AM, Alvaro Herrera > wrote: >> Christoph Berg wrote: >>> Re: Michael Paquier 2016-05-24 >>> >>> > Yeah, that's really something that covers only a narrow case, though >>> > if we don't have it when we need it we're limited to some hacks. >>> > Perhaps people who have the advanced level to use such a thing have >>> > the level to use hacks anyway.. >>> >>> I'd think recovery_target_lsn would be more useful in practice than >>> the existing recovery_target_xid. So I don't see why it shouldn't just >>> be added, also given it's likely very unobtrusive to do so. >> >> Also, see >> http://www.postgresql.org/message-id/56bd0e4e.5050...@2ndquadrant.com > > Looking at xlog.c it is not that complicated, and we could add tests > in 003_recovery_targets.pl at the same time. Perhaps somebody looking > for a first participation would be interested in this small project? Oh, well. I have implemented it as attached by introducing recovery_target_lsn as a new recovery parameter. This takes into account recovery_target_inclusive and stops at the precise point of a record without being influenced by the xact records, in a way similar recovery_target_name. Tests and documentation are added, and this is part of the next CF. -- Michael recovery-target-lsn.patch Description: binary/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Tue, May 24, 2016 at 9:29 AM, Alvaro Herrera wrote: > Christoph Berg wrote: >> Re: Michael Paquier 2016-05-24 >> >> > Yeah, that's really something that covers only a narrow case, though >> > if we don't have it when we need it we're limited to some hacks. >> > Perhaps people who have the advanced level to use such a thing have >> > the level to use hacks anyway.. >> >> I'd think recovery_target_lsn would be more useful in practice than >> the existing recovery_target_xid. So I don't see why it shouldn't just >> be added, also given it's likely very unobtrusive to do so. > > Also, see > http://www.postgresql.org/message-id/56bd0e4e.5050...@2ndquadrant.com Looking at xlog.c it is not that complicated, and we could add tests in 003_recovery_targets.pl at the same time. Perhaps somebody looking for a first participation would be interested in this small project? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
Christoph Berg wrote: > Re: Michael Paquier 2016-05-24 > > > Yeah, that's really something that covers only a narrow case, though > > if we don't have it when we need it we're limited to some hacks. > > Perhaps people who have the advanced level to use such a thing have > > the level to use hacks anyway.. > > I'd think recovery_target_lsn would be more useful in practice than > the existing recovery_target_xid. So I don't see why it shouldn't just > be added, also given it's likely very unobtrusive to do so. Also, see http://www.postgresql.org/message-id/56bd0e4e.5050...@2ndquadrant.com -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
Re: Michael Paquier 2016-05-24 > Yeah, that's really something that covers only a narrow case, though > if we don't have it when we need it we're limited to some hacks. > Perhaps people who have the advanced level to use such a thing have > the level to use hacks anyway.. I'd think recovery_target_lsn would be more useful in practice than the existing recovery_target_xid. So I don't see why it shouldn't just be added, also given it's likely very unobtrusive to do so. Christoph -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Mon, May 23, 2016 at 6:25 PM, Craig Ringer wrote: > On 24 May 2016 at 09:12, Michael Paquier wrote: >> >> Hi all, >> >> Today somebody has pointed me out that it could be interesting to be >> able to recovery up to a given LSN position. One argument behind that >> was to allow a maximum of things to recover up to the point where a >> relation block got corrupted by a specific record because of a broken >> segment. So that would be simply having recovery_target_lsn in >> recovery.conf similar to what we have now. >> Thoughts? >> > > Sounds useful, if somewhat niche. I've needed that in the past, and just > zeroed the WAL segment after the end of the target record. Not super > user-friendly. Yeah, that's really something that covers only a narrow case, though if we don't have it when we need it we're limited to some hacks. Perhaps people who have the advanced level to use such a thing have the level to use hacks anyway.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 24 May 2016 at 09:12, Michael Paquier wrote: > Hi all, > > Today somebody has pointed me out that it could be interesting to be > able to recovery up to a given LSN position. One argument behind that > was to allow a maximum of things to recover up to the point where a > relation block got corrupted by a specific record because of a broken > segment. So that would be simply having recovery_target_lsn in > recovery.conf similar to what we have now. > Thoughts? > > Sounds useful, if somewhat niche. I've needed that in the past, and just zeroed the WAL segment after the end of the target record. Not super user-friendly. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
