fix: [Postgre] QueryBuilder::updateBatch() does not work <column_name>::<type>

[kenjis]

Description
Fixes #7387
See https://forum.codeigniter.com/showthread.php?tid=88871

This PR introduces a new syntax to specify column name and its type, <column_name>::<type>.
E.g., updated_at::TIMESTAMP

$data = [
    [
        'name'                  => 'Derek Jones',
        'country'               => 'Greece',
        'updated_at::TIMESTAMP' => '2023-12-02 18:47:52',
    ],
    [
        'name'                  => 'Ahmadinejad',
        'country'               => 'Greece',
        'updated_at::TIMESTAMP' => '2023-12-02 18:47:52',
    ],
];
$builder->updateBatch($data, 'name');
/*
 * Produces:
 * UPDATE "db_user"
 * SET
 * "country" = _u."country",
 * "updated_at" = _u."updated_at"
 * FROM (
 * SELECT 'Greece' "country", 'Derek Jones' "name", '2023-12-02 18:47:52'::TIMESTAMP "updated_at" UNION ALL
 * SELECT 'Greece' "country", 'Ahmadinejad' "name", '2023-12-02 18:47:52'::TIMESTAMP "updated_at"
 * ) _u
 * WHERE "db_user"."name" = _u."name"
 */

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

Run vendor/bin/rector process system --dry-run --no-progress-bar

Error: ] Could not process                                                      
         "/home/runner/work/CodeIgniter4/CodeIgniter4/vendor/rector/rector/vendo
         r/symplify/easy-parallel/src/ValueObject/ParallelProcess.php" file, due
         to:                                                                    
         "Child process timed out after 120 seconds". On line: 108              

Error: ] Could not process some files, due to:                                  
         "Reached system errors count limit of 50, exiting...".                 

Error: Process completed with exit code 1.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7551028040/job/20557620126?pr=8426

[samsonasik]

@kenjis I am looking of way to improve the performance of Rector, for now, it may require update the parallel config for job size:

-$rectorConfig->parallel(120, 8, 15);
+$rectorConfig->parallel(120, 8, 10);

could you try update it?

[kenjis]

Okay.

1 file with changes
===================

1) system/Database/BaseBuilder.php:1856

    ---------- begin diff ----------
@@ @@

                 $escapedValue = $escape ? $this->db->escape($rowValue) : $rowValue;

-                if ($keyType !== null) {
-                    $clean[] = new SqlValue($escapedValue, $keyType);
-                } else {
-                    $clean[] = $escapedValue;
-                }
+                $clean[] = $keyType !== null ? new SqlValue($escapedValue, $keyType) : $escapedValue;
             }

             $row = $clean;
    ----------- end diff -----------

Applied rules:
 * SimplifyIfElseToTernaryRector


 [OK] 1 file would have changed (dry-run) by Rector                             

Error: Process completed with exit code 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7554007555/job/20565980505?pr=8426

[kenjis]

Oh no, I got the error on my MBA.

$ vendor/bin/rector process system/Database/BaseBuilder.php
 0/1 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   0%
                                                                                                                        
 [ERROR] Could not process                                                                                              
         "/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/rector/rector/vendor/symplify/easy-parallel/src/Val
         ueObject/ParallelProcess.php" file, due to:                                                                    
         "Child process timed out after 120 seconds". On line: 108                                                      
                                                                                                                        

                                                                                                                        
 [ERROR] Could not process some files, due to:                                                                          
         "Reached system errors count limit of 50, exiting...". 

[samsonasik]

Hm.., timeout may need to be adjusted

-$rectorConfig->parallel(120, 8, 10);
+$rectorConfig->parallel(240, 8, 10);

to make work on various resource, could you try?

[kenjis]

I reran vendor/bin/rector process system/Database/BaseBuilder.php and passed.

[michalsn]

Does this problem only occur in Postgre?

[kenjis]

Does this problem only occur in Postgre?

Almost yes. At least, there is no bug report.

I found the case when using binary data on MySQL #8282 (comment)
but binary data is special.

[michalsn]

