On Fri, Mar 8, 2024 at 6:47 AM Jelte Fennema-Nio <m...@jeltef.nl> wrote: > 1. 0001-0005 are needed for any protocol change, and hopefully > shouldn't require much discussion
I feel bad arguing about the patches that you think are a slam-dunk, but I find myself disagreeing with the design choices. Regarding 0001, I considered making this change as part of ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed but decided against it, because it seemed like it was making the assumption that we always wanted to initiate new connections with the latest protocol version that we know how to accept, and I wasn't sure that would always be true. I don't think it would be catastrophic if this got committed or anything -- it could always be changed later if the need arose -- but I wanted to mention that I had a reason for not doing it, and I'm still not particularly convinced that we should do it. I'm really unhappy with 0002-0004. Before ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed, any parameter included in the startup message that wasn't in a short, hard-coded list was treated as a request to set a GUC. That left no room for any other type of protocol modification, so that commit carved out an exception, where when we something that starts with _pq_., it's assumed to be setting some other kind of thing, not a GUC. But 0004 throws that out the window and decides, nope, those are just GUCs, too. Even if we don't have a specific reason today why we'd ever want such a thing, it seems short-sighted to give up on the possibility that in the future we will. Because if we implement what this patch wants to do in this way, basically consuming the entire namespace that ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed created in on shot, and then later we want the sort of thing that I'm postulating, we'll have to manufacture another new namespace for that need. And it seems to me that there are other ways we could do this. For example, suppose we introduced just one new protocol parameter; for the sake of argument, I'll call it _pq_.protocol_set. If the client sends this parameter and the server accepts it, then the client knows that the server supports a new protocol message ProtocolSetParameter, which is the only way to set GUCs in the new PROTOCOL_EXTENSION category. New libpq functions with names like, I don't know, PQsetProtocolParameter(), are added to send such messages, and they return an error if there are network problems or whatever, or if the server didn't accept the _pq_.protocol_set option at startup time. With this kind of design, you're just consuming one element of the _pq_ namespace, and the next person who wants to do something can consume one more element, and we'll be able to go on for a very long time without running out of room. This is really how I intended this mechanism to be used, and the only real downside I see as compared to what you've done is that you can't set the protocol GUCs in the startup packet, but must set them afterward via separate messages. If that's a problem, then the proposal I just outline needs modification ... but no matter what we do exactly, I don't want the very first protocol extension we ever add to eat up all of _pq_. I intended that to support decades worth of extensibility, not just one patch. -- Robert Haas EDB: http://www.enterprisedb.com
