Support BCP on TSQL #temp table#4632
Support BCP on TSQL #temp table#4632Ayush-061 wants to merge 15 commits intobabelfish-for-postgresql:BABEL_5_X_DEVfrom
Conversation
| -- Create sp_tablecollations_100 in tempdb for BCP temp table support | ||
| -- This procedure enables SqlBulkCopy to work with temp tables by providing | ||
| -- column collation metadata that BCP needs to encode string data correctly. | ||
| CREATE OR REPLACE PROCEDURE sys.create_sp_tablecollations_100_in_tempdb_dbo() | ||
| LANGUAGE C | ||
| AS 'babelfishpg_tsql', 'create_sp_tablecollations_100_in_tempdb_dbo_internal'; | ||
|
|
||
| CALL sys.create_sp_tablecollations_100_in_tempdb_dbo(); | ||
| ALTER PROCEDURE tempdb_dbo.sp_tablecollations_100 OWNER TO sysadmin; | ||
| GRANT EXECUTE ON PROCEDURE tempdb_dbo.sp_tablecollations_100 TO PUBLIC; | ||
| DROP PROCEDURE sys.create_sp_tablecollations_100_in_tempdb_dbo; |
There was a problem hiding this comment.
upgrades are actually simple because we know tempdb_dbos schema will always exists. so we don't have to do this. we will instead have to create the tempdb_dbo.sp_tablecollations_100 procedure here like a normal procedure
| #endif /* USE_LIBXML */ | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
I thinks its about time babelfish temp table's code have a seperate file in extension : )
There was a problem hiding this comment.
@Ayush-061 lets start with this one. Could you instead create a new file (maybe called - temp-table-hooks) and write this function there instead. We'll move the rest slowly later.
There was a problem hiding this comment.
Only the function get_tsql_temp_table_attribute or the other one is_temp_table name too?
| * (reads from actual pg_attribute catalog). | ||
| */ | ||
| Datum | ||
| get_enr_attributes(PG_FUNCTION_ARGS) |
There was a problem hiding this comment.
can you please check if the function - ENRMetadataGetTupDesc will simplify your job here.
There was a problem hiding this comment.
cattups[ENR_CATTUP_ATTRIBUTE] already has HeapTuples ready, but with ENRMetadataGetTupDesc we would need to manually build HeapTuples from Form_pg_attribute. Both will work .
contrib/babelfishpg_tsql/sql/upgrades/babelfishpg_tsql--5.5.0--5.6.0.sql
Outdated
Show resolved
Hide resolved
| temp_name_ptr = strchr(table_name_input, '#'); | ||
| if (!temp_name_ptr) | ||
| PG_RETURN_NULL(); | ||
|
|
||
| /* Lowercase the table name and strip trailing "]" */ | ||
| table_name = pstrdup(temp_name_ptr); | ||
| for (i = 0; table_name[i]; i++) | ||
| { | ||
| if (table_name[i] == ']') | ||
| { | ||
| table_name[i] = '\0'; | ||
| break; | ||
| } | ||
| table_name[i] = tolower((unsigned char) table_name[i]); | ||
| } |
There was a problem hiding this comment.
maybe use object_id to get the oid of the table and then use get_ENR_withoid(). That would help skip these checks and string handling
| foreach(lc, enr->md.cattups[ENR_CATTUP_ATTRIBUTE]) | ||
| { | ||
| HeapTuple tup = (HeapTuple) lfirst(lc); | ||
|
|
||
| tuplestore_puttuple(tupstore, tup); | ||
| } |
There was a problem hiding this comment.
check if this can be simply replaced by the function ENRMetadataGetTupDesc
| if (object_name[0] != '#') | ||
| { | ||
| pfree(db_name); | ||
| pfree(schema_name); | ||
| pfree(object_name); | ||
| PG_RETURN_NULL(); | ||
| } |
There was a problem hiding this comment.
haven't we already verified that this is a temp table .. before calling this function?
There was a problem hiding this comment.
This is if user calls the function babelfish_get_temp_table_attributes. for procedure the check is done by the other function is_temp_table_name
| /* | ||
| * is_temp_table_name - Check if a name refers to a temp table | ||
| * Returns true if the object name part starts with # | ||
| */ |
There was a problem hiding this comment.
Nit: please add more comments mentioning that the name can be a four-part qualified name, within quoted-identifiers and even within square brackets qualifier.
ayushdsh
left a comment
ayushdsh
left a comment
There was a problem hiding this comment.
I am okay with almost all pieces. I just want one more pass on get_tsql_temp_table_attributes function which I will try to conclude today.
Deepesh125
left a comment
Deepesh125
left a comment
There was a problem hiding this comment.
The approach is fine for me. But we need to prove that it won't cause any interoperability issue or unexpected behaviour.
| GRANT SELECT ON sys.spt_tablecollations_view TO PUBLIC; | ||
|
|
||
| -- Function to get pg_attribute rows for #temp tables (ENR and non-ENR) | ||
| CREATE OR REPLACE FUNCTION sys.babelfish_get_temp_table_attributes(IN table_name sys.varchar(4000)) |
There was a problem hiding this comment.
for safer side, it should be sys.nvarchar(128)
| GRANT EXECUTE ON FUNCTION sys.babelfish_get_temp_table_attributes(IN sys.varchar(4000)) TO PUBLIC; | ||
|
|
||
| -- Function to check if a name refers to a #temp table | ||
| CREATE OR REPLACE FUNCTION sys.is_temp_table_name(IN name sys.varchar(4000)) |
There was a problem hiding this comment.
same as above, use sys.nvarchar
|
|
||
| PG_RETURN_TEXT_P(cstring_to_text(xpath_expr)); | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
revert file back to original
| pfree(input); | ||
|
|
||
| /* Must be a temp table (starts with #) */ | ||
| if (object_name[0] != '#') |
There was a problem hiding this comment.
object_name could be empty or null? any chance this could be called from PG dialect?
| downcase_truncate_split_object_name(input, NULL, &db_name, &schema_name, &object_name); | ||
| pfree(input); | ||
|
|
||
| /* Must be a temp table (starts with #) */ |
There was a problem hiding this comment.
To avoid confusion, let's use tsql temp tables
| input = text_to_cstring(PG_GETARG_VARCHAR_PP(0)); | ||
|
|
||
| /* Parse the table name */ | ||
| downcase_truncate_split_object_name(input, NULL, &db_name, &schema_name, &object_name); |
There was a problem hiding this comment.
why do we need db_name? we can pass NULL instead
| /* Try ENR first */ | ||
| if (currentQueryEnv != NULL) | ||
| { | ||
| enr = get_ENR(currentQueryEnv, object_name, true); |
There was a problem hiding this comment.
have we tested this part of code for following change of commands?
TSQL Dialect -> TSQL Proc -> calls get_tsql_temp_table_attributes
TSQL Dialect -> TSQL Proc -> PG proc -> call get_tsql_temp_table_attributes
TSQL Dialect -> PG proc -> call get_tsql_temp_table_attributes
TSQL Dialect -> PG proc -> TSQL proc -> call get_tsql_temp_table_attributes
remember that TSQL temp table can be created at stage in between
| /* Setup return - use pg_attribute's tuple descriptor */ | ||
| per_query_ctx = rsinfo->econtext->ecxt_per_query_memory; | ||
| oldcontext = MemoryContextSwitchTo(per_query_ctx); | ||
|
|
||
| pg_attribute_rel = table_open(AttributeRelationId, AccessShareLock); | ||
| tupdesc = CreateTupleDescCopy(RelationGetDescr(pg_attribute_rel)); | ||
|
|
||
| tupstore = tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random, | ||
| false, work_mem); | ||
|
|
||
| rsinfo->returnMode = SFRM_Materialize; | ||
| rsinfo->setResult = tupstore; | ||
| rsinfo->setDesc = BlessTupleDesc(tupdesc); |
There was a problem hiding this comment.
is there any reason why we are preferring this way of SRF instead of what PG Doc says, https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-C-RETURN-SET
| HeapTuple tup = (HeapTuple) lfirst(lc); | ||
|
|
||
| tuplestore_puttuple(tupstore, tup); |
There was a problem hiding this comment.
I would suggest to make a copy before puttuple
| /* Check if object_name starts with # */ | ||
| if (object_name[0] == '#') | ||
| result = true; |
There was a problem hiding this comment.
Is it fine to make such decision just base on name? we should look into currentQueryEnv.
There could be issue with interoperability here since name with # in PG is valid and can be regular table. And sys.sp_tablecollations_100 could be called for those PG relations, right? how are we making sure that it works fine under those scenarios?
4 participants
Description
Through this changes now users will be able to perform BCP on #temptable through dotnet's
SqlBulkCopy.sp_tablecollations_100does not route for temp table.sp_tablecollations_100BCP is now supported for #temptablebabelfish_get_temp_table_attributesto get the attributes for both ENR and Non-ENR temp tables.Issues Resolved
BABEL-5264
Test Scenarios Covered
Use case based -
Boundary conditions -
Arbitrary inputs -
Negative test cases -
Minor version upgrade tests -
Major version upgrade tests -
Performance tests -
Tooling impact -
Client tests -
Check List
By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.