fix(3088): disable pgsql endpoint by default #3094

lsytj0413 · 2025-04-03T02:30:36Z

when configuration it in config.toml, a log was printed：

Using the deprecated config option 'admin.query-enging.pgsql-bind-address'.

AhmedSoliman · 2025-04-03T07:57:22Z

What I suggest is to add an option to disable this port and we set it as disabled by default. I'm not sure if we'll go all in on removing/deprecating it before we are sure that it's not valuable to the community.

lsytj0413 · 2025-04-03T08:06:15Z

What I suggest is to add an option to disable this port and we set it as disabled by default. I'm not sure if we'll go all in on removing/deprecating it before we are sure that it's not valuable to the community.

Is there any functionality that PostgreSQL queries can achieve but DataFusion queries cannot?

AhmedSoliman · 2025-04-03T08:41:48Z

Not really. It's mainly a convenience to use any postgres client to query instead restate CLI or the admin http endpoint.

lsytj0413 · 2025-04-03T08:45:44Z

Not really. It's mainly a convenience to use any postgres client to query instead restate CLI or the admin http endpoint.

Ok， @tillrohrmann says there is an discussion about how to implement this, please let me known if the solution is finalized.

tillrohrmann

Sorry for getting back to you so late @lsytj0413. The 1.3.0 release took more of my capacity than I hoped. I've given your PR a review and I think it does not fully allow to disable the psql endpoint yet. I think we need to make

restate/crates/worker/src/lib.rs

Line 223 in 84e6e0f

