Fix typmod for sys.binary datatype (babelfish-for-postgresql#2037)

sys.binary datatype is created as a domain over sys.bbf_binary. In babelfish we assume hex code as varbinary which must then be casted to binary. For casting we fetch rule from hashmap after coercing both source and target to base type.
So binary --> bbf_binary && varbinary-->bbf_varbinary
This cast is currently defined as binary coercible, that is NO modification needed.
https://github.com/babelfish-for-postgresql/babelfish_extensions/blob/c57e8bc7820e1ef697dd9898b3aa433ba504a7ab/contrib/babelfishpg_tsql/src/pltsql_coerce.c#L114

We fix it by adding proper function for varbinary --> binary cast

-> The default length for local variables defined for binary and varbinary should be 1
DECLARE @A binary should be equivalent to DECLARE @A binary(1)
DECLARE @A varbinary should be equivalent to DECLARE @A varbinary(1)

->Problems during dump and restore done during upgrade.
The new function created for the cast may not exists in the source version and the ugrade scripts are run only after pg dump and restore carried out during version upgrade.
So ultimately during dump and restore no cast exists for varbinary to binary, which is a necessary requirement or upgrade will fail with error :
_"pg_restore: error: could not execute query: ERROR: argument of DEFAULT must be type sys."binary", not type sys.varbinary"_.
if we are dumping a procedure with sys.binary arg and a default value

-> binary to varbinary
we do not need to modify binary to varbinary cast since we have a type modifer defined for base type of sys.varbinary
i.e. CAST(sys.bbf_varbiniary as sys.bbf_varbiniary).

Task: BABEL-4544
Signed-off-by: Tanzeel Khan <[email protected]>

.github/workflows/upgrade-test.yml

@@ -121,7 +121,7 @@ jobs:
           done
  
       - name: Upload Logs
-        if: always() && steps.test-file-rename.outcome == 'success'
+        if: always() && steps.upgrade-and-test.outcome != 'success'
         uses: actions/upload-artifact@v4
         with:
           name: upgrade-logs-${{ matrix.upgrade-path.title }}

contrib/babelfishpg_common/sql/binary.sql

@@ -46,6 +46,11 @@ CREATE TYPE sys.BBF_BINARY (
     COLLATABLE     = false
 );
 
+CREATE OR REPLACE FUNCTION sys.varbinarybinary (sys.BBF_VARBINARY, integer, boolean)
+RETURNS sys.BBF_BINARY
+AS 'babelfishpg_common', 'binary'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE; 
+
 CREATE CAST (sys.BBF_BINARY AS sys.BBF_VARBINARY)
 WITHOUT FUNCTION AS IMPLICIT;
 

contrib/babelfishpg_common/sql/upgrades/babelfishpg_common--2.6.0--3.0.0.sql

@@ -0,0 +1,16 @@
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION ""babelfishpg_common"" UPDATE TO '3.0.0'" to load this file. \quit
+
+SELECT set_config('search_path', 'sys, '||current_setting('search_path'), false);
+
+/* This helper function would only be useful and strictly be used during 1.x->2.3 and 2.3->3.0 upgrade. */
+CREATE OR REPLACE FUNCTION sys.babelfish_update_server_collation_name() RETURNS VOID
+LANGUAGE C
+AS 'babelfishpg_common', 'babelfish_update_server_collation_name';
+
+SELECT sys.babelfish_update_server_collation_name();
+
+DROP FUNCTION sys.babelfish_update_server_collation_name();
+
+-- Reset search_path to not affect any subsequent scripts
+SELECT set_config('search_path', trim(leading 'sys, ' from current_setting('search_path')), false);

contrib/babelfishpg_tsql/src/pltsql_coerce.c

@@ -60,6 +60,8 @@ extern PGDLLIMPORT select_common_typmod_hook_type select_common_typmod_hook;
 extern PGDLLIMPORT handle_constant_literals_hook_type handle_constant_literals_hook;
 extern PGDLLIMPORT set_common_typmod_case_expr_hook_type set_common_typmod_case_expr_hook;
 
+extern bool babelfish_dump_restore;
+
 PG_FUNCTION_INFO_V1(init_tsql_coerce_hash_tab);
 PG_FUNCTION_INFO_V1(init_tsql_datatype_precedence_hash_tab);
 PG_FUNCTION_INFO_V1(get_immediate_base_type_of_UDT);
@@ -126,7 +128,7 @@ tsql_cast_raw_info_t tsql_cast_raw_infos[] =
 	{PG_CAST_ENTRY, "sys", "bbf_varbinary", "pg_catalog", "int4", NULL, 'i', 'f'},
 	{PG_CAST_ENTRY, "sys", "bbf_varbinary", "pg_catalog", "int2", NULL, 'i', 'f'},
 	{TSQL_CAST_ENTRY, "sys", "bbf_varbinary", "sys", "rowversion", "varbinaryrowversion", 'i', 'f'},
-	{TSQL_CAST_WITHOUT_FUNC_ENTRY, "sys", "bbf_varbinary", "sys", "bbf_binary", NULL, 'i', 'b'},
+	{TSQL_CAST_ENTRY, "sys", "bbf_varbinary", "sys", "bbf_binary", "varbinarybinary", 'i', 'f'},
 /*  binary     {only allow to cast to integral data type) */
 	{PG_CAST_ENTRY, "sys", "bbf_binary", "pg_catalog", "int8", NULL, 'i', 'f'},
 	{PG_CAST_ENTRY, "sys", "bbf_binary", "pg_catalog", "int4", NULL, 'i', 'f'},
@@ -671,9 +673,26 @@ init_tsql_coerce_hash_tab(PG_FUNCTION_ARGS)
 
 						if (!OidIsValid(entry->castfunc))
 						{
+							/*
+							 * varbinary to binary implicit type cast without function should be allowed during MVU
+							 * since the cast function might not exists when source version is before 14_11 and 15_6
+							 */ 
+							if (babelfish_dump_restore && ((*common_utility_plugin_ptr->is_tsql_varbinary_datatype) (castsource) 
+								&& (*common_utility_plugin_ptr->is_tsql_binary_datatype) (casttarget)))
+							{
+								entry->castfunc = 0;
+								entry->castcontext = COERCION_CODE_IMPLICIT;
+								entry->castmethod = COERCION_METHOD_BINARY;
+								value = hash_search(ht_tsql_cast_info, key, HASH_ENTER, NULL);
+								*(tsql_cast_info_entry_t *) value = *entry;
+								continue;
+							}
 							/* function is not loaded. wait for next scan */
-							inited_ht_tsql_cast_info = false;
-							continue;
+							else
+							{
+								inited_ht_tsql_cast_info = false;
+								continue;
+							}
 						}
 					}
 					break;

