Horton

Thread

  1. protocol-level wait-for-LSN

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-28T15:51:44Z

    This is something I hacked together on the way back from pgconf.eu. 
It's highly experimental.

The idea is to do the equivalent of pg_wal_replay_wait() on the protocol 
level, so that it is ideally fully transparent to the application code. 
The application just issues queries, and they might be serviced by a 
primary or a standby, but there is always a correct ordering of reads 
after writes.

Additionally, I'm exploring whether this is an idea for a protocol 
extension that might be a bit more complex than, say, longer cancel 
keys, something we could have a discussion around protocol versioning 
around.

The patch adds a protocol extension called _pq_.wait_for_lsn as well as 
a libpq connection option wait_for_lsn to activate the same.  (Use e.g., 
psql -d 'wait_for_lsn=1'.)

With this protocol extension, two things are changed:

- The ReadyForQuery message sends back the current LSN.

- The Query message sends an LSN to wait for.  (This doesn't handle the 
extended query protocol yet.)

To make any real use of this, you'd need some middleware, like a hacked 
pgbouncer, that transparently redirects queries among primaries and 
standbys, which doesn't exist yet.  But if it did, I imagine it could be 
pretty useful.

There might be other ways to slice this.  Instead of using a 
hypothetical middleware, the application would use two connections, one 
for writing, one for reading, and the LSN would be communicated between 
the two.  I imagine in this case, at least the one half of the protocol, 
shipping the current LSN with ReadyForQuery, could be useful, instead of 
requiring application code to issue pg_current_wal_insert_lsn() explicitly.

Thoughts?

  2. Re: protocol-level wait-for-LSN

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-10-28T16:58:08Z

    On 28/10/2024 17:51, Peter Eisentraut wrote:
> This is something I hacked together on the way back from pgconf.eu. It's 
> highly experimental.
> 
> The idea is to do the equivalent of pg_wal_replay_wait() on the protocol 
> level, so that it is ideally fully transparent to the application code. 
> The application just issues queries, and they might be serviced by a 
> primary or a standby, but there is always a correct ordering of reads 
> after writes.
> 
> Additionally, I'm exploring whether this is an idea for a protocol 
> extension that might be a bit more complex than, say, longer cancel 
> keys, something we could have a discussion around protocol versioning 
> around.
> 
> The patch adds a protocol extension called _pq_.wait_for_lsn as well as 
> a libpq connection option wait_for_lsn to activate the same.  (Use e.g., 
> psql -d 'wait_for_lsn=1'.)
> 
> With this protocol extension, two things are changed:
> 
> - The ReadyForQuery message sends back the current LSN.

+1

> - The Query message sends an LSN to wait for.  (This doesn't handle the 
> extended query protocol yet.)

I'd suggest adding a new message type for this, so that it works the 
same with simple and extended query. Or if you just want to wait without 
issuing any query.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  3. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-28T17:02:30Z

    On Mon, 28 Oct 2024 at 16:51, Peter Eisentraut <peter@eisentraut.org> wrote:
> The idea is to do the equivalent of pg_wal_replay_wait() on the protocol
> level, so that it is ideally fully transparent to the application code.
> The application just issues queries, and they might be serviced by a
> primary or a standby, but there is always a correct ordering of reads
> after writes.

Sounds super useful. This came up in the Unconference session about
protocols on PGConf.dev too. I'll

> There might be other ways to slice this.  Instead of using a
> hypothetical middleware, the application would use two connections, one
> for writing, one for reading, and the LSN would be communicated between
> the two.  I imagine in this case, at least the one half of the protocol,
> shipping the current LSN with ReadyForQuery, could be useful, instead of
> requiring application code to issue pg_current_wal_insert_lsn() explicitly.

I think this usecase is already super useful by itself. And having
both directions would still be preferred I think.

  4. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-28T17:07:57Z

    On Mon, 28 Oct 2024 at 17:58, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > - The Query message sends an LSN to wait for.  (This doesn't handle the
> > extended query protocol yet.)
>
> I'd suggest adding a new message type for this, so that it works the
> same with simple and extended query. Or if you just want to wait without
> issuing any query.

I imagine a libpq interface like this.

lsn = PQcurrentLSN(primaryConn)
PQsendWaitLSN(secondaryConn, lsn)
PQsendQuery(secondaryConn, ...)

One thing I'm wondering is if the current lsn could be a read-only GUC
that is reported through ParameterStatus. Because a downside of making
it part of ReadyForQuery is that you only get a ReadyForQuery at the
end of a pipeline, while a pipeline can contain multiple commits if
you use explicit BEGIN/COMMIT in your pipeline. It might be nice to be
able to wait on those commits before you've received ReadyForQuery. On
the other hand, that seems like a rather exotic usecase that maybe is
not worth thinking about too much.

  5. Re: protocol-level wait-for-LSN

    Tatsuo Ishii <ishii@postgresql.org> — 2024-10-29T05:06:00Z

    > The patch adds a protocol extension called _pq_.wait_for_lsn as well