TaskCenter::spawn_child(

conditional on whether a bind address was configured or not.

What's a pity is that I didn't manage to give you feedback quickly enough to include it in the 1.3.0 release. Changing the default behavior in a bug fix release is not great. Maybe we could argue that this was an oversight and it should have been shipped with 1.3.0?

tillrohrmann · 2025-04-10T13:06:36Z

crates/admin/src/cluster_controller/protobuf.rs

@@ -8,6 +8,7 @@
 // the Business Source License, use of this software will be governed
 // by the Apache License, Version 2.0.

+#[cfg(feature = "clients")]


This might no longer be necessary.

tillrohrmann · 2025-04-10T13:13:05Z

crates/storage-query-postgres/src/service.rs

@@ -42,7 +42,8 @@ pub struct PostgresQueryService {
 impl PostgresQueryService {
    pub fn from_options(options: &QueryEngineOptions, query_context: QueryContext) -> Self {
        Self {
-            bind_address: options.pgsql_bind_address,
+            #[allow(deprecated)]
+            bind_address: options.pgsql_bind_address.expect("is set"),


Does this mean there is no way to disable the pgsql endpoint?

tillrohrmann · 2025-04-10T13:13:49Z

crates/storage-query-postgres/src/pgwire_server.rs

@@ -152,6 +153,8 @@ pub fn spawn_connection(
    incoming_socket: TcpStream,
    addr: SocketAddr,
 ) {
+    warn!("New connection from {addr} to Postgres query server, this is deprecated");


I think we are already printing a deprecation warning in line 118. So this one can be removed.

tillrohrmann · 2025-04-10T13:14:58Z

crates/types/src/config/admin.rs

+            query_engine: QueryEngineOptions {
+                memory_size: value.query_engine.memory_size,
+                tmp_dir: value.query_engine.tmp_dir,
+                query_parallelism: value.query_engine.query_parallelism,
+                pgsql_bind_address: Some(
+                    value
+                        .query_engine
+                        .pgsql_bind_address
+                        .unwrap_or("0.0.0.0:9071".parse().unwrap()),
+                ),
+            },


I think this will always set the pgsql_bind_address to Some and thereby start the postgres endpoint.

tillrohrmann · 2025-04-10T13:16:52Z

crates/types/src/config/admin.rs

+        #[allow(deprecated)]
+        if value.query_engine.pgsql_bind_address.is_some() {
+            print_warning_deprecated_config_option("admin.query-enging.pgsql-bind-address", None);
+        }


If you introduce a QueryEngineOptionsShadow, then it could go there.

It would be great to point users towards "You can make SQL queries using the CLI or with the :9070/query API."

lsytj0413 · 2025-04-11T01:55:52Z

Sorry for getting back to you so late @lsytj0413. The 1.3.0 release took more of my capacity than I hoped. I've given your PR a review and I think it does not fully allow to disable the psql endpoint yet. I think we need to make

restate/crates/worker/src/lib.rs

Line 223 in 84e6e0f

TaskCenter::spawn_child(

conditional on whether a bind address was configured or not.
What's a pity is that I didn't manage to give you feedback quickly enough to include it in the 1.3.0 release. Changing the default behavior in a bug fix release is not great. Maybe we could argue that this was an oversight and it should have been shipped with 1.3.0?

In this implementation, I only print a warning log when the user explicitly sets this parameter, and use the default configuration when the user doesn't set it. This indeed doesn't disable the function. I think we have the following ways to disable this function:

keep the system's behavior consistent with the previous version. Only output a warning log to prompt the user when they use this feature, and officially discontinue this feature in the next version (after fully notifying the users).
modify the system behavior to disable the function when the user does not explicitly enable it. If the user explicitly enables it, then enable the function and output a warning log for prompting.

This implementation adopts the first solution (because it will keep the behavior consistent when the user upgrades the system). May I ask which solution do you think is more appropriate? Or are there any other better solutions?

tillrohrmann · 2025-04-11T07:48:04Z

I think you are right that we shouldn't change the system's behavior in a bug fix release. Technically we are printing a deprecation warning if the user opens a connection with the pgsql client so to some extent we have this covered. Then I would lean towards disabling the pgsql endpoint by default with the next minor release (1.4.0). This would then also mean that we need to wait with merging this PR until we no longer want to release bug fix releases for 1.3.

lsytj0413 · 2025-04-11T09:01:12Z

I think you are right that we shouldn't change the system's behavior in a bug fix release. Technically we are printing a deprecation warning if the user opens a connection with the pgsql client so to some extent we have this covered. Then I would lean towards disabling the pgsql endpoint by default with the next minor release (1.4.0). This would then also mean that we need to wait with merging this PR until we no longer want to release bug fix releases for 1.3.

I'd like to confirm what to do next (Please forgive me if this is too verbose. My English is not yet very proficient.)

v1.3.x：we only printing a deprecation warning when server is starting and the user opens a connection with the pgsql client
v1.4.0：disable pgsql endpoint by defaults, and the user can explicitly enable it
v1.5.0：remove pgsql endpoint support

Is my understanding correct?

tillrohrmann · 2025-04-11T09:53:02Z

v1.3.x：we only printing a deprecation warning when server is starting and the user opens a connection with the pgsql client

v1.4.0：disable pgsql endpoint by defaults, and the user can explicitly enable it

v1.5.0：remove pgsql endpoint support

Is my understanding correct?

Yes, that sounds like a good plan.

lsytj0413 · 2025-04-12T04:43:19Z

@tillrohrmann I have change the implementation, it will print an warning when server is starting, ptal

tillrohrmann · 2025-04-14T08:43:29Z

Thanks for updating the PR @lsytj0413. Since we are already printing a deprecation warning when connecting to the pqsql endpoint, I think we are actually covered and don't need to add more. I think that my previous statements were a bit confusing in this regard. Sorry for that. The next thing we should do is to disable the pgsql endpoint for the 1.4.0 release.

lsytj0413 · 2025-04-14T12:13:32Z

Thanks for updating the PR @lsytj0413. Since we are already printing a deprecation warning when connecting to the pqsql endpoint, I think we are actually covered and don't need to add more. I think that my previous statements were a bit confusing in this regard. Sorry for that. The next thing we should do is to disable the pgsql endpoint for the 1.4.0 release.

I have change this implementation to step 2 - disable pgsql endpoint by defaults, ptal

tillrohrmann

Thanks a lot for disabling the Pgsql endpoint by default @lsytj0413. The changes look good to me. Merging this PR once we know that we no longer need to create 1.3.x bug fix releases.

lsytj0413 force-pushed the fix-3088 branch 2 times, most recently from bb2a631 to 88f355f Compare April 3, 2025 02:32

tillrohrmann requested changes Apr 10, 2025

View reviewed changes

lsytj0413 force-pushed the fix-3088 branch 2 times, most recently from bef7eef to 2108c74 Compare April 11, 2025 09:16

lsytj0413 force-pushed the fix-3088 branch 3 times, most recently from aae306f to 754ec5f Compare April 14, 2025 11:15

lsytj0413 changed the title ~~fix(3088): mark pgsql endpoint as deprecated~~ fix(3088): disable pgsql endpoint by default Apr 14, 2025

fix(3088): disable pgsql endpoint by default

487a455

lsytj0413 force-pushed the fix-3088 branch from 754ec5f to 487a455 Compare April 14, 2025 11:42

tillrohrmann approved these changes Apr 15, 2025

View reviewed changes

tillrohrmann added the release-blocker Blocker for the next release label Apr 15, 2025

fix(3088): disable pgsql endpoint by default #3094

Are you sure you want to change the base?

fix(3088): disable pgsql endpoint by default #3094

Uh oh!

Conversation

lsytj0413 commented Apr 3, 2025

Uh oh!

AhmedSoliman commented Apr 3, 2025

Uh oh!

lsytj0413 commented Apr 3, 2025

Uh oh!

AhmedSoliman commented Apr 3, 2025

Uh oh!

lsytj0413 commented Apr 3, 2025

Uh oh!

tillrohrmann left a comment

Choose a reason for hiding this comment

Uh oh!

tillrohrmann Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

tillrohrmann Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

tillrohrmann Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

tillrohrmann Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

tillrohrmann Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

tillrohrmann Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

lsytj0413 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tillrohrmann commented Apr 11, 2025

Uh oh!

lsytj0413 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tillrohrmann commented Apr 11, 2025

Uh oh!

lsytj0413 commented Apr 12, 2025

Uh oh!

tillrohrmann commented Apr 14, 2025

Uh oh!

lsytj0413 commented Apr 14, 2025

Uh oh!

tillrohrmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsytj0413 commented Apr 11, 2025 •

edited

Loading

lsytj0413 commented Apr 11, 2025 •

edited

Loading