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. However that in turn implies that XLogRead() does not need the "tli" argument at all. > > > Why do we leave this responsibility to ReadPageInternal? Wouldn't it > > > make more sense to leave XLogRead be always responsible for setting > > > these correctly, and remove those lines from ReadPageInternal? > > > > I think there's no rule that ReadPageInternal() must use XLogRead(). If we > > do > > what you suggest, we need make this responsibility documented. I'll consider > > that. I think now we should not add any responsibility to XLogPageReadCB or its subroutines because some extensions might already have their implementation of XLogPageReadCB w/o XLogRead, and this change would break them. -- Antonin Houska Web: https://www.cybertec-postgresql.com
