Re: XLByte* usage

Hi,

Now that XLRecPtr's are plain 64bit integers what are we supposed to use
in code comparing and manipulating them? There already is plenty example
of both, but I would like new code to go into one direction not two...

I personally find direct comparisons/manipulations far easier to read
than the XLByte* equivalents.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 16.12.2012 16:16, Andres Freund wrote:
> Now that XLRecPtr's are plain 64bit integers what are we supposed to use
> in code comparing and manipulating them? There already is plenty example
> of both, but I would like new code to go into one direction not two...
>
> I personally find direct comparisons/manipulations far easier to read
> than the XLByte* equivalents.

I've still used XLByte* macros, but I agree that plain < = > are easier
to read. +1 for using < = > in new code.

- Heikki

On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 16.12.2012 16:16, Andres Freund wrote:
>>
>> Now that XLRecPtr's are plain 64bit integers what are we supposed to use
>> in code comparing and manipulating them? There already is plenty example
>> of both, but I would like new code to go into one direction not two...
>>
>> I personally find direct comparisons/manipulations far easier to read
>> than the XLByte* equivalents.
>
>
> I've still used XLByte* macros, but I agree that plain < = > are easier to
> read. +1 for using < = > in new code.
>

Do we ever see us changing this from 64-bit integers to something else
? If so, a macro would be much better.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

On 17.12.2012 11:04, Pavan Deolasee wrote:
> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> On 16.12.2012 16:16, Andres Freund wrote:
>>>
>>> Now that XLRecPtr's are plain 64bit integers what are we supposed to use
>>> in code comparing and manipulating them? There already is plenty example
>>> of both, but I would like new code to go into one direction not two...
>>>
>>> I personally find direct comparisons/manipulations far easier to read
>>> than the XLByte* equivalents.
>>
>> I've still used XLByte* macros, but I agree that plain< => are easier to
>> read. +1 for using< => in new code.
>
> Do we ever see us changing this from 64-bit integers to something else
> ? If so, a macro would be much better.

I don't see us changing it again any time soon. Maybe in 20 years time
people will start overflowing 2^64 bytes of WAL generated in the
lifetime of a database, but I don't think we need to start preparing for
that yet.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 17.12.2012 11:04, Pavan Deolasee wrote:
>> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)com> wrote:
>>> I've still used XLByte* macros, but I agree that plain <=> are easier to
>>> read. +1 for using <=> in new code.

>> Do we ever see us changing this from 64-bit integers to something else
>> ? If so, a macro would be much better.

> I don't see us changing it again any time soon. Maybe in 20 years time
> people will start overflowing 2^64 bytes of WAL generated in the
> lifetime of a database, but I don't think we need to start preparing for
> that yet.

Note that to get to 2^64 in twenty years, an installation would have had
to have generated an average of 29GB of WAL per second, 24x7 for the
entire twenty years, with never a dump-and-reload. We're still a few
orders of magnitude away from needing to think about this.

But, if the day ever comes when 64 bits doesn't seem like enough, I bet
we'd move to 128-bit integers, which will surely be available on all
platforms by then. So +1 for using plain comparisons --- in fact, I'd
vote for running around and ripping out the macros altogether. I had
already been thinking of fixing the places that are still using memset
to initialize XLRecPtrs to "invalid".

regards, tom lane

On 2012-12-17 12:47:41 -0500, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> > On 17.12.2012 11:04, Pavan Deolasee wrote:
> >> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
> >> <hlinnakangas(at)vmware(dot)com> wrote:
> >>> I've still used XLByte* macros, but I agree that plain <=> are easier to
> >>> read. +1 for using <=> in new code.
>
> >> Do we ever see us changing this from 64-bit integers to something else
> >> ? If so, a macro would be much better.
>
> > I don't see us changing it again any time soon. Maybe in 20 years time
> > people will start overflowing 2^64 bytes of WAL generated in the
> > lifetime of a database, but I don't think we need to start preparing for
> > that yet.
>
> Note that to get to 2^64 in twenty years, an installation would have had
> to have generated an average of 29GB of WAL per second, 24x7 for the
> entire twenty years, with never a dump-and-reload. We're still a few
> orders of magnitude away from needing to think about this.

Agreed. And it seems achieving such rates would require architectural
changes that would make manually changing all those comparisons the
tiniest problem.

> But, if the day ever comes when 64 bits doesn't seem like enough, I bet
> we'd move to 128-bit integers, which will surely be available on all
> platforms by then. So +1 for using plain comparisons --- in fact, I'd
> vote for running around and ripping out the macros altogether. I had
> already been thinking of fixing the places that are still using memset
> to initialize XLRecPtrs to "invalid".