> as a libpq connection option wait_for_lsn to activate the same.  (Use
> e.g., psql -d 'wait_for_lsn=1'.)
> 
> With this protocol extension, two things are changed:
> 
> - The ReadyForQuery message sends back the current LSN.

If other protocol extension X tries to add something to the
ReadyForQuery message too, what would happen?
Currently ReadyForQuery message is like this:

Byte1('Z')
Int32
Byte1

With the wait_for_lsn extension, It becomes:

Byte1('Z')
Int32
Byte1
String

Suppose the X extension wants to extend like this:

Byte1('Z')
Int32
Byte1
Int32

It seems impossible to coexist both.

Does this mean once the wait_for_lsn extension is brought into the
frontend/backend protocol specification, no other extensions that touch
ReadyForQuery cannot be defined?

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

  6. Re: protocol-level wait-for-LSN

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-29T06:28:51Z

    On 29.10.24 06:06, Tatsuo Ishii wrote:
>> The patch adds a protocol extension called _pq_.wait_for_lsn as well
>> as a libpq connection option wait_for_lsn to activate the same.  (Use
>> e.g., psql -d 'wait_for_lsn=1'.)
>>
>> With this protocol extension, two things are changed:
>>
>> - The ReadyForQuery message sends back the current LSN.
> 
> If other protocol extension X tries to add something to the
> ReadyForQuery message too, what would happen?

I think one would have to define that somehow.  If it's useful, the 
additional fields of both extensions could be appended, in some defined 
order.  But this is an interesting question to think about.

  7. Re: protocol-level wait-for-LSN

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2024-10-29T10:45:41Z

    On Mon, 28 Oct 2024, 16:51 Peter Eisentraut, <peter@eisentraut.org> wrote:
>
> This is something I hacked together on the way back from pgconf.eu.
> It's highly experimental.
>
> The idea is to do the equivalent of pg_wal_replay_wait() on the protocol
> level, so that it is ideally fully transparent to the application code.
> The application just issues queries, and they might be serviced by a
> primary or a standby, but there is always a correct ordering of reads
> after writes.

+1

> Thoughts?

I think it would be quite beneficial to include the cluster ID in
these messages, so that e.g. logical replication can be guaranteed to
be cought up to the recent commit when querying a sharded cluster.

So instead of just LSN, PostgreSQL would return the [cluster_id, LSN]
pair (maybe: pairs, given that you may want to have forward-only view
of a logical primary across 2 different logical subscribers; postgres
could supply a pair [cluster_id, LSN] for every logical subscriber
slot), while the "wait for LSN" protocol feature would accept a list
of these, waiting for the logical subscriptions and/or physical
replication that source their changes from those cluster_ids to catch
up to those LSNs.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

  8. Re: protocol-level wait-for-LSN

    Jesper Pedersen <jesper.pedersen@comcast.net> — 2024-10-29T16:03:04Z

    Hi Peter,

On 10/28/24 12:58 PM, Heikki Linnakangas wrote:
> I'd suggest adding a new message type for this, so that it works the 
> same with simple and extended query. Or if you just want to wait without 
> issuing any query.

I agree it is a good idea to have a feature like this.

However, I agree with Heikki that we should have a separate message type 
for this. There are a lot of protocol implementations outside of 
PostgreSQL/Core, and they would have to adjust based on the version 
number of Core itself if we add fields to existing message types.

Maybe there should be an "Extension ('x') (F)" message that only has a 
fixed "header", and the rest of the fields are based on the "header" 
limited by the message length field - sort of free-form. The result is 
returned as a "DataRow ('D') (B)" list.

Thanks for working on this !

Best regards,
  Jesper

  9. Re: protocol-level wait-for-LSN

    Jesper Pedersen <jesper.pedersen@comcast.net> — 2024-10-29T16:22:58Z

    Hi,

On 10/29/24 12:03 PM, Jesper Pedersen wrote:
> On 10/28/24 12:58 PM, Heikki Linnakangas wrote:
>> I'd suggest adding a new message type for this, so that it works the 
>> same with simple and extended query. Or if you just want to wait 
>> without issuing any query.
> 
> I agree it is a good idea to have a feature like this.
> 
> However, I agree with Heikki that we should have a separate message type 
> for this. There are a lot of protocol implementations outside of 
> PostgreSQL/Core, and they would have to adjust based on the version 
> number of Core itself if we add fields to existing message types.
> 
> Maybe there should be an "Extension ('x') (F)" message that only has a 
> fixed "header", and the rest of the fields are based on the "header" 
> limited by the message length field - sort of free-form. The result is 
> returned as a "DataRow ('D') (B)" list.
>

