From a15bef50a7b8dcd98cf2d4a3b41350162703ce7b Mon Sep 17 00:00:00 2001 From: reshke Date: Fri, 20 Dec 2024 05:33:49 +0000 Subject: [PATCH 1/2] Return correct logic to proxy routing --- router/qrouter/proxy_routing.go | 37 ++++++++++++++++------------ router/qrouter/proxy_routing_test.go | 13 ++++++++++ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/router/qrouter/proxy_routing.go b/router/qrouter/proxy_routing.go index 09d95bf8a..61eddd80e 100644 --- a/router/qrouter/proxy_routing.go +++ b/router/qrouter/proxy_routing.go @@ -944,6 +944,9 @@ func (qr *ProxyQrouter) routeWithRules(ctx context.Context, stmt lyx.Node, sph s return nil, err } case *lyx.Select: + + var err error + /* We cannot route SQL statements without a FROM clause. However, there are a few cases to consider. */ if len(node.FromClause) == 0 && (node.LArg == nil || node.RArg == nil) { for _, expr := range node.TargetList { @@ -964,24 +967,22 @@ func (qr *ProxyQrouter) routeWithRules(ctx context.Context, stmt lyx.Node, sph s } } } - } else { - /* Deparse populates FromClause info, so do recurse into both branches if possible */ - if node.LArg != nil { - if err := qr.deparseShardingMapping(ctx, node.LArg, meta); err != nil { - return nil, err - } - } - if node.RArg != nil { - if err := qr.deparseShardingMapping(ctx, node.RArg, meta); err != nil { - return nil, err - } + } else if node.LArg != nil && node.RArg != nil { + /* deparse populates FromClause info, + * so do recurse into both branches even if error encountered + */ + l_err := qr.deparseShardingMapping(ctx, node.LArg, meta) + r_err := qr.deparseShardingMapping(ctx, node.RArg, meta) + if l_err != nil { + err = l_err } - if node.LArg == nil && node.RArg == nil { - /* SELECT stmts, which would be routed with their WHERE clause */ - if err := qr.deparseShardingMapping(ctx, stmt, meta); err != nil { - return nil, err - } + if r_err != nil { + err = r_err } + } else { + // SELECT stmts, which + // would be routed with their WHERE clause + err = qr.deparseShardingMapping(ctx, stmt, meta) } /* @@ -1015,6 +1016,10 @@ func (qr *ProxyQrouter) routeWithRules(ctx context.Context, stmt lyx.Node, sph s return routingstate.RandomMatchState{}, nil } + if err != nil { + return nil, err + } + case *lyx.Delete, *lyx.Update: // UPDATE and/or DELETE, COPY stmts, which // would be routed with their WHERE clause diff --git a/router/qrouter/proxy_routing_test.go b/router/qrouter/proxy_routing_test.go index cace0229b..ef488a0ab 100644 --- a/router/qrouter/proxy_routing_test.go +++ b/router/qrouter/proxy_routing_test.go @@ -1678,6 +1678,19 @@ func TestRouteWithRules_Select(t *testing.T) { }, err: nil, }, + + { + query: ` + SELECT c.relname, NULL::pg_catalog.text FROM pg_catalog.pg_class c WHERE c.relkind IN ('r', 'p') AND (c.relname) LIKE 'x%' AND pg_catalog.pg_table_is_visible(c.oid) AND c.relnamespace <> (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'pg_catalog') +UNION ALL +SELECT NULL::pg_catalog.text, n.nspname FROM pg_catalog.pg_namespace n WHERE n.nspname LIKE 'x%' AND n.nspname NOT LIKE E'pg\\_%' +LIMIT 1000 +`, + distribution: distribution.ID, + exp: routingstate.RandomMatchState{}, + err: nil, + }, + // TODO rewrite routeByClause to support this // { // query: "SELECT * FROM users WHERE '5f57cd31-806f-4789-a6fa-1d959ec4c64a' = id;", From c0fd7a3236ce62fb444ae4a3a838c9290103e589 Mon Sep 17 00:00:00 2001 From: denchick Date: Fri, 20 Dec 2024 10:33:35 +0100 Subject: [PATCH 2/2] potugi --- router/qrouter/proxy_routing.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/router/qrouter/proxy_routing.go b/router/qrouter/proxy_routing.go index 61eddd80e..b29f11162 100644 --- a/router/qrouter/proxy_routing.go +++ b/router/qrouter/proxy_routing.go @@ -945,7 +945,7 @@ func (qr *ProxyQrouter) routeWithRules(ctx context.Context, stmt lyx.Node, sph s } case *lyx.Select: - var err error + var deparseError error /* We cannot route SQL statements without a FROM clause. However, there are a few cases to consider. */ if len(node.FromClause) == 0 && (node.LArg == nil || node.RArg == nil) { @@ -968,21 +968,18 @@ func (qr *ProxyQrouter) routeWithRules(ctx context.Context, stmt lyx.Node, sph s } } } else if node.LArg != nil && node.RArg != nil { - /* deparse populates FromClause info, - * so do recurse into both branches even if error encountered + /* deparse populates the FromClause info, + * so it do recurse into both branches, even if an error is encountered */ - l_err := qr.deparseShardingMapping(ctx, node.LArg, meta) - r_err := qr.deparseShardingMapping(ctx, node.RArg, meta) - if l_err != nil { - err = l_err + if err := qr.deparseShardingMapping(ctx, node.LArg, meta); err != nil { + deparseError = err } - if r_err != nil { - err = r_err + if err := qr.deparseShardingMapping(ctx, node.RArg, meta); err != nil { + deparseError = err } } else { - // SELECT stmts, which - // would be routed with their WHERE clause - err = qr.deparseShardingMapping(ctx, stmt, meta) + /* SELECT stmts, which would be routed with their WHERE clause */ + deparseError = qr.deparseShardingMapping(ctx, stmt, meta) } /* @@ -1016,8 +1013,8 @@ func (qr *ProxyQrouter) routeWithRules(ctx context.Context, stmt lyx.Node, sph s return routingstate.RandomMatchState{}, nil } - if err != nil { - return nil, err + if deparseError != nil { + return nil, deparseError } case *lyx.Delete, *lyx.Update: