On 2021-Oct-19, Andres Freund wrote: > Hi, > > On 2021-10-19 15:13:04 -0400, Robert Haas wrote: > > This is a followup to > > http://postgr.es/m/ca+tgmoz5a26c6oxkapafyuy_sx0vg6vxdd_q6asezsvrphd...@mail.gmail.com. > > I'm suspicious of the following code in CreateReplicationSlot: > > > > /* setup state for WalSndSegmentOpen */ > > sendTimeLineIsHistoric = false; > > sendTimeLine = ThisTimeLineID; > > > > The first thing that's odd about this is that if this is physical > > replication, it's apparently dead code, because AFAICT sendTimeLine > > will not be used for anything in that case. > > It's quite confusing. It's *really* not helped by physical replication using > but not really using an xlogreader to keep state. Which presumably isn't > actually used during a physical CreateReplicationSlot(), but is referenced by > a comment :/
Yeah, that's not very nice. My preference would be to change physical replication to use xlogreader in the regular way, and avoid confounding backdoors like its current approach. > > But it sure seems strange that the code would apparently work just > > as well as it does today with the following patch: > > > > diff --git a/src/backend/replication/walsender.c > > b/src/backend/replication/walsender.c > > index b811a5c0ef..44fd598519 100644 > > --- a/src/backend/replication/walsender.c > > +++ b/src/backend/replication/walsender.c > > @@ -945,7 +945,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) > > > > /* setup state for WalSndSegmentOpen */ > > sendTimeLineIsHistoric = false; > > - sendTimeLine = ThisTimeLineID; > > + sendTimeLine = rand() % 10; Hah. Yeah, when you can do things like that and the tests don't break, that indicates a problem in the tests. > Istm we should introduce an InvalidTimeLineID, and explicitly initialize > sendTimeLine to that, and assert that it's valid / invalid in a bunch of > places? That's not a bad idea; it'll help discover bogus code. Obviously, some additional tests wouldn't harm -- we have a lot more coverage now than in embarrasingly recent past, but it can still be improved. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Las mujeres son como hondas: mientras más resistencia tienen, más lejos puedes llegar con ellas" (Jonas Nightingale, Leap of Faith)
