Refactor PostgreSQL Connection-Level Parameters Handling by rahim-kanji · Pull Request #4899 · sysown/proxysql

rahim-kanji · 2025-04-01T20:53:55Z

Changes

Previously, ProxySQL enforced a strict subset of PostgreSQL connection parameters as per the official documentation, rejecting any undocumented parameters. However, actual PostgreSQL servers accept additional parameters (e.g., extra_float_digits) and apply them at the connection/session level. To align with PostgreSQL's behavior and improve compatibility, ProxySQL now:

Accepts all connection parameters provided by clients, not just the documented subset.
Defer parameter validation until after successful authentication to optimize resource usage.

Other changes:

Treat client_encoding as normal server parameter/variable.
Optimized RESET ALL command handling.

Information regarding the TAP test:

Using libpq to test ProxySQL's handling of undocumented parameters isn't possible, as libpq enforces a strict subset of PostgreSQL connection parameters as per the official documentation, rejecting any undocumented parameters. However, actual PostgreSQL servers accept additional parameters (e.g., extra_float_digits) and apply them at the connection/session level. To test this behavior, a raw socket is used to connect to a ProxySQL server and send custom built messages to communicate with ProxySQL. It currently works with plain text password authentication, without ssl support.
Failure due to an invalid parameter returned by the PostgreSQL server, differs from ProxySQL's behavior. PostgreSQL returns an error during the connection handshake phase, whereas in ProxySQL, the connection succeeds, but the error is encountered when executing a query. This is behaviour is intentional, as newer PostgreSQL versions may introduce new parameters that ProxySQL is not yet aware of.

Closes #4890
Closes #4898

* Accept all parameters sent by the client, mirroring PostgreSQL's permissive handling. * Validate and apply parameters only after successful authentication. This avoids wasting resources on invalid connections.

Optimized RESET ALL command handling. Few optimizations. Code cleanup

Note: - Using libpq to test ProxySQL's handling of undocumented parameters isn't possible, as libpq enforces a strict subset of PostgreSQL connection parameters as per the official documentation, rejecting any undocumented parameters. However, actual PostgreSQL servers accept additional parameters (e.g., extra_float_digits) and apply them at the connection/session level. To test this behavior, a raw socket is used to connect to a ProxySQL server and send custom built messages to communicate with ProxySQL. It currently works with plain text password authentication, without ssl support. - Failure due to an invalid parameter returned by the PostgreSQL server, differs from ProxySQL's behavior. PostgreSQL returns an error during the connection handshake phase, whereas in ProxySQL, the connection succeeds, but the error is encountered when executing a query. This is behaviour is intentional, as newer PostgreSQL versions may introduce new parameters that ProxySQL is not yet aware of.

…ving direct usage of pgsql_tracked_variables[idx].idx. This change improves consistency and reduces potential human error. * Fixed some warning

noizu

This looks like working code to me.

I would avoid the

#if FALSE

#endif

macros if these serve a purpose for debugging give them a meaningful macro name or at least include a comment in the macro'd out code for why it's macro'd out. // Debug X functionality, etc.

Maybe verify mallocs but it's highly unlikely to fail and the end user has bigger problems at hand if it does.

Consider sticking to // style comments to make commented out code during dev/debug easier. outside of doc generation comments.

Loooks good to me but I am a neophyte to this code base ^_^.

noizu · 2025-04-09T08:57:07Z

include/PgSQL_Connection.h

+
+// Generate parameter array
+constexpr const char* param_name[] = {
+#define PARAM(name, val) name,


would

#define PARAM(name, val) (name) ,

break the macro logic?

Thanks for pointing this out. In the current context, the macro:
#define PARAM(name, val) name,
is intended to extract just the name part, which is a plain c-string, not a complex expression. Since name is not an expression and there’s no operator precedence or evaluation involved, parentheses are not strictly necessary here.

noizu · 2025-04-09T08:57:41Z

include/PgSQL_Connection.h

-static const Param_Name_Validation target_session_attrs {(const char*[]){"any","read-write","read-only","primary","standby","prefer-standby",nullptr},0 };
-static const Param_Name_Validation load_balance_hosts	{(const char*[]){"disable","random",nullptr},-1};
+static const std::unordered_map<std::string_view, const Param_Name_Validation*> PgSQL_Param_Name_Str = {
+#define PARAM(name, val) {name, val},


same question would {(name), (val)}, // break the macro logic.

(check previous reply)

noizu · 2025-04-09T08:58:37Z

include/PgSQL_Connection.h

+#undef PARAM
+};
+
+#if 0


if this isn't removed later on consider a macro with a meaningful name #if DEBUGGING_X

(check comments section)

noizu · 2025-04-09T08:59:04Z

include/PgSQL_Connection.h

 #define PG_EVENT_EXCEPT  0x04
 #define PG_EVENT_TIMEOUT 0x08

+#if 0


magic number, magic exclusion.

(check comments section)

noizu · 2025-04-09T09:00:23Z

include/PgSQL_Connection.h

 };
