Обсуждение: small typo in src/backend/access/transam/xlog.c
Hi
in void
BootStrapXLOG(void)* to seed it other than the system clock value...) The upper half of the
* uint64 value is just the tv_sec part, while the lower half is the XOR
* of tv_sec and tv_usec. This is to ensure that we don't lose uniqueness
* unnecessarily if "uint64" is really only 32 bits wide. A person
* knowing this encoding can determine the initialization time of the
* installation, which could perhaps be useful sometimes.
*/
gettimeofday(&tv, NULL);
sysidentifier = ((uint64) tv.tv_sec) << 32;
sysidentifier |= (uint32) (tv.tv_sec | tv.tv_usec);
should be
sysidentifier |= (uint32) (tv.tv_sec ^ tv.tv_usec);
Regardssysidentifier |= (uint32) (tv.tv_sec ^ tv.tv_usec);
On Mon, Jul 22, 2013 at 6:45 AM, didier <did447@gmail.com> wrote: > Hi > > in void > BootStrapXLOG(void) > > * to seed it other than the system clock value...) The upper half of > the > * uint64 value is just the tv_sec part, while the lower half is the > XOR > * of tv_sec and tv_usec. This is to ensure that we don't lose > uniqueness > * unnecessarily if "uint64" is really only 32 bits wide. A person > * knowing this encoding can determine the initialization time of > the > * installation, which could perhaps be useful sometimes. > */ > gettimeofday(&tv, NULL); > sysidentifier = ((uint64) tv.tv_sec) << 32; > sysidentifier |= (uint32) (tv.tv_sec | tv.tv_usec); > > should be > sysidentifier |= (uint32) (tv.tv_sec ^ tv.tv_usec); And why is that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-07-22 15:55:46 -0400, Robert Haas wrote: > On Mon, Jul 22, 2013 at 6:45 AM, didier <did447@gmail.com> wrote: > > Hi > > > > in void > > BootStrapXLOG(void) > > > > * to seed it other than the system clock value...) The upper half of > > the > > * uint64 value is just the tv_sec part, while the lower half is the > > XOR > > * of tv_sec and tv_usec. This is to ensure that we don't lose > > uniqueness > > * unnecessarily if "uint64" is really only 32 bits wide. A person > > * knowing this encoding can determine the initialization time of > > the > > * installation, which could perhaps be useful sometimes. > > */ > > gettimeofday(&tv, NULL); > > sysidentifier = ((uint64) tv.tv_sec) << 32; > > sysidentifier |= (uint32) (tv.tv_sec | tv.tv_usec); > > > > should be > > sysidentifier |= (uint32) (tv.tv_sec ^ tv.tv_usec); > > And why is that? The comment above tells: "while the lower half is the XOR of tv_sec and tv_usec." I don't think it really matters. the bitwise OR has the tenency to collect too many set bits, but ... who cares? On the other hand, changing it is easy and shouldn't cause any problems. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2013-07-22 15:55:46 -0400, Robert Haas wrote: >> And why is that? > The comment above tells: "while the lower half is the XOR of tv_sec and > tv_usec." Yeah, the code doesn't match the comment; this mistake seems to be aboriginal. > I don't think it really matters. the bitwise OR has the tenency to > collect too many set bits, but ... who cares? This is making the value less unique than intended, so I think it's worth doing something about. However, it's also worth noting that the intended behavior (as described by the comment) was designed to allow for the possibility that "uint64" is really only 32 bits --- a possibility we stopped supporting several versions ago. So rather than just quickly s/|/^/, maybe we should step back and think about whether we want to change the sysid generation algorithm altogether. We could for instance keep the high half as tv_sec, while making the low half be something like (tv_usec << 12) | (getpid() & 0xfff). This would restore the intended ability to reverse-engineer the exact creation time from the sysidentifier, and also add a little more uniqueness by way of the creating process's PID. (Note tv_usec must fit in 20 bits.) regards, tom lane
On Mon, Jul 22, 2013 at 07:32:20PM -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2013-07-22 15:55:46 -0400, Robert Haas wrote: > >> And why is that? > > > The comment above tells: "while the lower half is the XOR of tv_sec and > > tv_usec." > > Yeah, the code doesn't match the comment; this mistake seems to be > aboriginal. > > > I don't think it really matters. the bitwise OR has the tenency to > > collect too many set bits, but ... who cares? > > This is making the value less unique than intended, so I think it's > worth doing something about. However, it's also worth noting that the > intended behavior (as described by the comment) was designed to allow > for the possibility that "uint64" is really only 32 bits --- a > possibility we stopped supporting several versions ago. So rather than > just quickly s/|/^/, maybe we should step back and think about whether > we want to change the sysid generation algorithm altogether. > > We could for instance keep the high half as tv_sec, while making the low > half be something like (tv_usec << 12) | (getpid() & 0xfff). This would > restore the intended ability to reverse-engineer the exact creation time > from the sysidentifier, and also add a little more uniqueness by way of > the creating process's PID. (Note tv_usec must fit in 20 bits.) Can someone make a change here so we can close the issue? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes: > On Mon, Jul 22, 2013 at 07:32:20PM -0400, Tom Lane wrote: >> We could for instance keep the high half as tv_sec, while making the low >> half be something like (tv_usec << 12) | (getpid() & 0xfff). This would >> restore the intended ability to reverse-engineer the exact creation time >> from the sysidentifier, and also add a little more uniqueness by way of >> the creating process's PID. (Note tv_usec must fit in 20 bits.) > Can someone make a change here so we can close the issue? Done. regards, tom lane