Hello, At Sat, 28 Sep 2019 15:00:35 +0200, Antonin Houska <a...@cybertec.at> wrote in <9236.1569675635@antos> > Antonin Houska <a...@cybertec.at> wrote: > > > Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > > > > > BTW that tli_p business to the openSegment callback is horribly > > > inconsistent. Some callers accept a NULL tli_p, others will outright > > > crash, even though the API docs say that the callback must determine the > > > timeline. This is made more complicated by us having the TLI in "seg" > > > also. Unless I misread, the problem is again that the walsender code is > > > doing nasty stuff with globals (endSegNo). As a very minor stylistic > > > point, we prefer to have out params at the end of the signature. > > > > XLogRead() tests for NULL so it should not crash but I don't insist on doing > > it this way. XLogRead() actually does not have to care whether the "open > > segment callback" determines the TLI or not, so it (XLogRead) can always > > receive a valid pointer to seg.ws_tli. > > This is actually wrong - seg.ws_tli is not always the correct value to > pass. If seg.ws_tli refers to the segment from which data was read last time, > then XLogRead() still needs a separate argument to specify from which TLI the > current call should read. If these two differ, new file needs to be opened.
openSegment represents the file *currently* opened. XLogRead needs the TLI *to be* opened. If they are different, as far as wal logical wal sender and pg_waldump is concerned, XLogRead switches to the new TLI and the new TLI is set to openSegment.ws_tli. So, it seems to me that the parameter doesn't need to be inout? It is enough that it is an "in" parameter. > The problem of walsender.c is that its implementation of XLogRead() does not > care about the TLI of the previous read. If the behavior of the new, generic > implementation should be exactly the same, we need to tell XLogRead() that in > some cases it also should not compare the current TLI to the previous > one. That's why I tried to use the NULL pointer, or the InvalidTimeLineID > earlier. Physical wal sender doesn't switch TLI. So I don't think the behavior doesn't harm (or doesn't fire). openSegment holds the TLI set at the first call. (Even if future wal sender switches TLI, the behavior should be needed.) > Another approach is to add a boolean argument "check_tli", but that still > forces caller to pass some (random) value of the tli. The concept of > InvalidTimeLineID seems to me less disturbing than this. So I think InvalidTimeLineID is not needed. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