I thought about that and had guessed you would be against it because it
would cause useless diversion of the branches? Otherwise I am all for
it.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec 17, 2012 at 11:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> On 17.12.2012 11:04, Pavan Deolasee wrote:
>>> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
>>> <hlinnakangas(at)vmware(dot)com> wrote:
>>>> I've still used XLByte* macros, but I agree that plain <=> are easier to
>>>> read. +1 for using <=> in new code.
>
>>> Do we ever see us changing this from 64-bit integers to something else
>>> ? If so, a macro would be much better.
>
>> I don't see us changing it again any time soon. Maybe in 20 years time
>> people will start overflowing 2^64 bytes of WAL generated in the
>> lifetime of a database, but I don't think we need to start preparing for
>> that yet.
>
> Note that to get to 2^64 in twenty years, an installation would have had
> to have generated an average of 29GB of WAL per second, 24x7 for the
> entire twenty years, with never a dump-and-reload. We're still a few
> orders of magnitude away from needing to think about this.
>

I probably did not mean increasing that to beyond 64-bit. OTOH I
wondered if we would ever want to steal a few bits from the LSN field,
given the numbers you just put out. But it was more of a question than
objection.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

On Mon, Dec 17, 2012 at 11:26 PM, Pavan Deolasee
<pavan(dot)deolasee(at)gmail(dot)com> wrote:

>>
>
> I probably did not mean increasing that to beyond 64-bit. OTOH I
> wondered if we would ever want to steal a few bits from the LSN field,
> given the numbers you just put out. But it was more of a question than
> objection.
>

BTW, now that XLogRecPtr is uint64, can't we change the pd_lsn field
to use the same type ? At least the following comment in bufpage.h
looks outdated or at the minimum needs some explanation as why LSN in
the page header needs to split into two 32-bit values.

123 /* for historical reasons, the LSN is stored as two 32-bit values. */
124 typedef struct
125 {
126 uint32 xlogid; /* high bits */
127 uint32 xrecoff; /* low bits */
128 } PageXLogRecPtr;

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2012-12-17 12:47:41 -0500, Tom Lane wrote:
>> But, if the day ever comes when 64 bits doesn't seem like enough, I bet
>> we'd move to 128-bit integers, which will surely be available on all
>> platforms by then. So +1 for using plain comparisons --- in fact, I'd
>> vote for running around and ripping out the macros altogether. I had
>> already been thinking of fixing the places that are still using memset
>> to initialize XLRecPtrs to "invalid".

> I thought about that and had guessed you would be against it because it
> would cause useless diversion of the branches? Otherwise I am all for
> it.

That's the only argument I can see against doing it --- but Heikki's
patch was already pretty invasive in the same areas this would touch,
so I'm thinking this won't make back-patching much worse. The
notational simplification seems worth it.

regards, tom lane

On 2012-12-17 23:45:51 +0530, Pavan Deolasee wrote:
> On Mon, Dec 17, 2012 at 11:26 PM, Pavan Deolasee
> <pavan(dot)deolasee(at)gmail(dot)com> wrote:
>
> >>
> >
> > I probably did not mean increasing that to beyond 64-bit. OTOH I
> > wondered if we would ever want to steal a few bits from the LSN field,
> > given the numbers you just put out. But it was more of a question than
> > objection.
> >
>
> BTW, now that XLogRecPtr is uint64, can't we change the pd_lsn field
> to use the same type ? At least the following comment in bufpage.h
> looks outdated or at the minimum needs some explanation as why LSN in
> the page header needs to split into two 32-bit values.
>
> 123 /* for historical reasons, the LSN is stored as two 32-bit values. */
> 124 typedef struct
> 125 {
> 126 uint32 xlogid; /* high bits */
> 127 uint32 xrecoff; /* low bits */
> 128 } PageXLogRecPtr;

pg_upgrade'ability. The individual bytes aren't necessarily laid out the
same with two such uint32s as with one uint64.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> writes:
> BTW, now that XLogRecPtr is uint64, can't we change the pd_lsn field
> to use the same type ?

No, at least not without breaking on-disk compatibility on little-endian
machines.

regards, tom lane

On 2012-12-17 13:16:47 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-12-17 12:47:41 -0500, Tom Lane wrote:
> >> But, if the day ever comes when 64 bits doesn't seem like enough, I bet
> >> we'd move to 128-bit integers, which will surely be available on all
> >> platforms by then. So +1 for using plain comparisons --- in fact, I'd
> >> vote for running around and ripping out the macros altogether. I had
> >> already been thinking of fixing the places that are still using memset
> >> to initialize XLRecPtrs to "invalid".
>
> > I thought about that and had guessed you would be against it because it
> > would cause useless diversion of the branches? Otherwise I am all for
> > it.
>
> That's the only argument I can see against doing it --- but Heikki's
> patch was already pretty invasive in the same areas this would touch,
> so I'm thinking this won't make back-patching much worse.