I understand that we need this to be "atomic" which is difficult with 
the above since we can already use "Query 'Q' (F)", but we need it at 
"ReadyForQuery 'Z' (B)" level.

Having a new "ReadyForQuery v2 ('z') (B)" would require changes to all 
non-Core implementations as well...

Likely a protocol v4 thing.

Best regards,
  Jesper

  10. Re: protocol-level wait-for-LSN

    Tatsuo Ishii <ishii@postgresql.org> — 2024-10-30T06:49:19Z

    >>> With this protocol extension, two things are changed:
>>>
>>> - The ReadyForQuery message sends back the current LSN.
>> If other protocol extension X tries to add something to the
>> ReadyForQuery message too, what would happen?
> 
> I think one would have to define that somehow.  If it's useful, the
> additional fields of both extensions could be appended, in some
> defined order.  But this is an interesting question to think about.

I think this kind of extension, which adds new filed to an existing
message type, should be implemented as v4 protocol.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

  11. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-30T08:41:03Z

    On Wed, 30 Oct 2024 at 07:49, Tatsuo Ishii <ishii@postgresql.org> wrote:
> > I think one would have to define that somehow.  If it's useful, the
> > additional fields of both extensions could be appended, in some
> > defined order.  But this is an interesting question to think about.
>
> I think this kind of extension, which adds new filed to an existing
> message type, should be implemented as v4 protocol.

Could you explain why you think a major version bump is needed? In
what situation do you care about this. Because for my usecases (client
implementations & pgbouncer) I don't think that would be necessary. If
a client doesn't send the _pq_.wait_for_lsn protocol parameter, it
will never receive this new version.

I don't really see a problem with having two protocol parameters
change the same message. Yes, you have to define what the result of
their combination is, but that seems trivial to do for additions of
fields. You either define the first protocol parameter that was added
to the spec, to add its field before the second. Or you could do it
based on something non-time-dependent, like the alphabetic order of
the protocol parameter, or the alphabetic order of the fields that
they add.

The main guarantees I'd like to uphold are listed here:
https://www.postgresql.org/message-id/CAGECzQR5PMud4q8Atyz0gOoJ1xNH33g7g-MLXFML1_Vrhbzs6Q@mail.gmail.com

  12. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-30T09:03:27Z

    On Mon, 28 Oct 2024 at 16:51, Peter Eisentraut <peter@eisentraut.org> wrote:
> Thoughts?

+                   snprintf(xloc, sizeof(xloc), "%X/%X",
LSN_FORMAT_ARGS(logptr))
+                   pq_sendstring(&buf, xloc);

nit: I feel that sending the LSN as a string seems unnecessarily
wasteful of bytes. I'd rather send it as its binary representation.

  13. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-30T09:04:41Z

    On Mon, 28 Oct 2024 at 17:58, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > - The Query message sends an LSN to wait for.  (This doesn't handle the
> > extended query protocol yet.)
>
> I'd suggest adding a new message type for this, so that it works the
> same with simple and extended query. Or if you just want to wait without
> issuing any query.

Big +1 to this. After thinking about it more, I think this would make
a fancy pooler much easier to implement. Because then the pooler could
simply send such a new WaitForLSN message whenever it wants to, e.g.
before handing off the server connection to the client. Instead of
having to intercept every Query/Execute message that the client is
sending, and modify that in place before sending it on to the server.

Writing the previous down made me realize that using a separate
message would be nice for this usecase too. As opposed to including it
in ReadyForQuery. Because if the fancy pooler wants to configure these
LSNs transparently for a client that has not set the protocol
parameter, it would need to strip the new LSN field from the
ReadyForQuery message before forwarding it to the client. Stripping
out a whole message is generally easier to do than modifying messages
in place.

  14. Re: protocol-level wait-for-LSN

    Tatsuo Ishii <ishii@postgresql.org> — 2024-10-30T11:34:55Z

    > On Wed, 30 Oct 2024 at 07:49, Tatsuo Ishii <ishii@postgresql.org> wrote:
>> > I think one would have to define that somehow.  If it's useful, the
>> > additional fields of both extensions could be appended, in some
>> > defined order.  But this is an interesting question to think about.
>>
>> I think this kind of extension, which adds new filed to an existing
>> message type, should be implemented as v4 protocol.
> 
> Could you explain why you think a major version bump is needed? In
> what situation do you care about this. Because for my usecases (client
> implementations & pgbouncer) I don't think that would be necessary. If
> a client doesn't send the _pq_.wait_for_lsn protocol parameter, it
> will never receive this new version.

Yes, if there's only one extension for a message type, it would not be
a big problem. But if there's more than one extensions that want to
change the same type, problem arises as I have already discussed them
upthread.

