Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ALTER VIEW and CREATE OR ALTER VIEW syntax on schema bound views in Babelfish #3570

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

%type <node> tsql_stmt

%type <node> tsql_AlterFunctionStmt tsql_CreatePartitionStmt
%type <node> tsql_AlterFunctionStmt tsql_CreatePartitionStmt tsql_AlterViewStmt
%type <node> tsql_CreateFunctionStmt tsql_VariableSetStmt tsql_CreateTrigStmt tsql_TransactionStmt tsql_UpdateStmt tsql_DeleteStmt tsql_IndexStmt
%type <partspec> tsql_PartitionSpec
%type <node> tsql_DropIndexStmt tsql_InsertStmt
Expand Down
30 changes: 30 additions & 0 deletions contrib/babelfishpg_tsql/src/backend_parser/gram-tsql-rule.y
Original file line number Diff line number Diff line change
Expand Up @@ -2476,6 +2476,7 @@ tsql_stmt :
| AlterTSDictionaryStmt
| AlterUserMappingStmt
| tsql_AlterUserStmt
| tsql_AlterViewStmt
| AnalyzeStmt
| CallStmt
| CheckPointStmt
Expand Down Expand Up @@ -4110,6 +4111,35 @@ tsql_AlterFunctionStmt:
}
;

tsql_AlterViewStmt:
TSQL_ALTER VIEW qualified_name opt_column_list opt_reloptions
AS SelectStmt opt_check_option
{
ViewStmt *n = makeNode(ViewStmt);
n->view = $3;
n->aliases = $4;
n->query = $7;
n->replace = true;
n->options = $5;
n->withCheckOption = $8;
n->createOrAlter = true;
$$ = (Node *) n;
}
| CREATE OR TSQL_ALTER VIEW qualified_name opt_column_list opt_reloptions
AS SelectStmt opt_check_option
{
ViewStmt *n = makeNode(ViewStmt);
n->view = $5;
n->aliases = $6;
n->query = $9;
n->replace = false;
n->options = $7;
n->withCheckOption = $10;
n->createOrAlter = true;
$$ = (Node *) n;
}
;

/*
* These rules define the WITH clause in a CREATE PROCEDURE
* or CREATE FUNCTION statement. This is very similar to
Expand Down
1 change: 1 addition & 0 deletions contrib/babelfishpg_tsql/src/backend_parser/parser.c
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ pgtsql_base_yylex(YYSTYPE *lvalp, YYLTYPE * llocp, core_yyscan_t yyscanner)
case PROCEDURE:
case TSQL_PROC:
case FUNCTION:
case VIEW:
cur_token = TSQL_ALTER;
break;
}
Expand Down
5 changes: 4 additions & 1 deletion contrib/babelfishpg_tsql/src/dbcmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,10 @@ gen_createdb_subcmds(const char *dbname, const char *owner)

/* create sysdatabases under current DB's DBO schema */
appendStringInfo(&query, "CREATE VIEW dummy.sysdatabases AS SELECT * FROM sys.sysdatabases; ");
appendStringInfo(&query, "ALTER VIEW dummy.sysdatabases OWNER TO dummy; ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose for this line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use ALTER VIEW, it will raise syntax error near OWNER while doing initbbf because of different ALTER VIEW behavior between PG and tsql. So we use ALTER TABLE instead to avoid that, as PG can use ALTER TABLE as well to change the ownership of view. (Postgresql Alter Table)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add a comment into the code for this behavior and the purpose for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments in the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test cases for postgres endpoint. ALTER VIEW ... should continue to work as before for postgres endpoint.

Copy link
Contributor Author

@R4hul04 R4hul04 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test cases to check psql ALTER VIEW


/* Changed ALTER VIEW to ALTER TABLE or we will get a syntax error due to different
* ALTER VIEW behavior between PG and TSQL */
appendStringInfo(&query, "ALTER TABLE dummy.sysdatabases OWNER TO dummy; ");

/* create guest schema in the database and revoke create permission */
/* from guest user on guest schema. This has to be the last statement */
Expand Down
188 changes: 188 additions & 0 deletions contrib/babelfishpg_tsql/src/pl_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "commands/tablecmds.h"
#include "commands/trigger.h"
#include "commands/user.h"
#include "commands/view.h"
#include "common/md5.h"
#include "common/string.h"
#include "funcapi.h"
Expand All @@ -63,6 +64,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/guc_tables.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/plancache.h"
#include "utils/ps_status.h"
Expand Down Expand Up @@ -142,6 +144,8 @@ static void call_prev_ProcessUtility(PlannedStmt *pstmt,
QueryCompletion *qc);
static void set_pgtype_byval(List *name, bool byval);
static void pltsql_proc_get_oid_proname_proacl(AlterFunctionStmt *stmt, ParseState *pstate, Oid *oid, Acl **acl, bool *isSameFunc, bool is_proc);
static Acl *get_old_view_acl(Oid oldViewOid);
static void pg_class_update_acl(Oid newViewOid, Acl *oldViewAcl);
static bool pltsql_truncate_identifier(char *ident, int len, bool warn);
static Name pltsql_cstr_to_name(char *s, int len);
extern void pltsql_add_guc_plan(CachedPlanSource *plansource);
Expand Down Expand Up @@ -2766,6 +2770,123 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
}
break;
}