contrib/babelfishpg_tsql/src/pltsql_utils.c

@@ -39,7 +39,7 @@ bool		is_tsql_varchar_or_char_datatype(Oid oid); /* sys.char / sys.varchar */
 bool		is_tsql_nchar_or_nvarchar_datatype(Oid oid); /* sys.nchar / sys.nvarchar */
 bool		is_tsql_binary_or_varbinary_datatype(Oid oid); /* sys.binary / sys.varbinary */
 bool		is_tsql_text_ntext_or_image_datatype(Oid oid);
-
+bool		is_tsql_binary_or_varbinary_datatype(Oid oid);
 
 bool
 pltsql_createFunction(ParseState *pstate, PlannedStmt *pstmt, const char *queryString, ProcessUtilityContext context, 
@@ -1106,6 +1106,12 @@ is_tsql_text_ntext_or_image_datatype(Oid oid)
 		(*common_utility_plugin_ptr->is_tsql_image_datatype) (oid);
 }
 
+bool is_tsql_binary_or_varbinary_datatype(Oid oid)
+{
+	return (*common_utility_plugin_ptr->is_tsql_sys_binary_datatype) (oid) ||
+		(*common_utility_plugin_ptr->is_tsql_sys_varbinary_datatype) (oid);
+}
+
 /*
  * Try to acquire a lock with no wait
  */

contrib/babelfishpg_tsql/src/tsqlIface.cpp

@@ -91,6 +91,7 @@ extern "C"
 
 	extern PLtsql_type *parse_datatype(const char *string, int location);
 	extern bool is_tsql_text_ntext_or_image_datatype(Oid oid);
+	extern bool is_tsql_binary_or_varbinary_datatype(Oid oid);
 
 	extern int CurrentLineNumber;
 
@@ -4360,6 +4361,14 @@ makeDeclareStmt(TSqlParser::Declare_statementContext *ctx)
 		const char *name = downcase_truncate_identifier(nameStr.c_str(), nameStr.length(), true);
 		check_dup_declare(name);
 		PLtsql_type *type = parse_datatype(typeStr.c_str(), 0);
+
+		/* (N)(VAR)CHAR and BINARY datatype length is treated as 1 when nothing is provided */
+		if (type->atttypmod == -1 && (is_tsql_any_char_datatype(type->typoid)
+			|| is_tsql_binary_or_varbinary_datatype(type->typoid)))
+		{
+			std::string newTypeStr = typeStr + "(1)"; 
+			type = parse_datatype(newTypeStr.c_str(), 0);
+		}
 
 		PLtsql_variable *var = pltsql_build_variable(name, 0, type, true);
 