> I don't really see a problem with having two protocol parameters
> change the same message. Yes, you have to define what the result of
> their combination is, but that seems trivial to do for additions of
> fields. You either define the first protocol parameter that was added
> to the spec, to add its field before the second. Or you could do it
> based on something non-time-dependent, like the alphabetic order of
> the protocol parameter, or the alphabetic order of the fields that
> they add.

That sounds far from trivial. So each extension needs to check if any
other extension which modifies the same message type is activated?
That requires each extension implementation to have built-in knowledge
about any conflicting extension. Moreover each extension may not be
added at once.  If extension Y is added after extension X is defined,
then implementation of X needs to be changed because at the time when
X is defined, it did not need to care about Y. Another way to deal
with the problem could be defining a new protocol message which
describes those conflict information so that each extensions do not
need to have such information built-in, but maybe it is too complex.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

  15. Re: protocol-level wait-for-LSN

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-10-30T11:53:27Z

    On 30/10/2024 13:34, Tatsuo Ishii wrote:
>> On Wed, 30 Oct 2024 at 07:49, Tatsuo Ishii <ishii@postgresql.org> wrote:
>>>> I think one would have to define that somehow.  If it's useful, the
>>>> additional fields of both extensions could be appended, in some
>>>> defined order.  But this is an interesting question to think about.
>>>
>>> I think this kind of extension, which adds new filed to an existing
>>> message type, should be implemented as v4 protocol.
>>
>> Could you explain why you think a major version bump is needed? In
>> what situation do you care about this. Because for my usecases (client
>> implementations & pgbouncer) I don't think that would be necessary. If
>> a client doesn't send the _pq_.wait_for_lsn protocol parameter, it
>> will never receive this new version.
> 
> Yes, if there's only one extension for a message type, it would not be
> a big problem. But if there's more than one extensions that want to
> change the same type, problem arises as I have already discussed them
> upthread.
> 
>> I don't really see a problem with having two protocol parameters
>> change the same message. Yes, you have to define what the result of
>> their combination is, but that seems trivial to do for additions of
>> fields. You either define the first protocol parameter that was added
>> to the spec, to add its field before the second. Or you could do it
>> based on something non-time-dependent, like the alphabetic order of
>> the protocol parameter, or the alphabetic order of the fields that
>> they add.
> 
> That sounds far from trivial. So each extension needs to check if any
> other extension which modifies the same message type is activated?
> That requires each extension implementation to have built-in knowledge
> about any conflicting extension. Moreover each extension may not be
> added at once.  If extension Y is added after extension X is defined,
> then implementation of X needs to be changed because at the time when
> X is defined, it did not need to care about Y. Another way to deal
> with the problem could be defining a new protocol message which
> describes those conflict information so that each extensions do not
> need to have such information built-in, but maybe it is too complex.

Note that the "protocol extension" mechanism is *not* meant for 
user-defined extensions. That's not the primary purpose anyway. It 
allows evolving the protocol in core code in a backwards compatible way, 
but indeed the different extensions will need to be coordinated so that 
they don't clash with each other. If they introduced new message types 
for example, they better use different message type codes.

We might have made a mistake by calling this mechanism "protocol 
extensions", because it makes people think of user-defined extensions.

With user-defined extensions, yes, you have exactly the problem you 
describe.We have no rules on how a protocol extension is allowed to 
change the protocol. It might add fields, it might add messages, or it 
might change the meaning of existing messages. Or encapsulate the whole 
protocol in XML.

So yes, each protocol extension needs to know about all the other 
protocol extensions that it can be used with. In practice we'll avoid 
doing crazy stuff so that the protocol extensions are orthogonal, but if 
user-defined extensions get involved, there's not much we can do to 
ensure that.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  16. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-30T14:01:27Z

    On Wed, 30 Oct 2024 at 12:53, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> We might have made a mistake by calling this mechanism "protocol
> extensions", because it makes people think of user-defined extensions.

I think this is a real problem, that's probably worth fixing. I
created a separate thread to address this[1]

> So yes, each protocol extension needs to know about all the other
> protocol extensions that it can be used with. In practice we'll avoid
> doing crazy stuff so that the protocol extensions are orthogonal

Just as an example, let's say we add a server-side query time to the
protocol (which honestly seems like a pretty useful feature). So that
ReadyForQuery now returns the query time if the query_time protocol.
For clients it isn't difficult at all to support any combination of
query_time & wait_for_lsn options. As long as we define that the
wait_for_lsn field is before the query_time field if both exist, then
two simple if statements like this would do the trick:

if (wait_for_lsn_enabled) {
    // interpret next field as LSN
}
if (query_time_enabled) {
    // interpret next field as query time
}

