Copilot commented on code in PR #2069:
URL: https://github.com/apache/cloudberry/pull/2069#discussion_r4164625166
##########
gpcontrib/gp_stats_collector/sql/gpsc_fdw_norm.sql:
##########
@@ -0,0 +1,81 @@
+--
+-- Test that a failure to normalize plan/query text does not interrupt
+-- query execution.
+--
+-- A foreign table with an empty schema_name option makes postgres_fdw
+-- deparse Remote SQL containing a zero-length delimited identifier ("").
+-- The gpsc normalizer runs the core SQL lexer over the plan text, and the
+-- lexer error used to be escalated to the user query as
+-- "Unexpected exception in gpsc ...". Now a warning must be logged instead,
+-- and the query must continue. LIMIT 0 keeps the (invalid) Remote SQL from
+-- being sent to the loopback server.
+--
+-- start_ignore
+CREATE EXTENSION IF NOT EXISTS gp_stats_collector;
+CREATE EXTENSION IF NOT EXISTS postgres_fdw;
+SELECT gpsc.truncate_log();
+-- end_ignore
+
+CREATE OR REPLACE FUNCTION gpsc_status_order(status text)
+RETURNS integer
+AS $$
+BEGIN
+ RETURN CASE status
+ WHEN 'QUERY_STATUS_SUBMIT' THEN 1
+ WHEN 'QUERY_STATUS_START' THEN 2
+ WHEN 'QUERY_STATUS_END' THEN 3
+ WHEN 'QUERY_STATUS_DONE' THEN 4
+ ELSE 999
+ END;
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;
+
+SET gpsc.ignored_users_list TO '';
+SET gpsc.enable TO TRUE;
+SET gpsc.enable_utility TO FALSE;
+SET gpsc.logging_mode TO 'TBL';
+
+DO $$
+BEGIN
+ EXECUTE format(
+ 'CREATE SERVER gpsc_loopback FOREIGN DATA WRAPPER postgres_fdw '
+ 'OPTIONS (host %L, port %L, dbname %L)',
+ split_part(current_setting('unix_socket_directories'), ',', 1),
+ current_setting('port'),
+ current_database());
+END
+$$;
+CREATE USER MAPPING FOR CURRENT_USER SERVER gpsc_loopback;
+CREATE FOREIGN TABLE gpsc_ft (x text) SERVER gpsc_loopback
+ OPTIONS (schema_name '', table_name 'pg_class');
+
+-- Before the fix this failed with:
+-- ERROR: Unexpected exception in gpsc zero-length delimited identifier at or
near """"
+SELECT * FROM gpsc_ft LIMIT 0;
Review Comment:
`LIMIT 0` is not a reliable guard against sending this invalid Remote SQL.
`postgres_fdw` can push the limit into the Foreign Scan and remove the local
Limit (`postgres_fdw.c:7108–7218`). The first scan call then declares a remote
cursor, so the server rejects the empty schema identifier before returning any
rows. The test fails instead of producing the expected successful result and
DONE event. Keep the limit local, for example by adding `WHERE random() < 2`,
which cannot be shipped to the remote server. Update both query-text filters
and the expected output to match.
##########
gpcontrib/gp_stats_collector/src/ProtoUtils.cpp:
##########
@@ -150,6 +150,12 @@ set_query_plan(gpsc::SetQueryReq *req, QueryDesc
*query_desc,
norm_plan->len));
gpdb::pfree(norm_plan->data);
}
+ else
+ {
+ /* plan_id must be calculated even if
normalization failed */
+ qi->set_plan_id(
+ hash_any((unsigned char *)
es.str->data, es.str->len));
Review Comment:
This fallback makes `plan_id` depend on literal values and EXPLAIN cost/row
estimates, rather than the normalized plan defined in
`gpcontrib/gp_stats_collector/metric.md:123`. When normalization fails,
equivalent plan shapes can therefore receive different IDs, splitting any
grouping by `plan_id`. Leave the ID unset when normalization fails, or use an
equivalently stable fallback. If it is left unset, update the regression
assertion and expected output to expect a NULL ID; the log writer already
supports absent fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