/*
* This case handles the `ALTER VIEW` and `CREATE OR ALTER VIEW` statements in Babelfish.
*/
case T_ViewStmt:
{
ViewStmt *stmt = (ViewStmt *) parsetree;

/*
* We are using PostgreSQL's existing ViewStmt node which is shared between PostgreSQL's
* CREATE VIEW and T-SQL's ALTER VIEW/CREATE OR ALTER VIEW operations. To properly distinguish
* between these operations and not let CREATE VIEW inside this case we use createOrAlter flag
*
* Since both CREATE VIEW and CREATE OR ALTER VIEW set replace = false initially,
* we use the 'createOrAlter' flag to distinguish between them and implement the
* correct behavior when a view already exists
*/

if (sql_dialect == SQL_DIALECT_TSQL && (stmt->createOrAlter))
{
/*
* 1. Retrieve the OID of the old view using `RangeVarGetRelid()`.
* 2. If the old view does not exist, create the new view using `DefineView()` and increment the command counter.
* 3. If the old view exists:
* a. Save the ACL information of the current view.
* b. Drop the current view using `performDeletion()`.
* c. Create the new view using `DefineView()` and increment the command counter.
* f. Store the new view definition in the `bbf_view_def` catalog using `store_view_definition_hook()`.
* g. Update the ACL information for the new view using `pg_class_update_acl()`.
*/
ObjectAddress address, originalView;
Oid oldViewOid;
Acl *oldViewAcl = NULL;
bool isCompleteQuery = (context != PROCESS_UTILITY_SUBCOMMAND);
bool needCleanup;

if (!IS_TDS_CLIENT())
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("TSQL ALTER VIEW is not supported from PostgreSQL endpoint.")));
}

needCleanup = isCompleteQuery && EventTriggerBeginCompleteQuery();

