On Thu, 14 Mar 2024 at 16:45, Robert Haas <robertmh...@gmail.com> wrote: > I feel bad arguing about the patches that you think are a slam-dunk, > but I find myself disagreeing with the design choices.
No worries, thanks a lot for responding. I'm happy to discuss this design further. I didn't necessarily mean these patches were a slam-dunk. I mainly meant that these first few patches were not specific to any protocol change, but are changes we should agree on before any change to the protocol is possible at all. Based on your response, we currently disagree on a bunch of core things. I'll first try to summarize my view on (future) protocol changes and why I think the current core design in this patchset is the correct path forward, and then go into some details inline in your response below: In my view there can be, **by definition,** only two general types of protocol changes: 1. A "simple protocol change": This is one that requires agreement by both the client and server, that they understand the new message types involved in this change. e.g. the ParameterSet message proposal (this message type is either supported or it's not) 2. A "parameterized protocol change": This requires the same as 1, but should also allow some parameterization from the client, e.g. for the compression proposal, the client should specify what compression algorithm the server should use to compress data when sending it to the client. Client and Server can agree that a "simple protocol change" is supported by both advertising a minimum minor protocol version. And for a "parameterized protocol change" the client can send a _pq_ parameter in the startup packet. So, new _pq_ parameters should only ever be introduced for parameterized protocol changes. They are not meant to advertise support, they are meant to configure protocol features. For a non-configurable protocol feature, we'd simply bump the protocol version. And since _pq_ parameters thus always control some setting at the session level, we can simply store it as a GUC, because that's how we store all our parameters at a session level. > 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 think given the automatic downgrade supported by the NegotiateProtocolVersion, there's no real down-side to requesting the newest version by default. The only downside I can see is when connecting to other applications (i.e. non PostgreSQL servers) that implement the postgres protocol, but don't implement NegotiateProtocolVersion. But for that I added the max_protocol_version connection option in 0006 (of my latest patchset). > I'm really unhappy with 0002-0004. Just to be clear, (afaict) your argument below seems to only really be about 0004, not about 0002 or 0003. Was there anything about 0002 & 0003 that you were unhappy with? 0002 & 0003 are not dependent an 0004 imho. Because even when not making _pq_ parameters map to GUCs, we'd still need to change libpq to not instantly close the connection whenever a _pq_ parameter (or new protocol version) is not supported by the server (which is what 0002 & 0003 do). > 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. Okay, our interpretation is very different here. From my perspective introducing a non-GUC namespace is NOT AT ALL the benefit of the _pq_ prefix. The main benefit (imho) is that it allows sending an "optional" parameter (i.e GUC) in the startup packet. So, one where if the server doesn't recognize it the connection attempt still succeeds. If you specify a normal GUC in the connection parameters and the server doesn't know about it, the server will close the connection. So, to be able to send a GUC that depends on the protocol and/or server version in an optional way, you'd need to wait for an extra network roundtrip until the server tells you what protocol and/or server version they are. > 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. Since I believe a _pq_ parameter should only be used to control settings at the session level. I don't think it would be short-sighted to give-up on the possibility to store them as anything else as GUCs. Because in the many years that we've had GUCs, we've been able to store all session settings using that infrastructure. BUT PLEASE NOTE: I don't think we are giving up on the thing you're describing (see explanation in final part of this email) > 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 I very much think that's a problem. This would mean an extra roundtrip at connection startup. Which, as I described above, is to me the whole benefit of the _pq_ namespace. > ... 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. This seems to be the core of your argument. But honestly, I don't understand this logic at all. Why do you think that assigning _pq_ parameters to GUCs **for the ones that match an existing GUC** would have such a far reaching effect into the future. There's only a handful of _pq_ parameters being proposed on the mailinglist at the moment. Even if we implement all of those as GUCs, and in the future, we'd want a _pq_ parameter that does not map to GUC (which I personally doubt will ever be the case). Then we can simply change the server code like so and do something special for that parameter: } + else if (strcmp(nameptr, "_pq_.some_non_guc_parameter") == 0) + { + // Do something with the parameter + } else if (strncmp(nameptr, "_pq_.", 5) == 0 && !find_option(nameptr, false, true, ERROR)) { /* * We report unkown protocol extensions using the * NegotiateProtocolVersion message instead of erroring */ This would be completely backwards compatible afaict, because _pq_.some_non_guc_parameter would not have been a GUC before. So the only thing that would have happened if you sent it, is that you would get back that the server doesn't support it in the NegotiateProtocolVersion packet (just like what is happening currently since ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed). SO TO BE VERY CLEAR: (afaict) interpreting _pq_ parameters as GUCs right now does not limit our ability to do something differently for new _pq_ parameters that we introduce in the future.