Looks good. This is probably the best we can do right now.

[kenjis]

@codeigniter4/database-team Is the new syntax <column_name>::<type> okay?

[sclubricants]

It seems "CREATE CAST" might be the answer.

https://www.postgresql.org/docs/current/sql-createcast.html

Most Db implicitly do this but it looks like Postgre makes it configurable.

CREATE CAST (varchar AS integer) WITH INOUT AS IMPLICIT;

[sclubricants]

Alias name is not getting quoted like:

"alias"."column"

[kenjis]

It seems $this->QBOptions['alias'] is quoted.

CodeIgniter4/system/Database/BaseBuilder.php

Line 2010 in 0f43ba1

$this->QBOptions['alias'] = $this->db->protectIdentifiers($alias);

[sclubricants]

            $data = [
                [
                    'name'                  => 'Derek Jones',
                    'country'               => 'Greece',
                    'updated_at::TIMESTAMP' => '2023-12-02 18:47:52',
                ],
            ];

I really don' t think this is the best way to implement this. The reason is that you are altering the key name in the array and I feel this might cause problems further down the line. Your operation is on changing the expression of the value, not the key.

Here is a good use of RawSql. You could create a specific class to postgres and have it return RawSql.

            $data = [
                [
                    'name'                  => 'Derek Jones',
                    'country'               => 'Greece',
                    'updated_at' => new RawSql("'2023-12-02 18:47:52'::TIMESTAMP"),
                ],
            ];

All the sql rendering methods already support RawSql. It might be nice if we had a RawSql interface instead. Then we could have different classes under one type all with the __toString() method.

Where PostgresCastValue impliments RawSqlInterface

            $data = [
                [
                    'name'                  => 'Derek Jones',
                    'country'               => 'Greece',
                    'updated_at' => new PostgresCastValue('2023-12-02 18:47:52', 'TIMESTAMP'),
                ],
            ];

then all sql rendering methods look for a RawSqlInterface object and render it without quotes like RawSql is.

[kenjis]

I tested on PostgreSQL 13, Docker image postgres:13-alpine.

[sclubricants]

It seems that the issue is creating the virtual table via select union all. Postgre is not applying implicit casting like other dbms. Everything gets selected into table as int or text..

One option would be to lookup the target table and cast all variables to their type for batch operations. I would assume this affects upserts as well.

This might be resolved by create cast

https://www.postgresql.org/docs/current/sql-createcast.html

[sclubricants]

Or add a method to specify column types?

$builder->setType(['updated_at' => 'TIMESTAMP'])->updateBatch($data, 'name');

I hope there is another solution but this seems the best so far.

[michalsn]

As mentioned, RawSql is not portable and will break the code when we change the handler. First, we look for a non-invasive solution.

new SqlValue('2023-12-02 18:47:52', 'TIMESTAMP'),

This seems reasonable, although I would consider a different class name, such as SqlCast, which would better describe what we are doing.

The alternative - a new method for Query Builder, isn't bad from the user's point of view. The only problem I have with it is that this method would only be useful for one database handler... which doesn't harmonize with the rules we adopted for our Query Builder (no specific implementations for a single driver).

[kenjis]

Okay, there are four options.

(1) Key as <column_name>::<type>

$data = [
    [
        'name'                  => 'Derek Jones',
        'country'               => 'Greece',
        'updated_at::TIMESTAMP' => '2023-12-02 18:47:52',
    ],
];
$builder->updateBatch($data, 'name');

(2) Array as [value, type]

$data = [
    [
         'name'       => 'Derek Jones',
         'country'    => 'Greece',
         'updated_at' => ['2023-12-02 18:47:52', 'TIMESTAMP'],
     ],
];
$builder->updateBatch($data, 'name');

(3) New class that represents value and type

$data = [
    [
         'name'       => 'Derek Jones',
         'country'    => 'Greece',
         'updated_at' => new SqlCast('2023-12-02 18:47:52', 'TIMESTAMP'),
     ],
];
$builder->updateBatch($data, 'name');

(4) New method to specify column types

