Re: ThisTimeLineID can be used uninitialized

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_Q6aSEzsvrPHDwzw@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)