Обсуждение: CAST from numeric(18,3) to numeric doesnt work, posgresql 13.3

=?UTF-8?B?SsOhbiBQZWNzxZFr?= <jan.pecsok@gmail.com> writes:
> Where conversion from numeric (18,3) to numeric didnt work as expected.

Hmm, interesting.  The cause of this is that coerce_type_typmod
supposes that casting to a -1 typmod is a total no-op.  Which
is true so far as the run-time behavior is concerned, but not
if something inspects the exposed type of the expression.

The attached seems to be enough to fix it, though I wonder if
we've made the same assumption anywhere else.

Although this changes no existing regression test results,
I'm still a bit hesitant to back-patch it, because I am not
real sure that nothing out there is depending on the current
behavior (which has stood for decades).  I think it'd be all
right to put into HEAD, and maybe it's not too late for v14.

            regards, tom lane

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 5da72a4c36..78194afedf 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -758,25 +758,33 @@ coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod,
     CoercionPathType pathtype;
     Oid            funcId;

-    /*
-     * A negative typmod is assumed to mean that no coercion is wanted. Also,
-     * skip coercion if already done.
-     */
-    if (targetTypMod < 0 || targetTypMod == exprTypmod(node))
+    /* Skip coercion if already done */
+    if (targetTypMod == exprTypmod(node))
         return node;

+    /* Suppress display of nested coercion steps */
+    if (hideInputCoercion)
+        hide_coercion_node(node);
+
     pathtype = find_typmod_coercion_function(targetTypeId, &funcId);

     if (pathtype != COERCION_PATH_NONE)
     {
-        /* Suppress display of nested coercion steps */
-        if (hideInputCoercion)
-            hide_coercion_node(node);
-
         node = build_coercion_expression(node, pathtype, funcId,
                                          targetTypeId, targetTypMod,
                                          ccontext, cformat, location);
     }
+    else
+    {
+        /*
+         * We don't need to perform any actual coercion step, but we should
+         * apply a RelabelType to ensure that the expression exposes the
+         * intended typmod.
+         */
+        node = applyRelabelType(node, targetTypeId, targetTypMod,
+                                exprCollation(node),
+                                cformat, location, false);
+    }

     return node;
 }
diff --git a/src/test/regress/expected/expressions.out b/src/test/regress/expected/expressions.out
index 84159cb21f..7b6b0bb4f9 100644
--- a/src/test/regress/expected/expressions.out
+++ b/src/test/regress/expected/expressions.out
@@ -158,6 +158,43 @@ select count(*) from date_tbl
     13
 (1 row)

+--
+-- Test parsing of a no-op cast to a type with unspecified typmod
+--
+begin;
+create table numeric_tbl (f1 numeric(18,3), f2 numeric);
+create view numeric_view as
+  select
+    f1, f1::numeric(16,4) as f1164, f1::numeric as f1n,
+    f2, f2::numeric(16,4) as f2164, f2::numeric as f2n
+  from numeric_tbl;
+\d+ numeric_view
+                           View "public.numeric_view"
+ Column |     Type      | Collation | Nullable | Default | Storage | Description
+--------+---------------+-----------+----------+---------+---------+-------------
+ f1     | numeric(18,3) |           |          |         | main    |
+ f1164  | numeric(16,4) |           |          |         | main    |
+ f1n    | numeric       |           |          |         | main    |
+ f2     | numeric       |           |          |         | main    |
+ f2164  | numeric(16,4) |           |          |         | main    |
+ f2n    | numeric       |           |          |         | main    |
+View definition:
+ SELECT numeric_tbl.f1,
+    numeric_tbl.f1::numeric(16,4) AS f1164,
+    numeric_tbl.f1::numeric AS f1n,
+    numeric_tbl.f2,
+    numeric_tbl.f2::numeric(16,4) AS f2164,
+    numeric_tbl.f2 AS f2n
+   FROM numeric_tbl;
+
+explain (verbose, costs off) select * from numeric_view;
+                                                                      QUERY PLAN
                                

+-------------------------------------------------------------------------------------------------------------------------------------------------------
+ Seq Scan on public.numeric_tbl
+   Output: numeric_tbl.f1, (numeric_tbl.f1)::numeric(16,4), (numeric_tbl.f1)::numeric, numeric_tbl.f2,
(numeric_tbl.f2)::numeric(16,4),numeric_tbl.f2 
+(2 rows)
+
+rollback;
 --
 -- Tests for ScalarArrayOpExpr with a hashfn
 --
diff --git a/src/test/regress/sql/expressions.sql b/src/test/regress/sql/expressions.sql
index bf30f41505..e9aa1a0b28 100644
--- a/src/test/regress/sql/expressions.sql
+++ b/src/test/regress/sql/expressions.sql
@@ -66,6 +66,27 @@ select count(*) from date_tbl
 select count(*) from date_tbl
   where f1 not between symmetric '1997-01-01' and '1998-01-01';

