On Wed, Nov 25, 2015 at 7:18 PM, Magnus Hagander <magnus@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.
