Re: [REVIEW] pg_last_xact_insert_timestamp

Sorry for late to re-review.

One question is remaind,

> > Q1: The shmem entry for timestamp is not initialized on
> > allocating. Is this OK? (I don't know that for OSs other than
> > Linux) And zeroing double field is OK for all OSs?
>
> CreateSharedBackendStatus() initializes that shmem entries by doing
> MemSet(BackendStatusArray, 0, size). You think this is not enough?

Sorry for missing it. That's enough.

> > Nevertheless this is ok for all OSs, I don't know whether
> > initializing TimestampTz(double, int64 is ok) field with 8 bytes
> > zeros is OK or not, for all platforms. (It is ok for
> > IEEE754-binary64).
>
> Which code are you concerned about?

xlog.c: 5889
> beentry = pgstat_fetch_all_beentry();
>
> for (i = 0; i < MaxBackends; i++, beentry++)
> {
> xtime = beentry->st_xact_end_timestamp;

I think the last line in quoted code above reads possibly
zero-initialized double (or int64) value, then the doubted will
be compared and copied to another double.

> if (result < xtime)
> result = xtime;
>

> No, st_changecount is used to read st_xact_end_timestamp.
> st_xact_end_timestamp is read from the shmem to the local memory
> in pgstat_read_current_status(), and this function always checks
> st_changecount when reading the shmem value.

Yes, I confirmed that pg_lats_xact_insert_timestamp looks local
copy of BackendStatus. I've found it not unnecessary.

--
Kyotaro Horiguchi
NTT Open Source Software Center