[DNM] Rowexpr fix collations #26
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 13 commits
September 11, 2019 11:55
What this does: - it allows for Row Expressions node push downs (T_RowExpr) in a very hacky way. Chances are that to do it properly it needs to continue with a recursive exploration of the tree from that node. This was preventing the whole aggregate function ST_AsMVt to be pushed down to the foreign server. - it provides a crappy implementation of deparseRowExpr. It makes the planner happy to ship some SQL but it is not 100% valid as column names are lost when received in the foreign end.
For default input collation, inner_cxt.state is marked as FDW_COLLATE_NONE. There's the situation in which agg.collation == DEFAULT_COLLATION_OID and thus it should be safe to push down the aggregation.
A row expression cannot be deparsed as ROW(...) cause otherwise column names are lost when sending the deparsed query to the remote. RowExpr may be present in the planner tree, as the optimizer may "pull up" aggregation subqueries and it certainly does. In particular, ST_AsMVT may produce different results or may miss the geom column if column names are generated as f1, f2, ..., fn. This solves that issue by deparsing the row expression as an alias and embedding it into a subquery. This is the intended syntax transformation in pseudo-code: SELECT row_expr FROM from_clause WHERE where_caluse => SELECT alias FROM ( SELECT row_expr_fields FROM from_clause WHERE where_clause ) alias
That is the schema where postgis (and many other extensions) usually reside. This is done in order to overcome a bug in postgis type checking that yields "ERROR: parse_column_keys: no geometry column found". That bug was recently fixed in this commit: postgis/postgis@e096795#diff-86bc92dfea8fd30b975e278bd323067f
This reverts commit 83b3ef3.
This reverts commit 125f0fd.
This works with nested row expressions. E.g: EXPLAIN (analyze,VERBOSE) select count(((t, 'hola'::text))) FROM (SELECT cartodb_id, street FROM cdb_fdw_local_pg11.sf_stclines) t; Remote SQL: SELECT count(s1) FROM (SELECT s2, 'hola'::text FROM (SELECT cartodb_id, street FROM carto_lite.sf_stclines) s2) s1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a retake of the original patch, with two major changes:
As a result, this passes regression tests and causes remote execution of ST_AsMVT, as intended.
What's missing: