You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by "leborchuk (via GitHub)" <gi...@apache.org> on 2026/10/02 09:40:35 UTC
[PR] Guard generate template_text in gpsc [cloudberry]
leborchuk opened a new pull request, #2069:
URL: https://github.com/apache/cloudberry/pull/2069
Fix template generation here - queries on a foreign table failed with "ERROR: Unexpected exception in gpsc zero-length delimited identifier at or near """". The gpsc extension hooks ExecutorStart and normalizes the EXPLAIN VERBOSE plan text by running the core SQL lexer over it (gen_normplan). For a Foreign Scan the verbose plan embeds the FDW's Remote SQL, which contained an empty quoted identifier "" — a hard lexer error. gpsc converted this internal error into an ERROR that aborted the user's otherwise valid query. Adding the user to gpsc.ignored_users_list only bypassed gpsc processing.
The PG error was already caught by PG_TRY/PG_CATCH inside wrap();
We re-throw it as std::runtime_error only to catch it again in cpp_call and then fail query with ERROR.
A stats collector must never interrupt user queries.
We could check for exceptions, but it's useless - why we need to re-throw exception, what could we do on a caller side? Now wrappers just log a WARNING and return NULL instead
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Guard generate template_text in gpsc [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk commented on PR #2069:
URL: https://github.com/apache/cloudberry/pull/2069#issuecomment-5951701877
Fix nod needed at all - issue already fixed
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Guard generate template_text in gpsc [cloudberry]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Guard generate template_text in gpsc [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk closed pull request #2069: Guard generate template_text in gpsc
URL: https://github.com/apache/cloudberry/pull/2069
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org