PG_TRY();
{
StartTransactionCommand();

/* Without this, DDL event triggers won't fire for ALTER VIEW operations
* Currently T-SQL DDL triggers are not supported, but this code block is required for PostgreSQL DDL event triggers.
* This will be used later when T-SQL DDL trigger support is implemented.*/

if (isCompleteQuery)
EventTriggerDDLCommandStart(parsetree);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is the test case for this ?

Copy link
Contributor Author

@R4hul04 R4hul04 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this code, DDL event triggers won't fire for ALTER VIEW operations. Currently we do not support DDL triggers for T-SQL, but tested it out from from PSQL endpoint. This code can be used later, once we support DDL triggers in T-SQL. Added comment in the code.


/* Get the old view's OID and verify it exists */
oldViewOid = RangeVarGetRelid(stmt->view, AccessExclusiveLock, true);

/* If the view does not exist, check if the stmt is CREATE OR ALTER VIEW / ALTER VIEW */
if (!OidIsValid(oldViewOid))
{
if(stmt->replace) /* we have set replace to false for CREATE OR ALTER VIEW to avoid view does not exist error */
{
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("view \"%s\" does not exist",
stmt->view->relname)));
}
/* View doesn't exist - create it */
address = DefineView(stmt, queryString, pstmt->stmt_location, pstmt->stmt_len);
CommandCounterIncrement();
}
/* View exists */
else
{
/* Save ACL before dropping the view */
oldViewAcl = get_old_view_acl(oldViewOid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it seems like get_old_view_acl can potentially return NULL. There is even a check for it in pg_class_update_acl. Then it would cause a crash once you hit pfree(oldViewAcl).

So either add a check that get_old_view_acl cannot return NULL - throw an error (then the case in pg_class_update_acl is not needed), or ensure that pfree isn't called on NULL.

Copy link
Contributor Author

@R4hul04 R4hul04 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Older version views may have NULL ACL, added check to pfree(oldViewAcl) only if oldViewAcl is non null

CacheInvalidateRelcacheByRelid(oldViewOid);

/* Drop the old view */
originalView.objectId = oldViewOid;
originalView.classId = RelationRelationId;
originalView.objectSubId = 0;
performDeletion(&originalView, DROP_RESTRICT, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DROP_RESTRICT means this will fail if there's any objects dependent on the view. Could we test this - create an object dependent on a view and attempt to alter the underlying view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to test case

CommandCounterIncrement();

/* Create new view */
stmt->replace = true;
address = DefineView(stmt, queryString, pstmt->stmt_location, pstmt->stmt_len);
CommandCounterIncrement();

/* Store the view definition in babelfish_view_def */
if(store_view_definition_hook)
store_view_definition_hook(queryString, address);

/* Update ACL info */
pg_class_update_acl(address.objectId, oldViewAcl);

if(oldViewAcl != NULL)
pfree(oldViewAcl);
}
CommitTransactionCommand();
}
PG_FINALLY();
{
if (needCleanup)
EventTriggerEndCompleteQuery();
}
PG_END_TRY();
return;
}
/* check that no T-SQL ALTER VIEW operations reach this point because they should have been handled earlier in the code.*/
Assert(!(sql_dialect == SQL_DIALECT_TSQL && stmt->createOrAlter));
break;
}

case T_AlterTableStmt:
{
AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
Expand Down Expand Up @@ -4823,6 +4944,73 @@ pltsql_proc_get_oid_proname_proacl(AlterFunctionStmt *stmt, ParseState *pstate,
*isSameFunc = OidIsValid(funcOid);
}

/*
* Get the acl of view from pg_class given its oid
*/
static Acl *
get_old_view_acl(Oid oldViewOid)
{
HeapTuple tuple;
Datum aclDatum;
bool isNull;
Acl *oldViewAcl = NULL;

tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(oldViewOid));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("cache lookup failed for relation %u", oldViewOid)));

aclDatum = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relacl, &isNull);
if (!isNull)
oldViewAcl = DatumGetAclPCopy(aclDatum);

ReleaseSysCache(tuple);

return oldViewAcl;
}

/*
* Update the acl info of view in pg_class given its oid and acl info
*/
static void
pg_class_update_acl(Oid newViewOid, Acl *oldViewAcl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also add a test that verifies that the acl is properly updated when we do ALTER VIEW.

Copy link
Contributor Author

@R4hul04 R4hul04 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added .mix test file to test ACL changes.

{
Relation pg_class_rel;
HeapTuple classtup;

Datum values[Natts_pg_class];
bool nulls[Natts_pg_class];
bool replaces[Natts_pg_class];
HeapTuple newtup;

/* Get the tuple from syscache */
classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(newViewOid));
if (!HeapTupleIsValid(classtup))
elog(ERROR, "cache lookup failed for relation %u", newViewOid);

pg_class_rel = table_open(RelationRelationId, RowExclusiveLock);

memset(values, 0, sizeof(values));
memset(nulls, false, sizeof(nulls));
memset(replaces, false, sizeof(replaces));

if (oldViewAcl != NULL)
values[Anum_pg_class_relacl - 1] = PointerGetDatum(oldViewAcl);
else
nulls[Anum_pg_class_relacl - 1] = true;
replaces[Anum_pg_class_relacl - 1] = true;

newtup = heap_modify_tuple(classtup, RelationGetDescr(pg_class_rel),
values, nulls, replaces);

CatalogTupleUpdate(pg_class_rel, &newtup->t_self, newtup);

ReleaseSysCache(classtup);
heap_freetuple(newtup);
table_close(pg_class_rel, RowExclusiveLock);
}