I thought a while about this for while and decided its worth trying to
this before the next review round of xlogreader. Even if it causes some
breakage there. Doing it this way round seems less likely to introduce
bugs, especially if somebody else would go round and do this after the
next xlogreader review round but before committing it.

Attached is
1) removal of MemSet(&ptr, 0, sizeof(XLogRecPtr)
2) removal of XLByte(EQ|LT|LE|Advance)
3) removal of the dead NextLogPage I noticed along the way

In 2) unfortunately one has to make decision in which way to simplify
negated XLByte(LT|LE) expressions. I tried to make that choice very
careful and when over every change several times after that, so I hope
there aren't any bad changes, but more eyeballs are needed.

> The notational simplification seems worth it.

After doing this: Definitely. Imo some of the conditions are way much
easier to read now. Perhaps I am just bad in reading negations though...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> In 2) unfortunately one has to make decision in which way to simplify
> negated XLByte(LT|LE) expressions. I tried to make that choice very
> careful and when over every change several times after that, so I hope
> there aren't any bad changes, but more eyeballs are needed.

+ if (lsn > PageGetLSN(page))
+ if (lsn >= PageGetLSN(page))
+ if (lsn <= PageGetLSN(page))
+ if (max_lsn < this_lsn)

My only comment here would be that I would like it much better that the
code always use the same operators, and as we read from left to right,
that we pick < and <=.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 2012-12-18 13:14:10 +0100, Dimitri Fontaine wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > In 2) unfortunately one has to make decision in which way to simplify
> > negated XLByte(LT|LE) expressions. I tried to make that choice very
> > careful and when over every change several times after that, so I hope
> > there aren't any bad changes, but more eyeballs are needed.
>
> + if (lsn > PageGetLSN(page))
> + if (lsn >= PageGetLSN(page))
> + if (lsn <= PageGetLSN(page))
> + if (max_lsn < this_lsn)
>
> My only comment here would be that I would like it much better that the
> code always use the same operators, and as we read from left to right,
> that we pick < and <=.

I did that in most places (check the xlog.c changes), but in this case
it didn't seem to be appropriate because because that would have meant
partially reversing the order of operands which would have looked
confusing as well, given this check is a common patter across nearly
every xlog replay function.
Imo its a good idea trying to choose < or <= as operator but its also
important to keep the order consistent inside a single function/similar
functions.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund escribió:
> On 2012-12-17 13:16:47 -0500, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > On 2012-12-17 12:47:41 -0500, Tom Lane wrote:
> > >> But, if the day ever comes when 64 bits doesn't seem like enough, I bet
> > >> we'd move to 128-bit integers, which will surely be available on all
> > >> platforms by then. So +1 for using plain comparisons --- in fact, I'd
> > >> vote for running around and ripping out the macros altogether. I had
> > >> already been thinking of fixing the places that are still using memset
> > >> to initialize XLRecPtrs to "invalid".
> >
> > > I thought about that and had guessed you would be against it because it
> > > would cause useless diversion of the branches? Otherwise I am all for
> > > it.
> >
> > That's the only argument I can see against doing it --- but Heikki's
> > patch was already pretty invasive in the same areas this would touch,
> > so I'm thinking this won't make back-patching much worse.
>
> I thought a while about this for while and decided its worth trying to
> this before the next review round of xlogreader.

I have applied these three patches, after merging for recent changes.
Thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-28 14:59:50 -0300, Alvaro Herrera wrote:
> Andres Freund escribió:
> > On 2012-12-17 13:16:47 -0500, Tom Lane wrote:
> > > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > > On 2012-12-17 12:47:41 -0500, Tom Lane wrote:
> > > >> But, if the day ever comes when 64 bits doesn't seem like enough, I bet
> > > >> we'd move to 128-bit integers, which will surely be available on all
> > > >> platforms by then. So +1 for using plain comparisons --- in fact, I'd
> > > >> vote for running around and ripping out the macros altogether. I had
> > > >> already been thinking of fixing the places that are still using memset
> > > >> to initialize XLRecPtrs to "invalid".
> > >
> > > > I thought about that and had guessed you would be against it because it
> > > > would cause useless diversion of the branches? Otherwise I am all for
> > > > it.
> > >
> > > That's the only argument I can see against doing it --- but Heikki's
> > > patch was already pretty invasive in the same areas this would touch,
> > > so I'm thinking this won't make back-patching much worse.
> >
> > I thought a while about this for while and decided its worth trying to
> > this before the next review round of xlogreader.
>
> I have applied these three patches, after merging for recent changes.
> Thanks.

Thanks!

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services