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. 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. 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. -- Antonin Houska Web: https://www.cybertec-postgresql.com