+#endif
+
+class PgSQL_Conn_Param {


methods are straight forward but a doxygen compatible or whatever we use class doc section might be nice for other code readers. PPgSQL_Conn_Param is straight forward but it is also undocumented params now so might be worth mentioning.

Yes, noted. The class is now documented.

noizu · 2025-04-09T09:19:09Z

lib/PgSQL_Set_Stmt_Parser.cpp

 	return result;
 }

+#if 0


use a const or define for macro commented out sections like this and or add a comment on why it's disabled.

(check comments section)

noizu · 2025-04-09T09:20:32Z

lib/PgSQL_Protocol.cpp

 				if (validation != nullptr && validation->accepted_values) {
 					const char** accepted_value = validation->accepted_values;
-					while (accepted_value != nullptr) {
+					while (*accepted_value) {


noizu · 2025-04-09T09:21:41Z

test/tap/tests/pgsql-connection_parameters_test-t.cpp

@@ -0,0 +1,909 @@
+/**
+ * @file pgsql-connection_parameters_test-t-t.cpp


Should we consider an alternative to libpq if not already doing so for validation here?

We can, for normal cases, but what we're doing here is somewhat taboo, and other connectors might have the same limitations as libpq. I know the Java Connector also bypasses some rules, but I'm not sure if it's as lenient when the client does the same.

noizu · 2025-04-09T09:23:23Z

test/tap/tests/pgsql-connection_parameters_test-t.cpp

+};
+
+
+int main(int argc, char** argv) {


does the testing framework generate branch/line coverage or is it necessary to manually ensure new logic is covered?

Yes, we do have code coverage in place. However, if you're referring to test-wise code coverage (i.e., ensuring every test case covers specific branches or lines of code), then no, that's not automatically handled. We rely on manual verification to ensure that new logic is adequately covered by tests.

noizu · 2025-04-09T09:24:29Z

include/proxysql_structs.h

@@ -244,7 +244,7 @@ enum mysql_variable_name {
 };

 /* NOTE:


use // style comments to allow commenting out code during dev with out resorting to macro #if logic.

(check comments section)

rahim-kanji · 2025-04-10T07:49:33Z

Thank you for the feedback!

Regarding the use of #if 0, these blocks are currently retained for reference during development and are intended to be cleaned up in a future iteration. Since they are temporary, they haven’t been given specific macro names or explanations.

On the comment style, I appreciate the suggestion. As there's no strict in-house guideline, I typically use // for brief comments (one or two or three lines) and resort to /* */ for longer, multi-line comments. It’s largely a matter of personal preference at this stage.

rahim-kanji added 5 commits March 31, 2025 10:57

Refactored connection parameters

fbb9500

* Accept all parameters sent by the client, mirroring PostgreSQL's permissive handling. * Validate and apply parameters only after successful authentication. This avoids wasting resources on invalid connections.

Treat client_encoding as normal server parameter/variable.

db442ba

Optimized RESET ALL command handling. Few optimizations. Code cleanup

Fixed connection parameter validation

b8ffb7d

* Updated code to use pgsql_tracked_variables[idx] consistently, remo…

c2f2ae5

…ving direct usage of pgsql_tracked_variables[idx].idx. This change improves consistency and reduces potential human error. * Fixed some warning

This was linked to issues Apr 1, 2025

Refactor PostgreSQL Connection-Level Parameters Handling #4890

Closed

Remove Special Handling for client_encoding #4898

Closed

renecannao requested a review from noizu April 9, 2025 08:47

noizu reviewed Apr 9, 2025

View reviewed changes

rahim-kanji added 2 commits April 10, 2025 11:57

Added comment

8a5dfc2

Code cleanup

982be8b

rahim-kanji force-pushed the v3.0_refactor_connection_info_param branch from 958cf7d to 982be8b Compare April 10, 2025 07:00

renecannao merged commit 6df3d6b into v3.0 Apr 10, 2025
30 of 152 checks passed

renecannao deleted the v3.0_refactor_connection_info_param branch March 7, 2026 20:31

		@@ -0,0 +1,909 @@
		/**
		* @file pgsql-connection_parameters_test-t-t.cpp

Conversation

rahim-kanji commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

noizu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahim-kanji commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rahim-kanji commented Apr 1, 2025 •

edited

Loading

rahim-kanji commented Apr 10, 2025 •

edited

Loading