[1]: https://www.postgresql.org/message-id/CAGECzQQoc%2BV94TrF-5cMikCMaf-uUnU52euwSCtQBeDYqXnXyA%40mail.gmail.com

  17. Re: protocol-level wait-for-LSN

    Ants Aasma <ants.aasma@cybertec.at> — 2024-10-30T17:17:47Z

    On Mon, 28 Oct 2024 at 17:51, Peter Eisentraut <peter@eisentraut.org> wrote:
> This is something I hacked together on the way back from pgconf.eu.
> It's highly experimental.
>
> The idea is to do the equivalent of pg_wal_replay_wait() on the protocol
> level, so that it is ideally fully transparent to the application code.
> The application just issues queries, and they might be serviced by a
> primary or a standby, but there is always a correct ordering of reads
> after writes.

The idea is great, I have been wanting something like this for a long
time. For future proofing it might be a good idea to not require the
communicated-waited value to be a LSN.

In a sharded database a Lamport timestamp would allow for sequential
consistency. Lamport timestamp is just some monotonically increasing
value that is eagerly shared between all communicating participants,
including clients. For a single cluster LSNs work fine for this
purpose. But with multiple shards LSNs will not work, unless arranged
as a vector clock which is what I think Matthias proposed.

Even without sharding LSN might not be a final choice. Right now on
the primary the visibility order is not LSN order. So if a connection
does synchronous_commit = off commit, the write location is not even
going to see the commit. By publishing the end of the commit record it
would be better. But I assume at some point we would like to have a
consistent visibility order, which quite likely means using something
other than LSN as the logical clock.

I see the patch names the field LSN, but on the protocol level and for
the client library this is just an opaque 127 byte token. So basically
I'm thinking the naming could be more generic. And for a complete
Lamport timestamp implementation we would need the capability of
extracting the last seen value and another set-if-greater update
operation.

-- 
Ants Aasma
www.cybertec-postgresql.com

  18. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-30T17:45:27Z

    On Wed, 30 Oct 2024 at 18:18, Ants Aasma <ants.aasma@cybertec.at> wrote:
> The idea is great, I have been wanting something like this for a long
> time. For future proofing it might be a good idea to not require the
> communicated-waited value to be a LSN.

Yours and Matthias' feedback make total sense I think. From an
implementation perspective I think there are a few things necessary to
enable these wider usecases:
1. The token should be considered opaque for clients (should be documented)
2. The token should be defined as variable length in the protocol
3. We should have a hook to allow postgres extensions to override the
default token generation
4. We should have a hook to allow postgres extensions to override
waiting until the token "timestamp"

> Even without sharding LSN might not be a final choice. Right now on
> the primary the visibility order is not LSN order. So if a connection
> does synchronous_commit = off commit, the write location is not even
> going to see the commit. By publishing the end of the commit record it
> would be better. But I assume at some point we would like to have a
> consistent visibility order, which quite likely means using something
> other than LSN as the logical clock.

I was going to say that the default could probably still be LSN, but
this makes me doubt that. Is there some other token that we can send
now that we could "wait" on instead of the LSN, which would work for.
If not, I think LSN is still probably a good choice as the default. Or
maybe only as a default in case synchronous_commit != off.

  19. Re: protocol-level wait-for-LSN

    Jesper Pedersen <jesper.pedersen@comcast.net> — 2024-10-30T18:03:48Z

    Hi,

On 10/30/24 1:45 PM, Jelte Fennema-Nio wrote:
> On Wed, 30 Oct 2024 at 18:18, Ants Aasma <ants.aasma@cybertec.at> wrote:
>> The idea is great, I have been wanting something like this for a long
>> time. For future proofing it might be a good idea to not require the
>> communicated-waited value to be a LSN.
> 
> Yours and Matthias' feedback make total sense I think. From an
> implementation perspective I think there are a few things necessary to
> enable these wider usecases:
> 1. The token should be considered opaque for clients (should be documented)
> 2. The token should be defined as variable length in the protocol
> 3. We should have a hook to allow postgres extensions to override the
> default token generation
> 4. We should have a hook to allow postgres extensions to override
> waiting until the token "timestamp"
> 
>> Even without sharding LSN might not be a final choice. Right now on
>> the primary the visibility order is not LSN order. So if a connection
>> does synchronous_commit = off commit, the write location is not even
>> going to see the commit. By publishing the end of the commit record it
>> would be better. But I assume at some point we would like to have a
>> consistent visibility order, which quite likely means using something
>> other than LSN as the logical clock.
> 
> I was going to say that the default could probably still be LSN, but
> this makes me doubt that. Is there some other token that we can send
> now that we could "wait" on instead of the LSN, which would work for.
> If not, I think LSN is still probably a good choice as the default. Or
> maybe only as a default in case synchronous_commit != off.
> 

