Обсуждение: LSN as a recovery target
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? -- Michael
On 24 May 2016 at 09:12, Michael Paquier <michael.paquier@gmail.com> 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.
On Mon, May 23, 2016 at 6:25 PM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 24 May 2016 at 09:12, Michael Paquier <michael.paquier@gmail.com> 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
Re: Michael Paquier 2016-05-24 <CAB7nPqQRXsC8=ozh6GpjLnpZ=MeooUZOaAbzx28n2bjSMv2B4g@mail.gmail.com> > 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
Christoph Berg wrote: > Re: Michael Paquier 2016-05-24 <CAB7nPqQRXsC8=ozh6GpjLnpZ=MeooUZOaAbzx28n2bjSMv2B4g@mail.gmail.com> > > 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.5050503@2ndquadrant.com -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, May 24, 2016 at 9:29 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Christoph Berg wrote: >> Re: Michael Paquier 2016-05-24 <CAB7nPqQRXsC8=ozh6GpjLnpZ=MeooUZOaAbzx28n2bjSMv2B4g@mail.gmail.com> >> > 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.5050503@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
On Wed, May 25, 2016 at 1:32 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, May 24, 2016 at 9:29 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> Christoph Berg wrote: >>> Re: Michael Paquier 2016-05-24 <CAB7nPqQRXsC8=ozh6GpjLnpZ=MeooUZOaAbzx28n2bjSMv2B4g@mail.gmail.com> >>> > 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.5050503@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
Вложения
On 06/09/2016 02:33 PM, Michael Paquier wrote: > On Wed, May 25, 2016 at 1:32 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, May 24, 2016 at 9:29 AM, Alvaro Herrera >> <alvherre@2ndquadrant.com> wrote: >>> Christoph Berg wrote: >>>> Re: Michael Paquier 2016-05-24 <CAB7nPqQRXsC8=ozh6GpjLnpZ=MeooUZOaAbzx28n2bjSMv2B4g@mail.gmail.com> >>>>> 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.5050503@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
Вложения
On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat <adrien.nayrat@dalibo.com> 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: + <para> + 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 <xref linkend="recovery-target-inclusive">. + </para> Perhaps we could just add an additional sentence: "This parameter value is parsed using the system datatype <link=pg_lsn>". 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
On 20/08/16 02:13, Michael Paquier wrote: > On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat > <adrien.nayrat@dalibo.com> 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: > + <para> > + 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 <xref linkend="recovery-target-inclusive">. > + </para> > Perhaps we could just add an additional sentence: "This parameter > value is parsed using the system datatype <link=pg_lsn>". 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
On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek <petr@2ndquadrant.com> 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
On 20/08/2016 15:41, Michael Paquier wrote: > On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek <petr@2ndquadrant.com> 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
On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek <petr@2ndquadrant.com> wrote: > On 20/08/16 02:13, Michael Paquier wrote: >> On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat >> <adrien.nayrat@dalibo.com> 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: >> + <para> >> + 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 <xref linkend="recovery-target-inclusive">. >> + </para> >> Perhaps we could just add an additional sentence: "This parameter >> value is parsed using the system datatype <link=pg_lsn>". 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
Вложения
On 08/20/2016 04:16 PM, Michael Paquier wrote: > On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek <petr@2ndquadrant.com> wrote: >> On 20/08/16 02:13, Michael Paquier wrote: >>> On Fri, Aug 19, 2016 at 10:47 PM, Adrien Nayrat >>> <adrien.nayrat@dalibo.com> 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: >>> + <para> >>> + 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 <xref linkend="recovery-target-inclusive">. >>> + </para> >>> Perhaps we could just add an additional sentence: "This parameter >>> value is parsed using the system datatype <link=pg_lsn>". 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
On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat <adrien.nayrat@dalibo.com> 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
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
On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat <adrien.nayrat@dalibo.com> 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
On Tue, Aug 23, 2016 at 12:49 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat <adrien.nayrat@dalibo.com> 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
Вложения
On 23/08/16 09:33, Michael Paquier wrote: > On Tue, Aug 23, 2016 at 12:49 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat <adrien.nayrat@dalibo.com> 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
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 <robertmhaas@gmail.com> >> wrote: >>> On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier >>> <michael.paquier@gmail.com> wrote: >>>> On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat >>>> <adrien.nayrat@dalibo.com> 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
On 23 August 2016 at 09:39, Petr Jelinek <petr@2ndquadrant.com> 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 Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Aug 23, 2016 at 6:10 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 23 August 2016 at 09:39, Petr Jelinek <petr@2ndquadrant.com> 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
On Tue, Aug 23, 2016 at 8:50 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Aug 23, 2016 at 6:10 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On 23 August 2016 at 09:39, Petr Jelinek <petr@2ndquadrant.com> 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
Вложения
On 24 August 2016 at 05:50, Michael Paquier <michael.paquier@gmail.com> wrote: >>> Everything else looks in good order. Committed. Thanks. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 24 August 2016 at 05:50, Michael Paquier <michael.paquier@gmail.com> wrote: > >>>> Everything else looks in good order. > > Committed. Thanks. Thanks for the commit! -- Michael
On Sun, Sep 4, 2016 at 8:05 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On 24 August 2016 at 05:50, Michael Paquier <michael.paquier@gmail.com> 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
On 4 September 2016 at 04:50, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Sep 4, 2016 at 8:05 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>> On 24 August 2016 at 05:50, Michael Paquier <michael.paquier@gmail.com> 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 Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Sep 4, 2016 at 3:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 4 September 2016 at 04:50, Michael Paquier <michael.paquier@gmail.com> wrote: >> On Sun, Sep 4, 2016 at 8:05 AM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Sun, Sep 4, 2016 at 1:57 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>> On 24 August 2016 at 05:50, Michael Paquier <michael.paquier@gmail.com> 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
At 2016-09-04 07:02:01 +0100, simon@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
On 4 September 2016 at 14:32, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote: > At 2016-09-04 07:02:01 +0100, simon@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 Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 4 September 2016 at 14:32, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote: >> At 2016-09-04 07:02:01 +0100, simon@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? <para> + <acronym>WAL</acronym> records are appended to the <acronym>WAL</acronym> + logs as each new record is written. The insert position is described by + a Log Sequence Number (<acronym>LSN</acronym>) that is a byte offset into + the logs, increasing monotonically with each new record. Two + <acronym>LSN</acronym>s can be compared to calculate the volume of + <acronym>WAL</acronym> data that separates them, so are used to measure + progress of <acronym>WAL</acronym> during replication and recovery. + </para> Here we could for example append a sentence like "The system data type <link linkend="datatype-pg-lsn"><type>pg_lsn</></link> is an internal representation of the LSN". -- Michael
On 5 September 2016 at 06:55, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs <simon@2ndquadrant.com> 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? > > <para> > + <acronym>WAL</acronym> records are appended to the <acronym>WAL</acronym> > + logs as each new record is written. The insert position is described by > + a Log Sequence Number (<acronym>LSN</acronym>) that is a byte offset into > + the logs, increasing monotonically with each new record. Two > + <acronym>LSN</acronym>s can be compared to calculate the volume of > + <acronym>WAL</acronym> data that separates them, so are used to measure > + progress of <acronym>WAL</acronym> during replication and recovery. > + </para> > Here we could for example append a sentence like "The system data type > <link linkend="datatype-pg-lsn"><type>pg_lsn</></link> is an internal > representation of the LSN". Good input, thanks. v2 attached. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Mon, Sep 5, 2016 at 4:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 5 September 2016 at 06:55, Michael Paquier <michael.paquier@gmail.com> wrote: >> On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs <simon@2ndquadrant.com> 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? >> >> <para> >> + <acronym>WAL</acronym> records are appended to the <acronym>WAL</acronym> >> + logs as each new record is written. The insert position is described by >> + a Log Sequence Number (<acronym>LSN</acronym>) that is a byte offset into >> + the logs, increasing monotonically with each new record. Two >> + <acronym>LSN</acronym>s can be compared to calculate the volume of >> + <acronym>WAL</acronym> data that separates them, so are used to measure >> + progress of <acronym>WAL</acronym> during replication and recovery. >> + </para> >> Here we could for example append a sentence like "The system data type >> <link linkend="datatype-pg-lsn"><type>pg_lsn</></link> is an internal >> representation of the LSN". > > Good input, thanks. v2 attached. Looks good to me. Thanks for the wordsmithing. -- Michael