@@ -4386,7 +4395,19 @@ makeDeclareStmt(TSqlParser::Declare_statementContext *ctx)
 			{
 				std::string typeStr = ::getFullText(local->data_type());
 				PLtsql_type *type = parse_datatype(typeStr.c_str(), 0);  // FIXME: the second arg should be 'location'
+<<<<<<< HEAD
 				if (is_tsql_text_ntext_or_image_datatype(type->typoid))
+=======
+
+				/* (N)(VAR)CHAR and BINARY datatype length is treated as 1 when nothing is provided */
+				if (type->atttypmod == -1 && (is_tsql_any_char_datatype(type->typoid)
+					|| is_tsql_binary_or_varbinary_datatype(type->typoid)))
+				{
+					std::string newTypeStr = typeStr + "(1)";
+					type = parse_datatype(newTypeStr.c_str(), 0);
+				}
+				else if (is_tsql_text_ntext_or_image_datatype(type->typoid))
+>>>>>>> 8c3b3865 (Fix typmod for sys.binary datatype (#2037))
 				{
 					throw PGErrorWrapperException(ERROR, ERRCODE_DATATYPE_MISMATCH, "The text, ntext, and image data types are invalid for local variables.", getLineAndPos(local->data_type()));
 				}

test/JDBC/expected/BABEL-1566-vu-verify.out

@@ -281,7 +281,7 @@ select cast(cast(0xfe as binary) as text);
 go
 ~~START~~
 text
-0xfe
+0xfe0000000000000000000000000000000000000000000000000000000000
 ~~END~~
 
 

test/JDBC/expected/BABEL-1566.out

@@ -344,7 +344,7 @@ select cast(cast(0xfe as binary) as text);
 go
 ~~START~~
 text
-0xfe
+0xfe0000000000000000000000000000000000000000000000000000000000
 ~~END~~
 
 

test/JDBC/expected/BABEL-3166-before-14_11-or-15_6-vu-prepare.out

@@ -0,0 +1,43 @@
+-- function
+CREATE FUNCTION babel_3166_func(@a numeric, @b varchar, @c varchar(max), @d varchar(8), @e binary(6))
+RETURNS varbinary(8) AS BEGIN RETURN @e END;
+go
+
+-- Look at the probin for typmod information
+SELECT proname, probin FROM pg_proc WHERE proname = 'babel_3166_func';
+go
+~~START~~
+varchar#!#text
+babel_3166_func#!#{"version_num": "1", "typmod_array": ["1179652", "-1", "-8000", "8", "6", "8"], "original_probin": ""}
+~~END~~
+
+
+SELECT babel_3166_func(1.2, 'abc', 'abcd', 'abcdefgh', 0x12bcfe);
+go
+~~START~~
+varbinary
+12BCFE
+~~END~~
+
+
+-- procedure
+CREATE PROCEDURE babel_3166_proc @a numeric, @b varchar, @c varchar(max), @d varchar(8), @e binary(6)
+AS SELECT @e;
+go
+
+-- Look at the probin for typmod information
+SELECT proname, probin FROM pg_proc WHERE proname = 'babel_3166_proc';
+go
+~~START~~
+varchar#!#text
+babel_3166_proc#!#{"version_num": "1", "typmod_array": ["1179652", "-1", "-8000", "8", "6"], "original_probin": ""}
+~~END~~
+
+
+EXEC babel_3166_proc 1.2, 'abc', 'abcd', 'abcdefgh', 0x12bcfe;
+go
+~~START~~
+binary
+12BCFE000000
+~~END~~
+

test/JDBC/expected/BABEL-3166-before-14_11-or-15_6-vu-verify.out

@@ -0,0 +1,38 @@
+-- Look at function's probin for typmod information
+SELECT proname, probin FROM pg_proc WHERE proname = 'babel_3166_func';
+go
+~~START~~
+varchar#!#text
+babel_3166_func#!#{"version_num": "1", "typmod_array": ["1179652", "-1", "-8000", "8", "6", "8"], "original_probin": ""}
+~~END~~
+
+
+SELECT babel_3166_func(1.2, 'abc', 'abcd', 'abcdefgh', 0x12bcfe);
+go
+~~START~~
+varbinary
+12BCFE000000
+~~END~~
+
+
+
+DROP FUNCTION babel_3166_func;
+-- Look at procedures's probin for typmod information
+SELECT proname, probin FROM pg_proc WHERE proname = 'babel_3166_proc';
+go
+~~START~~
+varchar#!#text
+babel_3166_proc#!#{"version_num": "1", "typmod_array": ["1179652", "-1", "-8000", "8", "6"], "original_probin": ""}
+~~END~~
+
+
+EXEC babel_3166_proc 1.2, 'abc', 'abcd', 'abcdefgh', 0x12bcfe;
+go
+~~START~~
+binary
+12BCFE000000
+~~END~~
+
+
+DROP PROCEDURE babel_3166_proc;
+go

test/JDBC/expected/BABEL-3166-vu-prepare.out

@@ -22,7 +22,7 @@ SELECT babel_3166_func(1.2, 'abc', 'abcd', 'abcdefgh', 0x12bcfe);
 go
 ~~START~~
 varbinary
-12BCFE
+12BCFE000000
 ~~END~~
 
 

test/JDBC/expected/BABEL-3166-vu-verify.out

@@ -14,7 +14,7 @@ SELECT babel_3166_func(1.2, 'abc', 'abcd', 'abcdefgh', 0x12bcfe);
 go
 ~~START~~
 varbinary
-12BCFE
+12BCFE000000
 ~~END~~
 
 

test/JDBC/expected/BABEL_1940.out

@@ -276,7 +276,7 @@ SELECT CAST(CAST(0x61 AS BINARY(3)) AS VARBINARY(2))
 GO
 ~~START~~
 varbinary
-61
+6100
 ~~END~~