[PostgreSQL-Hackers] making write location work (was: Efficient transaction-controlled synchronous replication)

The protocol supports different write/fsync values, so the view should
display them.
We don't know what the standby end will be doing with the data in all cases.

For the main server, making the additional change will just decrease
performance, for no benefit.

In the future we would have a parameter that says how often we send
replies, but there's no point having a parameter if there is only one
meaningful value for standby servers currently.

Please leave this as it is now.

That's exactly the point. Currently, we have a protocol that supports
different write and fsync values, but the code as written does not
actually ever send a reply at any time when the two values can ever be
different. So there is no point in sending both of them. The write
location is completely redundant with the fsync location and therefore
completely useless. We shouldn't bother sending the value twice, or
displaying it twice, if it's absolutely 100% guaranteed to be
identical in every case.

The point of the patch that I posted is that it restores the previous
behavior, where we send an update before flushing WAL and again after
flushing WAL. If we do that, then the write location can be ahead of
the flush location when we've written but not flushed. If we don't do
that, and only send replies after flushing everything, then the two
fields are perforce always the same on the master. I don't see that
as being a useful behavior, and in fact I think it could be quite
confusing. Someone might assume that if we bother to expose both a
write_location and a flush_location, they are somehow different.

As of 9.1, we now support other tools that use the protocol, so you
cannot assume you know what is being sent, just because one sender has
certain characteristics.
They can be in 9.1 and almost certainly will be in 9.2

Oh, really? Is this strictly hypothetical or is such a beast
planned/already in existence?

I'm just afraid this is going to be confusing to users who will expect
it to do something that it doesn't.

Ask Magnus.

In any case, that's not the only argument for keeping it. We introduce
the view in this release and I would like it to stay the same from
now, since we know we will need that info later.

No more minor tweaks, please.

At least as I understand it, it's not our project policy to carry
around code that doesn't accomplish anything useful. I have no
objection to keeping the field; I simply think that if we're going to
have it, we should make it work, as in fact it did before you changed
it without discussion. You haven't offered any evidence at all that
it introduces any kind of a performance regression AT ALL, much less
that such any such regression can't be trivially patched around by
making SyncRepReleaseWaiters exit quickly if the flush LSN hasn't
advanced. The onus is as much on you to justify the change as it is
on me to justify changing it back.

What a stupid conversation.

There's no onus on me to have to keep justifying to you why the code
is the way it is, but I do.

If you want to make a change that I already know reduces performance,
you have to have a good reason. So far, you don't.

Stop fussing and wrap the release.

I agree with Robert. It's useless and confusing to send the same
location as flush_location as write_location redundantly. We should
either remove write_location from the pg_stat_replication view and
the protocol at all, or apply the proposed patch.

Regards,

You may be confused, but that doesn't mean its useless.

The protocol supports sending two values, so two are displayed.

If you wish to remove one from the display then that only makes sense
if you also prevent the protocol from supporting two values.

There is no benefit in doing that, so why do it? We are going to put
that back in 9.2 if you remove it now. Why not leave it, so we don't
need to rewrite all the monitoring tools that will use this view?

Just say this in the docs. "Currently, standbys return the same value
for write and flush locations and so in most cases these values will
be the same. It is possible to write a user program that sends replies
at different times and in that case the values would differ. In later
release these values will differ for standbys. when more options are
available."

It's almost certain that Magnus will fix the bug in pg_xlogstream (or
whatever its called) and a tool will be available, so lets leave this.

If we're going to put it back in 9.2, then we shouldn't remove it now.
We should just make it work. It's a three line patch. If 9.2 is
going to meaningfully distinguish between write location and flush
location, then we may as well do the same thing in 9.1. Then we'll be
ahead of the game: not only will the view have the same columns in
both releases, but they'll actually have the same semantics in both
releases.

What are you planning to use write_location for? BTW, I'm thinking to
add recv_location (not write_location) in 9.2 to support another sync rep
mode which makes transactions wait until the standby has received
(not fsync'd) the WAL. You're planning the same?
+1

Regards,

I think we have adequate consensus on this topic, so committed.

What if we didn't actually receive any new data?

regards, tom lane

The portion of the code immediately preceding what's included in the
diff guards against that, and there is a second guard in
XLogWalRcvSendReply().

That hardly seems like a convincing response. Adding a column to a
view when the column contains meaninful values seems less likely to
break things than initially adding it with a different value,
identical to another column, and then changing the semantics.

+1 for either dropping it or making it work.

-Kevin

That's my feeling also. There is *no* reason that we can't add a column
to the status view later, and every probability that we will find
reasons other than this to do so. So if the column isn't going to
provide useful information in 9.1, let's just drop it.

regards, tom lane

+1

Joe