+
+--
+-- Test parsing of a no-op cast to a type with unspecified typmod
+--
+begin;
+
+create table numeric_tbl (f1 numeric(18,3), f2 numeric);
+
+create view numeric_view as
+  select
+    f1, f1::numeric(16,4) as f1164, f1::numeric as f1n,
+    f2, f2::numeric(16,4) as f2164, f2::numeric as f2n
+  from numeric_tbl;
+
+\d+ numeric_view
+
+explain (verbose, costs off) select * from numeric_view;
+
+rollback;
+
+
 --
 -- Tests for ScalarArrayOpExpr with a hashfn
 --

On Thu, 5 Aug 2021 at 02:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Hmm, interesting.  The cause of this is that coerce_type_typmod
> supposes that casting to a -1 typmod is a total no-op.  Which
> is true so far as the run-time behavior is concerned, but not
> if something inspects the exposed type of the expression.
>
> The attached seems to be enough to fix it, though I wonder if
> we've made the same assumption anywhere else.

+1 for fixing this.

> Although this changes no existing regression test results,
> I'm still a bit hesitant to back-patch it, because I am not
> real sure that nothing out there is depending on the current
> behavior (which has stood for decades).  I think it'd be all
> right to put into HEAD, and maybe it's not too late for v14.

This isn't the first time someone has hit this [1]. The failing CTE
query there is quite interesting because if ::varchar[] with no typmod
wasn't a no-op, it could have been used to fix it. In fact, that's
what the error message and hint suggest doing.

So this fix is potentially more widely useful, though I couldn't say
whether anyone is relying on the current behaviour.

Regards,
Dean

[1] https://dba.stackexchange.com/questions/116218/surprising-results-for-data-types-with-type-modifier

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Thu, 5 Aug 2021 at 02:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Although this changes no existing regression test results,
>> I'm still a bit hesitant to back-patch it, because I am not
>> real sure that nothing out there is depending on the current
>> behavior (which has stood for decades).  I think it'd be all
>> right to put into HEAD, and maybe it's not too late for v14.

> This isn't the first time someone has hit this [1]. The failing CTE
> query there is quite interesting because if ::varchar[] with no typmod
> wasn't a no-op, it could have been used to fix it. In fact, that's
> what the error message and hint suggest doing.

Hmm.  Jan's example isn't that compelling, since you could always
create the table manually with the desired column type.  However
the CTE application is more interesting, since there's no really
easy way to satisfy the type-matching rule there.  (You could
cast to the more restrictive typmod, but that might be the Wrong
Thing for your query.)

So maybe we should back-patch, but I still feel nervous about that.
Anybody else have an opinion here?

With one eye on the calendar, I'm thinking that if we do decide
to back-patch we should wait a week, rather than shoving this in
just before a release wrap.  OTOH, if it's v14 only it'd be better
to have it appear in beta3 rather than later.  Maybe we should
push to v14 now, and consider back-patch in a few months if there
are not complaints about beta3?

            regards, tom lane

So maybe we should back-patch, but I still feel nervous about that.
Anybody else have an opinion here?

With one eye on the calendar, I'm thinking that if we do decide
to back-patch we should wait a week, rather than shoving this in
just before a release wrap.  OTOH, if it's v14 only it'd be better
to have it appear in beta3 rather than later.  Maybe we should
push to v14 now, and consider back-patch in a few months if there
are not complaints about beta3?

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> Here is a recent newbie help request I fielded in Discord:

> WITH RECURSIVE fizz_buzz (sequence, modulo_3, modulo_5) AS (
>   SELECT 1, CAST('' AS CHAR(4)), CAST('' AS CHAR(4))
>   UNION ALL
>   SELECT sequence + 1,
>        CASE WHEN MOD(sequence + 1, 3) = 0 THEN 'Fizz'
>      ELSE '' END,
>        CASE WHEN MOD(sequence + 1, 5) = 0 THEN 'Buzz'
>      ELSE '' END
>   FROM fizz_buzz
>   WHERE sequence < 100
> )
> SELECT
>     CASE WHEN CONCAT(modulo_3, modulo_5) = '' THEN sequence
>     ELSE CONCAT(modulo_3, modulo_5) END AS fizzbuzz
> FROM fizz_buzz;

I'd class that one as pilot error, in that the query author
isn't even trying to make the two sides of the union agree
type-wise.  It's not the same as the point at hand, where
someone did try to cast to the correct type but the parser
ignored it.

> And the just posted Bug 17137:
> https://www.postgresql.org/message-id/17137-3d3732d5a259612c%40postgresql.org
> Are these related?

Right, that one is a dup of the current issue.

> I would agree with doing this in v14 then back-patch with the next update.

Yeah, I'm going to push to v14 shortly if I don't hear objections soon.

            regards, tom lane

