Add PgLsn::INVALID; Add PgTimestamp for standby_status_update

[ericseppanen]

PgLsn:

• The postgres C code has InvalidXLogRecPtr, and I find this easier to understand than PgLsn(0).

PgTimestamp:

• This is a work in progress! I can re-send after the two problems below are resolved.
• Ensure that timestamps are always calculated the right way; the caller doesn't need to know the epoch details.
• PROBLEM: This probably belongs somewhere in postgres-types? Unless this kind of timestamp is specific to replication?
• PROBLEM: There's already a postgres_types::Timestamp, so there's some risk of confusion. Maybe if this were named ReplicationTimestamp instead? I'm not sure what the right name should be.

[ericseppanen]

cc @vitaly-burovoy, @sfackler

These are a couple of (proposed) small additions to the replication PR, sfackler#752

[vitaly-burovoy]

What is a reason for adding the constant PgLsn::INVALID?
If I'm not mistaken, InvalidXLogRecPtr is for internal purpose¹, it should not be sent to a client or a backend…

—
¹ https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/access/xlogdefs.h#l23

/*
 * Zero is used indicate an invalid pointer. Bootstrap skips the first possible
 * WAL segment, initializing the first WAL page at WAL segment size, so no XLOG
 * record can begin at zero.
 */

[ericseppanen]

If I'm not mistaken, InvalidXLogRecPtr is for internal purpose¹, it should not be sent to a client or a backend…

A fair question.

Some context might be helpful. I am currently copying the behavior from postgres' pg_basebackup, which has this code:

replybuf[len] = 'r';
len += 1;
fe_sendint64(blockpos, &replybuf[len]); /* write */
len += 8;
if (reportFlushPosition)
	fe_sendint64(lastFlushPosition, &replybuf[len]);	/* flush */
else
	fe_sendint64(InvalidXLogRecPtr, &replybuf[len]);	/* flush */
len += 8;
fe_sendint64(InvalidXLogRecPtr, &replybuf[len]);	/* apply */
len += 8;
fe_sendint64(now, &replybuf[len]);	/* sendTime */
len += 8;
replybuf[len] = replyRequested ? 1 : 0; /* replyRequested */
len += 1;

So it always sends InvalidXLogRecPtr for the "apply" field and sometimes sends InvalidXLogRecPtr for the "flush" field.

This C code defines the values that would be sent by this PR's standby_status_update:

fn standby_status_update(
        self: Pin<&mut Self>,
        write_lsn: PgLsn,
        flush_lsn: PgLsn,
        apply_lsn: PgLsn,
        ts: i64,
        reply: u8,
    )

... so to emulate the code above I need to put an LSN of 0 into one or more fields of standby_status_update().

It might be that this is a very exotic use case, and doesn't justify putting this in the library. I can drop this change if it seems questionable.

[ericseppanen]

Another strategy might be to change some or all of the parameters of standby_status_update to be Option<PgLsn>, and inside standby_status_update convert None to PgLsn(0).

[petrosagg]

What do you think of changing XLogData's timestamp field, standby_status_update, and hot_standby_feedback to all present and receive std::time::SystemTime and do the right thing internally? This way we don't need to expose another type and users don't have to think about the different epoch

[ericseppanen]

Yes, I agree SystemTime would be much better. Want me to write that and send a new PR?

What do you think about the idea of standby_status_update accepting Option<PgLsn> parameters?

[petrosagg]

I implemented the SystemTime idea here 1a0a907, let me know what you think

Regarding the Option<PgLsn>, I think I like your initial idea of offering an INVALID const item in the type more.
It would have been nice if the PgLsn type was originally implemented as a NonZeroU64 and leave the zero to be represented by the None variant of an option though

[ericseppanen]

I have started to think that INVALID might not have been such a great idea.

Reason: once it exists, people might be tempted to pass it as a parameter anywhere that PgLsn will fit: functions, structs, ...

And then I think, it would have been nice if the type system could clarify where it's OK to send INVALID and where it's not. Which is basically the definition of Option...

I like the idea of making PgLsn a NonZeroU64. Is it too late to make that change?

[ericseppanen]

Even if NonZeroU64 isn't going to happen, I'm still in favor of Option<PgLsn>.

[petrosagg]

Are all the fields able to accept this invalid Lsn? In the base backup example you sent it's definitely the case for apply and flush, do you know about write?

[ericseppanen]

Sorry, I don't know whether the write LSN could ever be 0.

@hlinnaka might know? He once wrote this:

In a normal streaming replication replica, the values mean:
write_lsn: the WAL has been received up to this point. It's not durable stored on disk yet, so the server might still lose it if it crashes.
flush_lsn: the WAL has been flushed to disk up to this point. All WAL <= flush_lsn should be safe even if the replica crashes
apply_lsn: the WAL has been applied up to this point. If you connect to the replica, you should see the effects of all WAL < apply_lsn.

It seems a little bonkers for a peer to be receiving data, but claiming it has never received anything. I'm tempted to say this can never be zero. Is it possible for this to be a new connection, that has never received any data but has received a keepalive message?