There are known wish-lists for a protocol v4, like

  https://github.com/pgjdbc/pgjdbc/blob/master/backend_protocol_v4_wanted_features.md

and a lot of clean-room implementations in drivers and embedded in 
projects/products.

Having LSN would be nice, but to break all existing implementations, no. 
Having to specify with startup parameters how a core message format 
looks like sounds like a bad idea to me,

  https://www.postgresql.org/docs/devel/protocol-message-formats.html

is it.

If we want to start on a protocol v4 thing then that is ok - but there 
are a lot of feature requests for that one.

Best regards,
  Jesper

  20. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-30T19:49:54Z

    On Wed, 30 Oct 2024 at 19:04, Jesper Pedersen
<jesper.pedersen@comcast.net> wrote:
> Having LSN would be nice, but to break all existing implementations, no.
> Having to specify with startup parameters how a core message format
> looks like sounds like a bad idea to me,

It would really help if you would explain why you think it's a bad
idea to use a startup parameter for that, instead of simply stating
that you think it needs a major protocol version bump.

The point of enabling it through a startup parameter (aka protocol
option) is exactly so it will not break any existing implementations.
If clients request the protocol option (which as the name suggests is
optional), then they are expected to be able to parse it. If they
don't, then they will get the old message format. So no existing
implementation will be broken. If some middleware/proxy gets a request
for a startup option it does not support it can advertise that to the
client using the NegotiateProtocolVersion message. Allowing the client
to continue in a mode where the option is not enabled.

So, not bumping the major protocol version and enabling this feature
through a protocol option actually causes less breakage in practice.

Also regarding the wishlist. I think it's much more likely for any of
those to happen in a minor version bump and/or protocol option than it
is that we'll bump the major protocol version.

P.S. Like I said in another email on this thread: I think for this
specific case I'd also prefer a separate new message, because that
makes it easier to filter that message out when received by PgBouncer.
But I'd still like to understand your viewpoint better on this,
because adding fields to existing message types is definitely one of
the types of changes that I personally think would be fine for some
protocol changes.

  21. Re: protocol-level wait-for-LSN

    Tatsuo Ishii <ishii@postgresql.org> — 2024-10-30T22:44:55Z

    >> So yes, each protocol extension needs to know about all the other
>> protocol extensions that it can be used with. In practice we'll avoid
>> doing crazy stuff so that the protocol extensions are orthogonal
> 
> Just as an example, let's say we add a server-side query time to the
> protocol (which honestly seems like a pretty useful feature). So that
> ReadyForQuery now returns the query time if the query_time protocol.
> For clients it isn't difficult at all to support any combination of
> query_time & wait_for_lsn options. As long as we define that the
> wait_for_lsn field is before the query_time field if both exist, then
> two simple if statements like this would do the trick:
> 
> if (wait_for_lsn_enabled) {
>     // interpret next field as LSN
> }
> if (query_time_enabled) {
>     // interpret next field as query time
> }

But

if (query_time_enabled) {
    // interpret next field as query time
}
if (wait_for_lsn_enabled) {
    // interpret next field as LSN
}

doesn't work, right? I don't like clients need to know the exact order
of each protocol extensions.

BTW,

> Just as an example, let's say we add a server-side query time to the
> protocol (which honestly seems like a pretty useful feature). So that
> ReadyForQuery now returns the query time if the query_time protocol.

Probaby it's better CommandComplete returns the query time because
there could be multiple query-time in multi-statement query or
extended query protocol.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

  22. Re: protocol-level wait-for-LSN

    Jesper Pedersen <jesper.pedersen@comcast.net> — 2024-10-31T12:59:01Z

    Hi,

On 10/30/24 3:49 PM, Jelte Fennema-Nio wrote:
> On Wed, 30 Oct 2024 at 19:04, Jesper Pedersen
> <jesper.pedersen@comcast.net> wrote:
>> Having LSN would be nice, but to break all existing implementations, no.
>> Having to specify with startup parameters how a core message format
>> looks like sounds like a bad idea to me,
> 
> It would really help if you would explain why you think it's a bad
> idea to use a startup parameter for that, instead of simply stating
> that you think it needs a major protocol version bump.
> 
> The point of enabling it through a startup parameter (aka protocol
> option) is exactly so it will not break any existing implementations.
> If clients request the protocol option (which as the name suggests is
> optional), then they are expected to be able to parse it. If they
> don't, then they will get the old message format. So no existing
> implementation will be broken. If some middleware/proxy gets a request
> for a startup option it does not support it can advertise that to the
> client using the NegotiateProtocolVersion message. Allowing the client
> to continue in a mode where the option is not enabled.
> 
> So, not bumping the major protocol version and enabling this feature
> through a protocol option actually causes less breakage in practice.
>

