On Thu, Nov 26, 2015 at 8:17 AM, Michael Paquier <michael.paqu...@gmail.com> wrote:
> > > On Wed, Nov 25, 2015 at 11:08 PM, Magnus Hagander <mag...@hagander.net> > wrote: > >> On Wed, Nov 25, 2015 at 3:03 PM, Michael Paquier < >> michael.paqu...@gmail.com> wrote: >> >>> On Wed, Nov 25, 2015 at 7:18 PM, Magnus Hagander <mag...@hagander.net> >>> wrote: >>> >> In particular, it seems that in InitWalSenderSlot, we only initialize the >>>> sent location. Perhaps this is needed? >>>> >>> >>> Yes, that would be nice to start with a clean state. At the same time, I >>> am noticing that pg_stat_get_wal_senders() is comparing flush, apply and >>> write directly with 0. I think those should be InvalidXLogRecPtr. Combined >>> with your patch it gives the attached. >>> >> >> Good point. >> >> Another thought - why do we leave 0/0 in sent location, but turn the >> other three fields to NULL if it's invalid? That seems inconsistent. Is >> there a reason for that, or should we fix that as well while we're at it? >> > > In some code paths, values are expected to be equal to 0 as XLogRecPtr > values are doing direct comparisons with each other, so you can think that > 0 for a XLogRecPtr is slightly different from InvalidXLogRecPtr that could > be expected to be invalid but *not* minimal, imagine for example that we > decide at some point to redefine it as INT64_MAX. See for example > receivedUpto in xlog.c or WalSndWaitForWal in walsender.c. Honestly, I > think that it would be nice to make all the logic consistent under a unique > InvalidXLogRecPtr banner that is defined at 0 once and for all, adding at > the same time a comment in xlogdefs.h mentioning that this should not be > redefined to a higher value because comparison logic of XlogRecPtr's depend > on that. We have actually a similar constraint with TimeLineID = 0 that is > equivalent to infinite. Patch attached following those lines, which should > be backpatched for correctness. > I'm only talking about the actual value in pg_stat_replication here, not what we are using internally. These are two different things of course - let's keep them separate for now. In pg_stat_replication, we explicitly check for InvalidXLogRecPtr and then explicitly set the resulting value to NULL in the SQL return. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
