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)