Yes, but it opens up for everybody changing all message formats by 
startup parameters.

And, it will be confusing to clean-room implementations: When you have 
this startup parameter then you get these message formats, when you have 
this startup parameter then you get these message formats -- and what 
about combinations ? Like Tatsuo-san stated up-thread.

You are also assuming that all PostgreSQL protocol implementations uses 
the Length (Int32) field very strict - so when one developer adds the 
startup parameter, but doesn't change the underlying implementation 
everything will break.

The protocol format must be 100% clear and well-documented in all cases.

> Also regarding the wishlist. I think it's much more likely for any of
> those to happen in a minor version bump and/or protocol option than it
> is that we'll bump the major protocol version.
> 

I agree that protocol v4 is likely far out unless somebody want to 
coordinate the work needed.

> P.S. Like I said in another email on this thread: I think for this
> specific case I'd also prefer a separate new message, because that
> makes it easier to filter that message out when received by PgBouncer.
> But I'd still like to understand your viewpoint better on this,
> because adding fields to existing message types is definitely one of
> the types of changes that I personally think would be fine for some
> protocol changes.
> 

If this door is open then it has to very clear how multiple startup 
parameters are handled at the protocol level, and that is a much bigger 
fish because what happens if extensions add startup parameters as well.

Adding a new message could be the way forward, but that opens the door 
for the wish-lists for v4.

Best regards,
  Jesper

  23. Re: protocol-level wait-for-LSN

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-10-31T13:25:09Z

    On Thu, 31 Oct 2024 at 13:59, Jesper Pedersen
<jesper.pedersen@comcast.net> wrote:
> And, it will be confusing to clean-room implementations: When you have
> this startup parameter then you get these message formats, when you have
> this startup parameter then you get these message formats -- and what
> about combinations ? Like Tatsuo-san stated up-thread.

I really don't understand why you think that's so difficult. To be
clear, no client is forced to implement any of these protocol options.
And all of these protocol options would be documented in the official
protocol docs. For instance the ReadyForQuery docs on the "Message
Formats" page in the docs could easily be made to look like the
following, which imho would be very clear to any implementer of the
protocol about ordering of these fields:

ReadyForQuery (B)
Byte1('Z')

Identifies the message type. ReadyForQuery is sent whenever the
backend is ready for a new query cycle.

Int32

Length of message contents in bytes, including self.

Int64: Only present if protocol option wait_for_lsn is set to 1 by the client

The LSN at time of commit

Int64: Only present if protocol option query_time is set to 1 by the client

Time it took to run the query in microseconds

Byte1

Current backend transaction status indicator. Possible values are 'I'
if idle (not in a transaction block); 'T' if in a transaction block;
or 'E' if in a failed transaction block (queries will be rejected
until block is ended).

> You are also assuming that all PostgreSQL protocol implementations uses
> the Length (Int32) field very strict - so when one developer adds the
> startup parameter, but doesn't change the underlying implementation
> everything will break.

Yes... But that seems equivalent to saying: If a developer of a
Postgres client advertises that they support protocol v4, but don't
actually implement it, then everything will break.

i.e. it's the job of the client author to not send protocol options
that it doesn't know anything about. Just like it's the job of the
client author not to request versions that it does not know anything
about.

> The protocol format must be 100% clear and well-documented in all cases.

Agreed. See above.

> If this door is open then it has to very clear how multiple startup
> parameters are handled at the protocol level, and that is a much bigger
> fish because what happens if extensions add startup parameters as well.

Postgres extensions **cannot** add such startup parameters. Heikki
already mentioned that the naming was confusing in the docs. At this
point in time we're only discussing protocol changes that are coming
from Postgres core (which is already a contentious enough topic).

  24. Re: protocol-level wait-for-LSN

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-04T09:47:27Z

    On 30.10.24 10:03, Jelte Fennema-Nio wrote:
> On Mon, 28 Oct 2024 at 16:51, Peter Eisentraut <peter@eisentraut.org> wrote:
>> Thoughts?
> 
> +                   snprintf(xloc, sizeof(xloc), "%X/%X",
> LSN_FORMAT_ARGS(logptr))
> +                   pq_sendstring(&buf, xloc);
> 
> nit: I feel that sending the LSN as a string seems unnecessarily
> wasteful of bytes. I'd rather send it as its binary representation.

My thinking here was: This protocol is also used by things that are not 
PostgreSQL.  They might have other representations for "position to wait 
for".  I don't know, but it's something to think about.

  25. Re: protocol-level wait-for-LSN

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2024-11-04T10:08:03Z

    On Wed, 30 Oct 2024, 18:45 Jelte Fennema-Nio, <postgres@jeltef.nl> wrote:
