Convert relabeltype into collateexpr in deparser #4580

halilozanakgul · 2021-01-26T10:50:55Z

Moves the RelabelTypeMutator calls to the get_rule_expr function

codecov · 2021-01-26T14:00:24Z

Codecov Report

Merging #4580 (5dd2a3d) into master (f96e93a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4580   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files         212      212           
  Lines       42701    42707    +6     
=======================================
+ Hits        39580    39588    +8     
+ Misses       3121     3119    -2

marcocitus · 2021-01-27T15:57:42Z

src/backend/distributed/deparser/ruleutils_11.c

@@ -4812,6 +4812,8 @@ get_rule_expr(Node *node, deparse_context *context,
 	CHECK_FOR_INTERRUPTS();
 	check_stack_depth();

+	node = RelabelTypeMutator(node);


get_rule_expr gets called a lot, so this walker will likely cause excessive CPU.

Is there a way to integrate the logic from RelabelTypeMutator into the switch statement below?

src/backend/distributed/deparser/ruleutils_11.c

marcocitus · 2021-01-28T10:06:02Z

src/backend/distributed/deparser/ruleutils_12.c

 		case T_CollateExpr:
 			{
+				node = RelabelTypeMutator(node);


Do we still need a mutator if we already know this is a RelabelType?

marcocitus · 2021-02-04T10:27:43Z

src/backend/distributed/deparser/ruleutils_13.c

@@ -4881,6 +4881,12 @@ get_rule_expr(Node *node, deparse_context *context,
 	CHECK_FOR_INTERRUPTS();
 	check_stack_depth();

+	if (nodeTag(node) == T_RelabelType &&
+		((RelabelType *)node)->resultcollid != InvalidOid)


why not do this in the switch statement below?

It didn't work. I guess the reason is: Switch statement checks the initial node only; however, the node we are looking for might be somewhere we can reach by recursive search. @halilozanakgul what do you think?

Hmz, I don't follow.

How can

if (nodeTag(node) == T_RelabelType && ((RelabelType *) node)->resultcollid != InvalidOid) { node = RelabelTypeMutator(node); } switch (nodeTag(node)) { case T_RelabelType: { ... } }

be different from

switch (nodeTag(node)) { case T_RelabelType: { RelabelType *relabel = (RelabelType *) node; Node *arg = (Node *) relabel->arg; if (relabel->resultcollid != InvalidOid) { node = RelabelTypeMutator(node); } ... } }

?

ah, you're right. I've misunderstood

Ah, because in the first case we go into the CollateExpr switch instead.

what if we do:

case T_RelabelType: { RelabelType *relabel = (RelabelType *) node; Node *arg = (Node *) relabel->arg; /* * Citus modification: * The planner rewrites CollateExpr to RelabelType, hence we print * RelabelType with a collation ID as a COLLATE expression. */ if (relabel->resultcollid != InvalidOid) { if (!PRETTY_PAREN(context)) appendStringInfoChar(buf, '('); get_rule_expr_paren(arg, context, showimplicit, node); appendStringInfo(buf, " COLLATE %s", generate_collation_name(collate->collOid)); if (!PRETTY_PAREN(context)) appendStringInfoChar(buf, ')'); break; } ... }

marcocitus · 2021-02-04T10:28:08Z

src/backend/distributed/planner/multi_physical_planner.c

+		return (Node *) query_tree_mutator((Query *) originalNode, RelabelTypeMutator,
+										   (void *) NULL, 0);
+	}
+	return expression_tree_mutator(originalNode, RelabelTypeMutator,


since the nodes will be visited recursively by the deparsing logic, do we still need a mutator?

marcocitus · 2021-02-04T14:29:18Z

src/backend/distributed/deparser/ruleutils_12.c


-				if (relabel->relabelformat == COERCE_IMPLICIT_CAST &&
-					!showimplicit)
+				if (relabel->resultcollid != InvalidOid)


can you add a comment that this is a Citus-specific modification? That will make adding support for future PG releases slightly easier.

marcocitus · 2021-02-04T14:29:34Z

src/backend/distributed/deparser/ruleutils_13.c

-									  relabel->resulttype,
-									  relabel->resulttypmod,
-									  node);
+				else {


strange formatting

marcocitus · 2021-02-04T14:29:54Z

src/backend/distributed/planner/multi_physical_planner.c

+CollateExpr *
+RelabelTypeMutator(RelabelType *relabelType)
+{
+	if (relabelType == NULL)


do we need this?

marcocitus · 2021-02-04T14:30:13Z

src/backend/distributed/deparser/ruleutils_13.c

 				{
-					/* don't show the implicit cast */
-					get_rule_expr_paren(arg, context, false, node);
+					CollateExpr *collate = RelabelTypeMutator(relabel);


Maybe RelabelTypeMutator -> RelabelTypeToCollateExpr?

… 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

halilozanakgul force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch from c3dd954 to 5357176 Compare January 26, 2021 10:54

halilozanakgul marked this pull request as ready for review January 27, 2021 07:15

halilozanakgul force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch from 5357176 to 11fdc26 Compare January 27, 2021 07:20

agedemenli force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch 6 times, most recently from c3213b4 to 2944406 Compare January 27, 2021 15:47

marcocitus reviewed Jan 27, 2021

View reviewed changes

marcocitus reviewed Jan 28, 2021

View reviewed changes

src/backend/distributed/deparser/ruleutils_11.c Show resolved Hide resolved

marcocitus reviewed Jan 28, 2021

View reviewed changes

halilozanakgul force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch from f7a2508 to 4b885f9 Compare February 4, 2021 10:00

marcocitus reviewed Feb 4, 2021

View reviewed changes

halilozanakgul force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch from 3b05616 to 84a544b Compare February 4, 2021 14:19

halilozanakgul requested review from marcocitus and agedemenli February 4, 2021 14:27

marcocitus reviewed Feb 4, 2021

View reviewed changes

halilozanakgul force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch from 84a544b to 0d31931 Compare February 4, 2021 14:51

marcocitus approved these changes Feb 4, 2021

View reviewed changes

Convert RelabelTypes into CollateExprs in get_rule_expr function

5dd2a3d

halilozanakgul force-pushed the convert-relabeltype-into-collateexpr-in-deparser branch from 0d31931 to 5dd2a3d Compare February 5, 2021 09:09

halilozanakgul merged commit cbb95af into master Feb 5, 2021

halilozanakgul deleted the convert-relabeltype-into-collateexpr-in-deparser branch February 5, 2021 10:33

onurctirtir mentioned this pull request Jan 24, 2022

Casting varchar to timestamp does not work in a query on a distributed table #5646

Closed

naisila mentioned this pull request Oct 3, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert relabeltype into collateexpr in deparser #4580

Convert relabeltype into collateexpr in deparser #4580

halilozanakgul commented Jan 26, 2021 •

edited

codecov bot commented Jan 26, 2021 •

edited

marcocitus Jan 27, 2021

marcocitus Jan 28, 2021

marcocitus Feb 4, 2021

agedemenli Feb 4, 2021

marcocitus Feb 4, 2021 •

edited

agedemenli Feb 4, 2021

marcocitus Feb 4, 2021 •

edited

marcocitus Feb 4, 2021 •

edited

marcocitus Feb 4, 2021

marcocitus Feb 4, 2021

marcocitus Feb 4, 2021

marcocitus Feb 4, 2021

marcocitus Feb 4, 2021

Convert relabeltype into collateexpr in deparser #4580

Convert relabeltype into collateexpr in deparser #4580

Conversation

halilozanakgul commented Jan 26, 2021 • edited

codecov bot commented Jan 26, 2021 • edited

Codecov Report

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

marcocitus Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcocitus Feb 4, 2021 • edited

Choose a reason for hiding this comment

marcocitus Feb 4, 2021 • edited

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

halilozanakgul commented Jan 26, 2021 •

edited

codecov bot commented Jan 26, 2021 •

edited

marcocitus Feb 4, 2021 •

edited

marcocitus Feb 4, 2021 •

edited

marcocitus Feb 4, 2021 •

edited