A customer reported a planning regression from 11.5 to 12.9. After
bisecting, it seems the fix for the thread subject here, commit
f230614da28, broke partition pruning in some cases. Here's a
reproducer:

drop table if exists test_pruning;
create table test_pruning (account_id character(16) primary key)
partition by hash(account_id);
create table p0_test_pruning partition of test_pruning
    for values with (modulus 5, remainder 0);
create table p1_test_pruning partition of test_pruning
    for values with (modulus 5, remainder 1);
create table p2_test_pruning partition of test_pruning
    for values with (modulus 5, remainder 2);
create table p3_test_pruning partition of test_pruning
    for values with (modulus 5, remainder 3);
create table p4_test_pruning partition of test_pruning
    for values with (modulus 5, remainder 4);

insert into test_pruning select 'XY' || lpad(i::text, 14, '0') from
generate_series(1,1000000,1) as i;

-- explicit cast on both operands
EXPLAIN (ANALYZE)
SELECT COUNT(*) FROM test_pruning WHERE account_id::BPCHAR =
'XY99999999999999'::BPCHAR;

-- explicit cast only on const
EXPLAIN (ANALYZE)
SELECT COUNT(*) FROM test_pruning WHERE account_id = 'XY99999999999999'::BPCHAR;

These queries both allowed partition pruning before, but with this
commit only the latter does:

                                                                   QUERY PLAN

-------------------------------------------------------------------------------------------------------------------------------------------------
 Finalize Aggregate  (cost=12159.67..12159.68 rows=1 width=8) (actual
time=66.891..68.641 rows=1 loops=1)
   ->  Gather  (cost=12159.46..12159.67 rows=2 width=8) (actual
time=66.784..68.635 rows=3 loops=1)
         Workers Planned: 2
         Workers Launched: 2
         ->  Partial Aggregate  (cost=11159.46..11159.47 rows=1
width=8) (actual time=56.140..56.142 rows=1 loops=3)
               ->  Parallel Append  (cost=0.00..11156.64 rows=1128
width=0) (actual time=56.137..56.138 rows=0 loops=3)
                     ->  Parallel Seq Scan on p2_test_pruning
(cost=0.00..2238.25 rows=320 width=0) (actual time=35.421..35.421
rows=0 loops=1)
                           Filter: ((account_id)::bpchar =
'XY99999999999999'::bpchar)
                           Rows Removed by Filter: 200799
                     ->  Parallel Seq Scan on p3_test_pruning
(cost=0.00..2236.50 rows=319 width=0) (actual time=39.589..39.589
rows=0 loops=1)
                           Filter: ((account_id)::bpchar =
'XY99999999999999'::bpchar)
                           Rows Removed by Filter: 200523
                     ->  Parallel Seq Scan on p4_test_pruning
(cost=0.00..2227.75 rows=318 width=0) (actual time=10.156..10.156
rows=0 loops=3)
                           Filter: ((account_id)::bpchar =
'XY99999999999999'::bpchar)
                           Rows Removed by Filter: 66599
                     ->  Parallel Seq Scan on p0_test_pruning
(cost=0.00..2224.25 rows=318 width=0) (actual time=17.785..17.785
rows=0 loops=2)
                           Filter: ((account_id)::bpchar =
'XY99999999999999'::bpchar)
                           Rows Removed by Filter: 99715
                     ->  Parallel Seq Scan on p1_test_pruning
(cost=0.00..2224.25 rows=318 width=0) (actual time=27.336..27.336
rows=0 loops=1)
                           Filter: ((account_id)::bpchar =
'XY99999999999999'::bpchar)
                           Rows Removed by Filter: 199452
 Planning Time: 0.476 ms
 Execution Time: 68.716 ms
(23 rows)

                                                                   QUERY PLAN

-------------------------------------------------------------------------------------------------------------------------------------------------
 Aggregate  (cost=8.44..8.45 rows=1 width=8) (actual time=0.016..0.016
rows=1 loops=1)
   ->  Index Only Scan using p2_test_pruning_pkey on p2_test_pruning
(cost=0.42..8.44 rows=1 width=0) (actual time=0.014..0.014 rows=0
loops=1)
         Index Cond: (account_id = 'XY99999999999999'::bpchar)
         Heap Fetches: 0
 Planning Time: 0.071 ms
 Execution Time: 0.038 ms
(6 rows)

--
John Naylor
EDB: http://www.enterprisedb.com

John Naylor <john.naylor@enterprisedb.com> writes:
> A customer reported a planning regression from 11.5 to 12.9. After
> bisecting, it seems the fix for the thread subject here, commit
> f230614da28, broke partition pruning in some cases.

[ facepalm... ]  I didn't consider the possibility that
find_typmod_coercion_function would find something ...
so we're getting an actual call of bpchar(), not just
a RelabelType.

            regards, tom lane