Re: removing global variable ThisTimeLineID

2021-11-23 Thread Robert Haas
On Tue, Nov 23, 2021 at 4:36 AM Drouvot, Bertrand wrote: > Indeed, I think the logical decoding on standby patch just needs to > change the Assert in GetWALInsertionTimeLine() to check > SharedRecoveryState is RECOVERY_STATE_DONE or RECOVERY_STATE_ARCHIVE. > > Would that make sense from your point

Re: removing global variable ThisTimeLineID

2021-11-10 Thread Robert Haas
On Tue, Nov 9, 2021 at 7:41 PM Michael Paquier wrote: > On Tue, Nov 09, 2021 at 02:15:51PM -0500, Robert Haas wrote: > > That's a good point. However, since I think newTLI is already in use > > in some of the functions StartupXLOG() calls, and since I think it > > would be good to use the same nam

Re: removing global variable ThisTimeLineID

2021-11-09 Thread Michael Paquier
On Tue, Nov 09, 2021 at 02:15:51PM -0500, Robert Haas wrote: > That's a good point. However, since I think newTLI is already in use > in some of the functions StartupXLOG() calls, and since I think it > would be good to use the same name in StartupXLOG() as in the > functions that it calls, what I'

Re: removing global variable ThisTimeLineID

2021-11-09 Thread Robert Haas
On Mon, Nov 8, 2021 at 10:31 PM Michael Paquier wrote: > I think that this patch is an improvement. Cool. > @@ -6686,8 +6682,8 @@ StartupXLOG(void) > - TimeLineID ThisTimeLineID, > - PrevTimeLineID; > + TimeLineID replayTLI, > + newTLI; > One problem with newTLI

Re: removing global variable ThisTimeLineID

2021-11-08 Thread Michael Paquier
On Mon, Nov 08, 2021 at 12:49:52PM -0500, Robert Haas wrote: > Even with this patch, the name ThisTimeLineID is still used for > multiple purposes. It remains part of the CheckPoint struct, and also > part of the xl_end_of_recovery struct. But in both of those cases, the > name ThisTimeLineID actua

Re: removing global variable ThisTimeLineID

2021-11-08 Thread Robert Haas
On Mon, Nov 8, 2021 at 8:27 AM Robert Haas wrote: > Perhaps. That's related to the point I made before, that it might be a > good idea to be more clear about which of these functions are intended > to be used in recovery and which ones are intended to be used in > normal running. I don't rule out

Re: removing global variable ThisTimeLineID

2021-11-08 Thread Robert Haas
On Sun, Nov 7, 2021 at 11:24 PM Michael Paquier wrote: > I got to wonder whether it would be better to add in GetFlushRecPtr() > the same assertion as GetWALInsertionTimeLine(). All the in-core > callers of this routine already assume that, and it would be buggy if > one passes down insertTLI to

Re: removing global variable ThisTimeLineID

2021-11-07 Thread Michael Paquier
On Tue, Nov 02, 2021 at 08:45:57AM -0400, Robert Haas wrote: > Also, XLogCtl->ThisTimeLineID isn't set until the end of recovery, so > calling this function during recovery would be a mistake. There seem > to be a number of interface functions in xlog.c that should only be > called when not in reco

Re: removing global variable ThisTimeLineID

2021-11-05 Thread Robert Haas
On Fri, Nov 5, 2021 at 9:49 AM Alvaro Herrera wrote: > I looked at these in a very quick skim. I agree that this is a > good step in a good direction. Cool. I have now committed these. I grouped them into just 3 commits rather than having 11 separate commits as I did in my earlier posting, but t

Re: removing global variable ThisTimeLineID

2021-11-05 Thread Alvaro Herrera
I looked at these in a very quick skim. I agree that this is a good step in a good direction. I 'git rebase -x'd this series in order to compile and run the tests on each patch individually. There are no compiler warnings anywhere and no test failures. -- Álvaro Herrera Valdivia,

Re: removing global variable ThisTimeLineID

2021-11-03 Thread Robert Haas
On Wed, Nov 3, 2021 at 9:12 AM Amul Sul wrote: > 0002: > -GetFlushRecPtr(void) > +GetFlushRecPtr(TimeLineID *insertTLI) > > Should we rename this argument to more generic as "tli", like > GetStandbyFlushRecPtr, since the caller in 003 patch trying to fetch a > TLI that means different for them, e.

Re: removing global variable ThisTimeLineID

2021-11-03 Thread Amul Sul
On Tue, Nov 2, 2021 at 12:46 AM Robert Haas wrote: > > [...] > I would like to clean this up. Attached is a series of patches which > try to do that. For ease of review, this is separated out into quite a > few separate patches, but most likely we'd combine some of them for > commit. Patches 0001

Re: removing global variable ThisTimeLineID

2021-11-02 Thread Robert Haas
On Mon, Nov 1, 2021 at 11:33 PM Michael Paquier wrote: > + /* > +* If we're writing and flushing WAL, the time line can't be changing, > +* so no lock is required. > +*/ > + if (insertTLI) > + *insertTLI = XLogCtl->ThisTimeLineID; > In 0002, there is no downside in putting th

Re: removing global variable ThisTimeLineID

2021-11-01 Thread Michael Paquier
On Mon, Nov 01, 2021 at 03:16:27PM -0400, Robert Haas wrote: > At the risk of stating the obvious, using the same variable for > different purposes at different times is not a great programming > practice. Commits 2f5c4397c39dea49c5608ba583868e26d767fc32 and > 902a2c280012557b85c7e0fce3f6f0e355cb2d

removing global variable ThisTimeLineID

2021-11-01 Thread Robert Haas
Hi, The global variable ThisTimeLineID is rather confusing. During recovery, in the startup process, when we're reading a WAL record, it is the timeline of the WAL record we are trying to read or have just read, except when we're trying to read the initial checkpoint record, when it's zero. In oth