$data = [
    [
         'name'       => 'Derek Jones',
         'country'    => 'Greece',
         'updated_at' => '2023-12-02 18:47:52',
     ],
];
$builder->setType(['updated_at' => 'TIMESTAMP'])->updateBatch($data, 'name');

[michalsn]

1. seems not very handy, we have to modify the key name
2. seems to be a little too loose, the array can be anything
3. probably my favorite solution
4. I'm not a fan of adding an extra method, especially since it would only be used by Postgre

The question is, how common a problem is casting for Postgre? I'm just worried about the scenario where people start asking to implement broader support for it in methods other than updateBatch. That could be a problem with option (3) for sure.

[sclubricants]

I was thinking of something along the libes of (4) but without a method. Instead the database could automatically lookup the keys and return their types. Then each could be casted to its type. This would only be for postgre.

[sclubricants]

@kenjis could you try explicitly casting all values as ::text and see if the error still occurs?

[kenjis]

@sclubricants Do you mean this?

UPDATE "db_user"
SET
"country" = _u."country",
"updated_at" = _u."updated_at"
FROM (
SELECT 'Greece'::TEXT "country", 'Derek Jones'::TEXT "name", '2023-12-02 18:47:52'::TEXT "updated_at" UNION ALL
SELECT 'Greece'::TEXT "country", 'Ahmadinejad'::TEXT "name", '2023-12-02 18:47:52'::TEXT "updated_at"
) _u
WHERE "db_user"."name" = _u."name"

ErrorException: pg_query(): Query failed: ERROR: column "updated_at" is of type timestamp without time zone but expression is of type text
LINE 4: "updated_at" = _u."updated_at"
^
HINT: You will need to rewrite or cast the expression. in /Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/Postgre/Connection.php:187

[sclubricants]

Is the only one we are having problems with timestamp? What about just date?

[sclubricants]

SELECT column_name, data_type FROM information_schema.columns WHERE table_schema = 'public' AND table_name = 'bike_details';

Get datatypes and save to $QBOptions. Then when building sql cast to datatype.

This might be a quick and easy patch until we find something better. This wont change the api.

Id like to talk to some postgre gooroos to see what other solutions there might be.

[kenjis]

Is the only one we are having problems with timestamp? What about just date?

UPDATE "db_type_test"
SET
"type_bigint" = _u."type_bigint",
"type_date" = _u."type_date",
"type_text" = _u."type_text"
FROM (
SELECT 9999999 "type_bigint", '2024-01-01'::TEXT "type_date", 'updated' "type_text", 'test1' "type_varchar" UNION ALL
SELECT 9999999 "type_bigint", '2024-01-01'::TEXT "type_date", 'updated' "type_text", 'test2' "type_varchar"
) _u
WHERE "db_type_test"."type_varchar" = _u."type_varchar"

ErrorException: pg_query(): Query failed: ERROR: column "type_date" is of type date but expression is of type text
LINE 4: "type_date" = _u."type_date",
^
HINT: You will need to rewrite or cast the expression. in /Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/Postgre/Connection.php:187

[sclubricants]

You should be able to do the casts in the set part rather than the select part.

I wander if the first part of set was cast as text would it work. If not then cast the right side qppropriately

..sorry hard to spell out from mobile..

[kenjis]

Oh, thanks.
The following query is valid.

UPDATE "db_type_test"
SET
    "type_bigint" = _u."type_bigint",
    "type_date" = _u."type_date"::DATE,
    "type_text" = _u."type_text"
FROM (
         SELECT 9999999 "type_bigint", '2024-01-01' "type_date", 'updated' "type_text", 'test1' "type_varchar" UNION ALL
         SELECT 9999999 "type_bigint", '2024-01-01' "type_date", 'updated' "type_text", 'test2' "type_varchar"
     ) _u
WHERE "db_type_test"."type_varchar" = _u."type_varchar"

[kenjis]

I was thinking of something along the libes of (4) but without a method. Instead the database could automatically lookup the keys and return their types. Then each could be casted to its type. This would only be for postgre.

I sent another PR #8439 with that way.