Thread

1. XLByte* usage

   Andres Freund <andres@2ndquadrant.com> — 2012-12-16T14:16:25Z

   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
   
   
   

2. Re: XLByte* usage

   Heikki Linnakangas <hlinnakangas@vmware.com> — 2012-12-17T08:30:18Z

   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
   
   
   

3. Re: XLByte* usage

   Pavan Deolasee <pavan.deolasee@gmail.com> — 2012-12-17T09:04:41Z

   On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
   <hlinnakangas@vmware.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
   
   
   

4. Re: XLByte* usage

   Heikki Linnakangas <hlinnakangas@vmware.com> — 2012-12-17T09:43:34Z

   On 17.12.2012 11:04, Pavan Deolasee wrote:
   > On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
   > <hlinnakangas@vmware.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
   
   
   

5. Re: XLByte* usage

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-17T17:47:41Z

   Heikki Linnakangas <hlinnakangas@vmware.com> writes:
   > On 17.12.2012 11:04, Pavan Deolasee wrote:
   >> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
   >> <hlinnakangas@vmware.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
   
   
   

6. Re: XLByte* usage

   Andres Freund <andres@2ndquadrant.com> — 2012-12-17T17:56:04Z

   On 2012-12-17 12:47:41 -0500, Tom Lane wrote:
   > Heikki Linnakangas <hlinnakangas@vmware.com> writes:
   > > On 17.12.2012 11:04, Pavan Deolasee wrote:
   > >> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
   > >> <hlinnakangas@vmware.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
   
   
   

7. Re: XLByte* usage

   Pavan Deolasee <pavan.deolasee@gmail.com> — 2012-12-17T17:56:21Z

   On Mon, Dec 17, 2012 at 11:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Heikki Linnakangas <hlinnakangas@vmware.com> writes:
   >> On 17.12.2012 11:04, Pavan Deolasee wrote:
   >>> On Mon, Dec 17, 2012 at 2:00 PM, Heikki Linnakangas
   >>> <hlinnakangas@vmware.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
   
   
   

8. Re: XLByte* usage

   Pavan Deolasee <pavan.deolasee@gmail.com> — 2012-12-17T18:15:51Z

   On Mon, Dec 17, 2012 at 11:26 PM, Pavan Deolasee
   <pavan.deolasee@gmail.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
   
   
   

9. Re: XLByte* usage

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-17T18:16:47Z

   Andres Freund <andres@2ndquadrant.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
   
   
   

10. Re: XLByte* usage

    Andres Freund <andres@2ndquadrant.com> — 2012-12-17T18:30:10Z

    On 2012-12-17 23:45:51 +0530, Pavan Deolasee wrote:
    > On Mon, Dec 17, 2012 at 11:26 PM, Pavan Deolasee
    > <pavan.deolasee@gmail.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
    
    
    

11. Re: XLByte* usage

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-17T18:39:17Z

    Pavan Deolasee <pavan.deolasee@gmail.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
    
    
    

12. Re: XLByte* usage

    Andres Freund <andres@2ndquadrant.com> — 2012-12-17T22:55:11Z

    On 2012-12-17 13:16:47 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.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
    

13. Re: XLByte* usage

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-12-18T12:14:10Z

    Andres Freund <andres@2ndquadrant.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
    
    
    

14. Re: XLByte* usage

    Andres Freund <andres@2ndquadrant.com> — 2012-12-18T12:25:04Z

    On 2012-12-18 13:14:10 +0100, Dimitri Fontaine wrote:
    > Andres Freund <andres@2ndquadrant.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
    
    
    

15. Re: XLByte* usage

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-28T17:59:50Z

    Andres Freund escribió:
    > On 2012-12-17 13:16:47 -0500, Tom Lane wrote:
    > > Andres Freund <andres@2ndquadrant.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
    
    
    

16. Re: XLByte* usage

    Andres Freund <andres@2ndquadrant.com> — 2012-12-28T18:45:17Z

    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@2ndquadrant.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