Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing #6391

naisila · 2022-10-03T10:49:24Z

DESCRIPTION: Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing

The standard planner converts some ::text casts to ::cstring and here we convert back because cstring is a pseudotype and it cannot be casted to most types. This problem occurs in CoerceViaIO nodes.
There was another problem with RelabelType nodes fixed in the following PR:
#4580
We undo the changes in that PR, and fix both CoerceViaIO and RelabelType nodes in the planning phase (not in the deparsing phase in ruleutils)

Note to reviewer: I added the test in prepared statements given that this bug was reported twice by users, and both their use-cases involved a prepared statement. Please note that this bug actually has nothing to do with a prepared statement. Feel free to suggest another place where we could put these tests.

For reference, this is a similar PR for a different issue related to parsing #4580
Thanks @onurctirtir for linking that PR in #5646 (comment)

Fixes #5646
Fixes #6061
Fixes #5033

onderkalaci · 2022-10-05T09:28:29Z

src/backend/distributed/deparser/ruleutils_13.c

@@ -5290,6 +5292,27 @@ get_rule_expr(Node *node, deparse_context *context,
 				CoerceViaIO *iocoerce = (CoerceViaIO *) node;
 				Node	   *arg = (Node *) iocoerce->arg;

+				/*


Why did we end-up doing it on the deparser? I though you were trying to fix this in the planner. There are some disadvantages of doing it in the deparser, here are my thoughts:

It is too late :) This is not a deparsing problem, so seems like should be fixed where the problem arise? (e.g., in the planner)

Relatively hard to follow, because we don't do these types of modifications during the deparse. Deparse is (should be) a read-only operation.

We do not deparse queries that go through standard_planner. This can be confusing addition, the reader of this code can think that we always deparse queries that passed through the standard_planner. There are obvious cases (like complex subqueries) that cannot be parsed back after standard_planner. We only need this because we use some expressions in re-written query, not the whole query.

Any change in deparser is a potential conflict with future Postgres versions. I'd rather avoid if there is another way to solve it. I think here we have?

So, overall, I'd be more inclined to see this (or any other change) on the SqlTaskList function instead of here.

I'd rather avoid if there is another way to solve it. I think here we have?

It gets more complicated to figure out where this problematic node is in more complicated queries. The deparsing logic is already visiting all nodes recursively so it does that for us. For example, the following also fails in Citus:
SELECT cast('2008-04-07 00:00:00'::varchar as timestamp(6) without time zone) AS dummy FROM any_dist_table;
Not sure whether the list is correct but to give an idea the node could be in
query->jointree->quals
query->targetList
query->returningList
query->jointree->fromlist
...possibly more lists
And for each of these lists we call get_rule_expr recursively to get to the CoerceViaIO node.

Relatively hard to follow, because we don't do these types of modifications during the deparse. Deparse is (should be) a read-only operation.

As I linked here, we already have a very similar addition in this get_rule_expr function in Relabel types. Deparse is already not a read-only operation as I stated before, such changes for Citus are very easily done in get_rule_expr.

Do you still think we should do this differently?

query->jointree->quals
query->targetList
query->returningList
query->jointree->fromlist
...possibly more lists

All is part of "query" so we should be doing a query_tree_mutator, not individual conversions. query_tree_mutator traverses the queryTree, see ResolveExternalParams as an example.

As I linked here, we already have a very similar addition in this get_rule_expr function in Relabel types. Deparse is already not a read-only operation as I stated before, such changes for Citus are very easily done in get_rule_expr.

That sounds like not the ideal thing we do. I would NOT expect to call a deparser and let it change the input. We can decide to follow not-good practice here or try to make it more intuitive.

Do you still think we should do this differently?

Yes, I do. We should only do this if we had to. Now, I'm even in favor of moving the Relabel type related conversion to the new query_tree_mutator we plan to write.

I think the follow up question is this: Do you think is it a good idea to do in the parser? I can see that this is easier, but I'm not very comfortable with such a change. I'd at least want to see a sketch of doing query_tree_mutator on the SQLTaskList and see how it looks.

If that turns out to be too complex, we might consider getting back to this solution.

Does that work?

query_tree_mutator traverses the queryTree, see ResolveExternalParams as an example.

I was not aware of this, that simplifies things a lot :)

Do you think is it a good idea to do in the parser?

If query_tree_mutator works as I am imagining right now it would work, I am all in favor of following your suggestion. The main reason I chose the deparser was that it was doing the traversing work for me.

Now, I'm even in favor of moving the Relabel type related conversion to the new query_tree_mutator we plan to write.

If all turns out beautifully, +1 on that.

If that turns out to be too complex, we might consider getting back to this solution. Does that work?

Yes, thanks for your suggestion. I will work on that and update the PR accordingly.

naisila · 2022-10-10T13:43:46Z

@onderkalaci

Now, I'm even in favor of moving the Relabel type related conversion to the new query_tree_mutator we plan to write.

I wrote a query tree mutator for modifying both CoerceIO and RelabelType problematic nodes.
For the RelabelType one, I added an extra condition that the collation shouldn't be the default one, because then we are adding extra unneeded "Collate Default" parts.

onderkalaci · 2022-10-18T09:36:53Z

src/backend/distributed/planner/multi_physical_planner.c

+ * Details will be written in comments in the corresponding if conditions.
+ */
+static Node *
+ModifyProblematicNodes(Node *inputNode)


problematicNode sounds too generic. What about AdjustImproperNodesForDeparse or such?

onderkalaci · 2022-10-18T09:39:05Z

src/test/regress/expected/multi_prepare_sql.out

+
+(1 row)
+
+PREPARE test_statement(text) AS


I'd put this test to arbitrary configs tests, which should cover a lot more tests. Can we move these to https://github.com/citusdata/citus/blob/main/src/test/regress/sql/prepared_statements_4.sql or such?

Please remember that you should create the tables in prepared_statements_create_load to use in prepared_statements_4``

onderkalaci · 2022-10-18T09:53:43Z

src/test/regress/expected/multi_prepare_sql.out

+(1 row)
+
+PREPARE test_statement(text) AS
+SELECT user_id FROM test WHERE t >= $1::timestamptz ORDER BY user_id;


Lets expand for different planners:

PREPARE test_statement_regular(text) AS SELECT user_id FROM test WHERE t >= $1::timestamptz ORDER BY user_id; PREPARE test_statement_router(int, text) AS SELECT user_id FROM test WHERE user_id = $1 AND t >= $2::timestamptz ORDER BY user_id; PREPARE test_statement_repartition(int, text) AS SELECT count(*) FROM test t1 JOIN test t2 USING (t) WHERE t1.user_id = $1 AND t >= $2::timestamptz; PREPARE test_statement_cte(text, text) AS with cte_1 as (SELECT user_id, t FROM test WHERE t >= $1::timestamptz ORDER BY user_id LIMIT 1) SELECT * FROM cte_1 WHERE t <=$2::timestamp ; PREPARE test_statement_insert(int, text) AS INSERT INTO test VALUES ($2::timestamptz, $1);

Good idea, thanks for providing the prepared statements.

onderkalaci · 2022-10-18T09:53:50Z

src/test/regress/expected/multi_prepare_sql.out

+INSERT INTO test VALUES ('2022-02-02', 0);
+INSERT INTO test VALUES ('2022-01-01', 1);
+INSERT INTO test VALUES ('2021-01-01', 2);
+EXECUTE test_statement('2022-01-01');


Executing prepared statements >=6 times is always useful (or even required) as Postgres does certain changes on the query after that

onderkalaci · 2022-10-18T09:56:17Z

src/backend/distributed/planner/multi_physical_planner.c

+			cstringToText->consttype = TEXTOID;
+			cstringToText->constlen = -1;
+
+			Type textType = typeidType(TEXTOID);


what about NULL values? PG seems to do something different:

/* * We assume here that UNKNOWN's internal representation is the same * as CSTRING. */ if (!con->constisnull) newcon->constvalue = stringTypeDatum(baseType, DatumGetCString(con->constvalue), inputTypeMod); else newcon->constvalue = stringTypeDatum(baseType, NULL, inputTypeMod);

I think some of our tests should trigger NULL values (also good to check for relable types for NULL values? Would we crash?

Or, maybe should we use makeConst instead of stringTypeDatum?

I think some of our tests should trigger NULL values

I am not able to reproduce a case where we reach this part and we have a NULL value. If there is a comparison condition it is detected early in the code and executed locally (returning 0 rows), or it's not converted to coerceio node: it appears as NULL:timestamp directly from the standard planner (not NULL::cstring::timestamp)

Example:

PREPARE test_statement_regular(text) AS SELECT user_id , $1::timestamptz FROM test ORDER BY user_id; -- regular not null value EXECUTE test_statement_regular('2022-01-01'); NOTICE: we are in coerceviaio node NOTICE: constvalue is 2022-01-01 NOTICE: issuing SELECT user_id, ('2022-01-01'::text)::timestamp with time zone AS timestamptz FROM public.test_102008 test WHERE true DETAIL: on server postgres@localhost:9701 connectionId: 1 -- null value: we have no coerceviaio nodes EXECUTE test_statement_regular(NULL); NOTICE: issuing SELECT user_id, NULL::timestamp with time zone AS timestamptz FROM public.test_102008 test WHERE true DETAIL: on server postgres@localhost:9701 connectionId: 1

I will add this NULL check for consistency but I am not able to add a test for it.

Or, maybe should we use makeConst instead of stringTypeDatum?

makeConst already assumes we are passing a correct Datum for the text type, so we need to use stringTypeDatum to obtain a correct Datum for text type.

also good to check for relable types for NULL values

If arg is NULL it will be handled from the CollateExpr node since we just pass that arg.
collateExpr->arg = relabelType->arg;
This if statement already keeps us safe from any other NULL/invalidOid cases:

if (IsA(inputNode, RelabelType) && OidIsValid(((RelabelType *) inputNode)->resultcollid) && ((RelabelType *) inputNode)->resultcollid != DEFAULT_COLLATION_OID)

naisila · 2022-10-31T13:31:57Z

Note: Not ready for review yet - test_statement_cte is not supposed to return empty set in CitusCacheManyConnectionsConfig and CitusSuperUserDefaultClusterConfig

naisila · 2022-11-02T07:22:34Z

@onderkalaci turns out that even in my local using == with timestamptz is tricky: in fact in my local even more configurations fail.

I used timestamp instead since it serves the exact same purpose for these tests.

Thanks

marcocitus · 2022-11-02T07:45:40Z

src/backend/distributed/planner/multi_physical_planner.c

@@ -2691,6 +2697,9 @@ SqlTaskList(Job *job)
 	List *fragmentCombinationList = FragmentCombinationList(rangeTableFragmentsList,
 															jobQuery, dependentJobList);

+	/* Adjust RelabelType and CoerceViaIO nodes that are improper for deparsing */
+	jobQuery = (Query *) AdjustImproperForDeparseNodes((Node *) jobQuery);


note that query_tree_mutator makes a deep copy of the query tree, so the overhead of always doing this can be significant for complex queries

I assume that's why RelabelTypeToCollateExpr is so far used in the deparser

I have added QTW_DONT_COPY_QUERY flag to the query_tree_mutator function such that it's modified in place

return (Node *) query_tree_mutator((Query *) inputNode, AdjustImproperForDeparseNodes, NULL, QTW_DONT_COPY_QUERY);

From Postgres:

/* * query_tree_mutator --- initiate modification of a Query's expressions * ... * Normally the top-level Query node itself is copied, but some callers want * it to be modified in-place; they must pass QTW_DONT_COPY_QUERY in flags. * All modified substructure is safely copied in any case. */ Query * query_tree_mutator(Query *query, Node *(*mutator) (), void *context, int flags)

Oops I missed All modified substructure is safely copied in any case.
But I guess that's fine, modified substructure shouldn't give significant overhead.

Well, that only takes care of the top-level Query struct and all the expression nodes below it are still copied.

Given the complexity of updating ruleutils, fixing the query tree separately kind of makes sense, but it would be good to add a check (a walker that checks whether any relabel types exist) to reduce the cost when there's nothing to fix.

Separately, we should maybe look into writing a non-copying mutator.

Good point.

onderkalaci

Thanks for the fix and applying @marcocitus's suggestion.

I'm happy that we fixed this on the planner not deparser. Let's wait before merging and give a chance for @marcocitus to have a look, just in case he has additional comments

onderkalaci · 2022-11-09T13:44:19Z

src/test/regress/expected/prepared_statements_4.out

+(1 row)
+
+PREPARE test_statement_cte(text, text) AS
+WITH cte_1 AS (SELECT user_id, t FROM test WHERE t >= $1::timestamp ORDER BY user_id)


Adding MATERIALIZE would make this test more interesting: WITH cte_1 AS MATERIALIZED AS ...

Because I want to see how intermediate results interact with this case, and MATERIALIZE would do that

onderkalaci · 2022-11-09T13:49:39Z

src/backend/distributed/planner/multi_physical_planner.c

+	{
+		return false;
+	}
+	else if (IsA(inputNode, RelabelType) || IsA(inputNode, CoerceViaIO))


should we expand CoerceViaIO check to if (IsA(arg, Const) && ((Const *) arg)->consttype == CSTRINGOID). I mean, basically get the check for CoerceViaIO in AdjustImproperForDeparseNodes to a function, and use it here as well?

With that, we can also avoid unnecessary mutator calls

(similar suggestion applies for RelabelType checks, extract the check into a function and use it in both places)

onderkalaci · 2022-11-09T13:50:29Z

src/backend/distributed/planner/multi_physical_planner.c

@@ -2691,6 +2697,12 @@ SqlTaskList(Job *job)
 	List *fragmentCombinationList = FragmentCombinationList(rangeTableFragmentsList,
 															jobQuery, dependentJobList);

+	/* Adjust RelabelType and CoerceViaIO nodes that are improper for deparsing */
+	if (QueryTreeHasRelabelTypeOrCoerceViaIONodes((Node *) jobQuery))
+	{


would be nice to have a comment on why we first check the query tree and then only do AdjustImproperForDeparseNodes later. It might not be obvious for the readers.

onderkalaci · 2023-01-30T07:15:07Z

@naisila is there a reason for not merging this?

onderkalaci · 2023-01-30T07:15:42Z

@naisila is there a reason for not merging this?

I mean, lets not get this to 11.2 at this point, but right after the release, we can get to 11.3 (if there is not something I'm missing)

naisila · 2023-01-30T08:25:02Z

@naisila is there a reason for not merging this?

I was a bit worried about query performance. I wonder, should I run a benchmark for this? The extra query tree walker whenever we have a distributed query is usually not a costly operation, but is it negligible enough even for complex queries? I wasn't sure.

If it could be a problem, we might consider fixing this in the ruleutils after all since that type of fix doesn't add any extra cost in performance.

codecov · 2023-04-24T12:22:31Z

Codecov Report

Merging #6391 (10a9e29) into main (1662694) will decrease coverage by 1.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #6391      +/-   ##
==========================================
- Coverage   93.19%   91.98%   -1.21%     
==========================================
  Files         269      269              
  Lines       57563    57593      +30     
==========================================
- Hits        53643    52977     -666     
- Misses       3920     4616     +696

naisila · 2023-04-28T13:42:15Z

I tested the performance of a "complex" query on my local machine. Query has 9 subqueries. Avg query time is 30ms in both cases.

Setup:

CREATE TABLE test(t timestamp, user_id int);
SELECT create_distributed_table('test', 'user_id');
INSERT INTO test VALUES ('2022-02-02', 0);
INSERT INTO test VALUES ('2022-01-01', 1);
INSERT INTO test VALUES ('2021-01-01', 2);

Main branch:

SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM
(SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM test WHERE t >= '2022-01-01'::timestamp) t1
WHERE t >= '2022-01-01'::timestamp) t2 WHERE t >= '2022-01-01'::timestamp) t3 WHERE t >= '2022-01-01'::timestamp) t4
WHERE t >= '2022-01-01'::timestamp) t5 WHERE t >= '2022-01-01'::timestamp) t6 WHERE t >= '2022-01-01'::timestamp) t7 
WHERE t >= '2022-01-01'::timestamp) t8 WHERE t >= '2022-01-01'::timestamp) t9 WHERE t >= '2022-01-01'::timestamp) t10 
WHERE t >= '2022-01-01'::timestamp;