/*
* Update the pg_type catalog entry for the given name to have
* typbyval set to the given value.
Expand Down
21 changes: 21 additions & 0 deletions contrib/babelfishpg_tsql/src/tsqlIface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4360,6 +4360,7 @@ storeOriginalQueryForBatchLevelStatement(TSqlParser::Batch_level_statementContex
{
int startIndex = -1;
int endIndex = -1;
int alterIndex = -1;
std::string originalQueryCopy = originalQuery;

if ((ctx->create_or_alter_procedure() && ctx->create_or_alter_procedure()->ALTER()))
Expand All @@ -4376,6 +4377,26 @@ storeOriginalQueryForBatchLevelStatement(TSqlParser::Batch_level_statementContex
originalQueryCopy.replace(startIndex, endIndex - startIndex, "CREATE");
return pstrdup(originalQueryCopy.c_str());
}
/* Replace ALTER VIEW definitions with CREATE VIEW */
else if (ctx->create_or_alter_view() && ctx->create_or_alter_view()->ALTER())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the purpose of these changes?

Copy link
Contributor Author

@R4hul04 R4hul04 Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the alter operation is complete, the definition that gets stored in catalogs is something like Alter view v1 as select 1; instead of Create view v1 as select 1;. This change allows storing correct definitions after ALTER/CREATE OR ALTER VIEW operations. Appropriate tests are added in the test files. SQL server does the same, it stores the definitions as CREATE VIEW instead of ALTER VIEW

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add the comments into the code as well.

Copy link
Contributor Author

@R4hul04 R4hul04 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments in the code

{
startIndex = ctx->create_or_alter_view()->ALTER()->getSymbol()->getStartIndex();
endIndex = startIndex + 5;
/* if the statement is "ALTER VIEW" */
if (!ctx->create_or_alter_view()->CREATE())
{
originalQueryCopy.replace(startIndex, endIndex - startIndex, "CREATE");
}
/* if the statement is "CREATE OR ALTER VIEW" */
else
{
startIndex = ctx->create_or_alter_view()->CREATE()->getSymbol()->getStartIndex();
alterIndex = ctx->create_or_alter_view()->ALTER()->getSymbol()->getStartIndex();
endIndex = alterIndex + 5;
originalQueryCopy.replace(startIndex, endIndex - startIndex, "CREATE");
}
return pstrdup(originalQueryCopy.c_str());
}
else
return pstrdup(originalQueryCopy.c_str());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -485,9 +485,6 @@ antlrcpp::Any TsqlUnsupportedFeatureHandlerImpl::visitCreate_or_alter_trigger(TS

antlrcpp::Any TsqlUnsupportedFeatureHandlerImpl::visitCreate_or_alter_view(TSqlParser::Create_or_alter_viewContext *ctx)
{
if (ctx->ALTER())
handle(INSTR_UNSUPPORTED_TSQL_ALTER_VIEW, "ALTER VIEW", getLineAndPos(ctx->ALTER()));

/* escape hatch of SCHEMABINDING option*/
if (escape_hatch_schemabinding_view != EH_IGNORE)
{
Expand Down
8 changes: 0 additions & 8 deletions test/JDBC/expected/BABEL-UNSUPPORTED.out
Original file line number Diff line number Diff line change
Expand Up @@ -2414,16 +2414,8 @@ CREATE VIEW v_babel_2017 AS SELECT * FROM t_babel_2017;
GO
ALTER VIEW v_babel_2017 AS SELECT a FROM t_babel_2017;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: 'ALTER VIEW' is not currently supported in Babelfish)~~

CREATE OR ALTER VIEW v_babel_2017 AS SELECT b FROM t_babel_2017;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: 'ALTER VIEW' is not currently supported in Babelfish)~~

DROP VIEW v_babel_2017;
GO
DROP TABLE t_babel_2017;
Expand Down
Loading
Loading