>
> On Wed, 30 Oct 2024 at 18:18, Ants Aasma <ants.aasma@cybertec.at> wrote:
> > The idea is great, I have been wanting something like this for a long
> > time. For future proofing it might be a good idea to not require the
> > communicated-waited value to be a LSN.
>
> Yours and Matthias' feedback make total sense I think. From an
> implementation perspective I think there are a few things necessary to
> enable these wider usecases:
> 1. The token should be considered opaque for clients (should be documented)

I disagree. It is critical that a consumer knows what to do with the
output. Blindly passing it around is not a valid strategy: In my
example of keeping track of replication slots the client also has to
keep track of every cluster ID to make it work correctly, as every
postgres instance may only know about a subset of other PG instances:
A client would have to know how to discern and how to merge the
returned set of [cluster_id, LSN] pairs into its own view of a global
progress:

Say, you connect to cluster A, which receives changes from clusters X
and Y, cluster B, which receives from X and Z, and cluster C, which
receives from all of X, Y, and Z. Cluster B should ignore [Y_ID, Lsn],
as keeping the [cluster id, LSN] pair around would be sensitive to
resource attacks, but the client will have to merge the response from
that scluster to make sure it doesn't accidentally "go back in time"
when it switches from cluster A or B to another cluster with the "wait
for this minimal replication state" 'token'.

> > Even without sharding LSN might not be a final choice. Right now on
> > the primary the visibility order is not LSN order. So if a connection
> > does synchronous_commit = off commit, the write location is not even
> > going to see the commit. By publishing the end of the commit record it
> > would be better. But I assume at some point we would like to have a
> > consistent visibility order, which quite likely means using something
> > other than LSN as the logical clock.

Or have CSN=LSN -based snapshots on the primary, too, as that also
would solve the unordered visibility issue on the primary, as well as
the unacknowledged read issue.

> I was going to say that the default could probably still be LSN, but
> this makes me doubt that. Is there some other token that we can send
> now that we could "wait" on instead of the LSN, which would work for.
> If not, I think LSN is still probably a good choice as the default. Or
> maybe only as a default in case synchronous_commit != off.

I don't see how we can have anything but LSN as 'wait-for-this'
condition, as everything else could appear out-of-order in the WAL (we
don't allow the record to be modified during
XLogInsert()/ReserveXLogInsertLocation()), and WAL is our one source
of truth for change capture.

PS. I have other complaints about timestamp-based
replication/snapshots, but unless someone thinks otherwise and/or it
is made relevant I'll consider that off-topic.


Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

  26. Re: protocol-level wait-for-LSN

    Robert Haas <robertmhaas@gmail.com> — 2024-11-04T20:07:49Z

    On Wed, Oct 30, 2024 at 6:45 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
> doesn't work, right? I don't like clients need to know the exact order
> of each protocol extensions.

I agree with this criticism, at least for the most part. Years and
years ago, the only way to specify EXPLAIN options was to say EXPLAIN
[ANALYZE] [VERBOSE] query. So, if you said, EXPLAIN VERBOSE ANALYZE
query, it didn't work. Actually, it still doesn't, but now you can say
EXPLAIN (VERBOSE, ANALYZE) query and that will work, because the new
options syntax allows for options to be specified in any order. And a
really key point here is that for quite a while we were resistant to
adding any new EXPLAIN options precisely because everyone knew the
requirement to mention the options in a specific order did not scale.
We could reasonably ask users to remember that ANALYZE had to come
before VERBOSE, but asking people to remember the correct order of
three or six or ten options would end up being quite annoying.

And I think the problem here is the same. When you want to add the
first set of optional fields to a protocol message, it seems perfectly
reasonable to decide that _pq_.tde=1 or _pq_.wait_for_lsn=1 turns them
on. When you add the second set of fields, it probably still feels
reasonable. But when you get up to half a dozen or so protocol
extensions that affect the same underlying set of messages, it's going
to start to be pretty annoying. Parsing that protocol message is going
to require pretty complicated code. Even if you don't care about the
contents of the extra fields, you still potentially need code to
understand and ignore them, unless you refuse support for the protocol
extension altogether.

Now, what makes this case less of a problem than the EXPLAIN case
mentioned above is that people are not typically going to construct
protocol messages by hand. As long as the protocol documentation is
clear about the ordering of fields and which fields are controlled by
which options, maybe it's not too horrible if everybody has to go
through and write a bunch of if-statements. But even so, wouldn't it
be easier if protocol extensions only added new message types instead
of redefining existing ones? Then you could just ignore message types
you don't care about. To be clear, I'm not saying that we should
never, ever extend an existing message type. I'm just saying that the
design of cramming a bunch of new fields into a message type doesn't
seem entirely scalable, and therefore I believe we should consider
whether there are reasonable alternatives.

-- 
Robert Haas
EDB: http://www.enterprisedb.com