This branch

SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM
(SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM test WHERE t >= '2022-01-01'::text::timestamp) t1
WHERE t >= '2022-01-01'::text::timestamp) t2 WHERE t >= '2022-01-01'::text::timestamp) t3 WHERE t >= '2022-01-01'::text::timestamp) t4
WHERE t >= '2022-01-01'::text::timestamp) t5 WHERE t >= '2022-01-01'::text::timestamp) t6 WHERE t >= '2022-01-01'::text::timestamp) t7
WHERE t >= '2022-01-01'::timestamp) t8 WHERE t >= '2022-01-01'::text::timestamp) t9 WHERE t >= '2022-01-01'::text::timestamp) t10
WHERE t >= '2022-01-01'::text::timestamp;

naisila · 2023-04-28T13:43:28Z

Note: have to fix https://app.circleci.com/pipelines/github/citusdata/citus/32391/workflows/13edbaf3-4827-4033-8f9a-70788b370680/jobs/1107031

… deparsing (#6391) Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing The standard planner converts some `::text` casts to `::cstring` and here we convert back because `cstring` is a pseudotype and it cannot be casted to most types. This problem occurs in CoerceViaIO nodes. There was another problem with RelabelType nodes fixed in the following PR: #4580 We undo the changes in that PR, and fix both CoerceViaIO and RelabelType nodes in the planning phase (not in the deparsing phase in ruleutils) Fixes #5646 Fixes #5033 Fixes #6061

naisila requested a review from onderkalaci October 3, 2022 10:49

onderkalaci reviewed Oct 5, 2022

View reviewed changes

naisila force-pushed the naisila/casting branch from c25d47f to 3a40199 Compare October 6, 2022 14:10

naisila closed this Oct 7, 2022

naisila force-pushed the naisila/casting branch from 3a40199 to 86e186f Compare October 7, 2022 12:51

naisila reopened this Oct 7, 2022

naisila force-pushed the naisila/casting branch 5 times, most recently from c41160c to 9e31d55 Compare October 10, 2022 13:29

naisila requested a review from onderkalaci October 10, 2022 13:41

onderkalaci reviewed Oct 18, 2022

View reviewed changes

naisila force-pushed the naisila/casting branch from 9e31d55 to 2384c53 Compare October 31, 2022 13:30

naisila force-pushed the naisila/casting branch 2 times, most recently from 635f5db to c75d80c Compare November 2, 2022 07:13

naisila requested a review from onderkalaci November 2, 2022 07:20

marcocitus reviewed Nov 2, 2022

View reviewed changes

naisila changed the title ~~Change cstring casts back to text when parsing an expression~~ Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing Nov 2, 2022

naisila force-pushed the naisila/casting branch 2 times, most recently from f3166ec to aacd5e7 Compare November 2, 2022 13:30

naisila requested a review from marcocitus November 2, 2022 13:53

onderkalaci approved these changes Nov 9, 2022

View reviewed changes

naisila removed the request for review from marcocitus November 14, 2022 13:28

naisila force-pushed the naisila/casting branch from aacd5e7 to d0ace27 Compare November 14, 2022 13:31

naisila requested a review from marcocitus November 14, 2022 13:32

naisila force-pushed the naisila/casting branch from d0ace27 to 1f40360 Compare April 24, 2023 12:20

naisila force-pushed the naisila/casting branch from 1f40360 to ad74ef6 Compare April 28, 2023 12:54

naisila added 2 commits May 4, 2023 16:16

Modify rewritten query's problematic nodes before creating task lists

18f4c37

Remove collate default outputs

10a9e29

naisila force-pushed the naisila/casting branch from ad74ef6 to 10a9e29 Compare May 4, 2023 13:30

naisila merged commit 072ae44 into main May 4, 2023
111 checks passed

naisila deleted the naisila/casting branch May 4, 2023 13:46

aykut-bozkurt mentioned this pull request Jul 11, 2023

Changelog entries for 12.0.0 #7049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing #6391

Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing #6391

naisila commented Oct 3, 2022 •

edited

onderkalaci Oct 5, 2022

naisila Oct 5, 2022 •

edited

onderkalaci Oct 5, 2022

naisila Oct 5, 2022

naisila commented Oct 10, 2022

onderkalaci Oct 18, 2022

onderkalaci Oct 18, 2022

onderkalaci Oct 18, 2022

naisila Oct 31, 2022

onderkalaci Oct 18, 2022

onderkalaci Oct 18, 2022

onderkalaci Oct 18, 2022

naisila Oct 31, 2022 •

edited

naisila Oct 31, 2022

naisila Oct 31, 2022

naisila commented Oct 31, 2022 •

edited

naisila commented Nov 2, 2022

marcocitus Nov 2, 2022

naisila Nov 2, 2022

naisila Nov 2, 2022 •

edited

marcocitus Nov 2, 2022 •

edited

naisila Nov 2, 2022

onderkalaci left a comment

onderkalaci Nov 9, 2022

onderkalaci Nov 9, 2022

onderkalaci Nov 9, 2022

onderkalaci commented Jan 30, 2023

onderkalaci commented Jan 30, 2023

naisila commented Jan 30, 2023 •

edited

codecov bot commented Apr 24, 2023 •

edited

naisila commented Apr 28, 2023

naisila commented Apr 28, 2023

Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing #6391

Adjusts query's CoerceViaIO & RelabelType nodes that are improper for deparsing #6391

Conversation

naisila commented Oct 3, 2022 • edited

Choose a reason for hiding this comment

naisila Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naisila commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naisila Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naisila commented Oct 31, 2022 • edited

naisila commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naisila Nov 2, 2022 • edited

Choose a reason for hiding this comment

marcocitus Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onderkalaci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onderkalaci commented Jan 30, 2023

onderkalaci commented Jan 30, 2023

naisila commented Jan 30, 2023 • edited

codecov bot commented Apr 24, 2023 • edited

Codecov Report

naisila commented Apr 28, 2023

Setup:

Main branch:

This branch

naisila commented Apr 28, 2023

naisila commented Oct 3, 2022 •

edited

naisila Oct 5, 2022 •

edited

naisila Oct 31, 2022 •

edited

naisila commented Oct 31, 2022 •

edited

naisila Nov 2, 2022 •

edited

marcocitus Nov 2, 2022 •

edited

naisila commented Jan 30, 2023 •

edited

codecov bot commented Apr 24, 2023 •

edited