Implement pgtle.create_shell_type function to support shell type creation in pg_tle #205

lyupan · 2023-06-08T20:38:35Z

Description of changes:
Implement pgtle.create_shell_type/create_shell_type_if_not_exists function:

They require two arguments: namespace and type name and can only be executed by pgtle_admin and superuser;
Similar to https://github.com/aws/pg_tle/blob/main/docs/03_managing_extensions.md#pgtleregister_feature_if_not_existsproc-regproc-feature-pg_tle_features, create_shell_type_if_not_exists returns false if the type already exists while create_shell_type fails;
Add a new pg_tle version 1.0.5 for the new datatype functions.

This is the first commit to support custom base types in pg_tle.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…tion in pg_tle. Co-authored-by: Jim Mlodgenski <jimmy76@gmail.com>

formalLogicGirl · 2023-06-09T14:35:41Z

pg_tle--1.0.1--1.0.4.sql

@@ -107,3 +107,26 @@ GRANT EXECUTE ON FUNCTION pgtle.install_extension_version_sql
  version text,
  ext text
 ) TO pgtle_admin;
+


We should add a new minor version, instead of modifying 1.0.4

Done. Added a new version 1.0.5

formalLogicGirl · 2023-06-09T14:38:08Z

src/datatype.c

+	tleadminoid = roleform->oid;
+	ReleaseSysCache(tuple);
+
+    CHECK_CAN_SET_ROLE(GetUserId(), tleadminoid);


nit : can we make the whitespace formatting consistent

We may need a pgformatter run -- it's available in the PostgreSQL repo.

Done, run pg_indent.

formalLogicGirl · 2023-06-09T14:42:51Z

test/expected/pg_tle_datatype.out

+SELECT CURRENT_USER;
+ current_user 
+--------------
+ lyup


this will not match for other user aliases running the test and fail the test, right?

Ah, this was a mistake. Fixed.

jkatz · 2023-06-09T15:45:01Z

src/datatype.c

+	Form_pg_authid roleform;
+	Oid			tleadminoid;
+
+	tuple = SearchSysCache1(AUTHNAME, CStringGetDatum("pgtle_admin"));


pgtle_admin should be defined in a constant. Unsure if it should be in constants.h or tleextension.h -- leaning towards tleextension.h as it contains names specific to the TLE extension itself, though maybe those need to be moved to constants.h

Defined a new constant PG_TLE_ADMIN.

I think you can simplify this by just calling get_role_oid and don't have to re-implement

Thanks for the suggestion! Fixed.

jkatz · 2023-06-09T15:46:55Z

src/datatype.c

+	address = TypeShellMake(typeName, typeNamespace, GetUserId());
+
+	/*
+	 * Make effects of commands visible
+	 */
+	CommandCounterIncrement();
+
+	if (OidIsValid(address.objectId))
+		PG_RETURN_BOOL(true);
+
+	PG_RETURN_BOOL(false);


I would opt to return an error if we're unable to create the shell type. That way, if this is in a transaction, it will fail.

Yeah, that makes sense. Updated the code.

jkatz · 2023-06-09T15:49:32Z

src/datatype.c

+	/*
+	 * Look to see if type already exists
+	 */
+	typoid = GetSysCacheOid2(TYPENAMENSP, Anum_pg_type_oid,
+							 CStringGetDatum(typeName),
+							 ObjectIdGetDatum(typeNamespace));
+
+	if (OidIsValid(typoid))
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_DUPLICATE_OBJECT),
+				 errmsg("type \"%s\" already exists", typeName)));
+	}


See comment below -- we could have a similar pattern to other functions, e.g. create_shell_type which will error if the type already exists, and create_shell_type_if_not_exists which will return false if it exists. Example:

https://github.com/aws/pg_tle/blob/main/docs/03_managing_extensions.md#pgtleregister_feature_if_not_existsproc-regproc-feature-pg_tle_features

Since we are mirroring the Postgres CREATE TYPE wouldn't it be good to keep the behavior similar? In Postgres, if the type already exists then an error is generated. I was thinking we want to do the same here.

Add a new create_shell_type_if_not_exists function:
create_shell_type now fails (report error) if the type already exists;
create_shell_type_if_not_exists returns false if it exists.

OOC when would a user call one over the other?

Thats a great question. Its not clear to me that there is a very compelling use case for create_shell_type_if_not_exists. In Postgres, there isn't a similar function. In pg_tle, by design, we want to prevent leaving behind a shell type when a custom base type created in pg_tle gets successfully dropped. So there is not supposed to be a scenario where a shell type is left behind, and if that were the case, it seems like an error message informing the user would be helpful. Shell types also can't be shared between different custom base types, so no expectation of the shell type having been already created for some other reason. So then it comes down to whether the user wants the create function to not error under any circumstance, in which case they should use create_shell_type_if_not_exists. Is that a significant enough requirement that we need this function for?

In pg_tle, by design, we want to prevent leaving behind a shell type when a custom base type created in pg_tle gets successfully dropped.

When a shell type is created, there's a field typisdefined setting to false indicating it's a shell type; when an actual type is defined, typisdefined will be set to true and the shell type becomes an actual type. When we drop the base type, there won't be any shell type left behind.

create_shell_type_if_not_exists is similar to register_feature_if_not_exists. They return false when the type/feature already exists, which may be useful for clients that don't want to get errors, for example if they don't want to fail the whole transaction.

Not failing the transaction may be the significant use case that justifies the if_not_exists form of this function. Okay.

jkatz

Good start! Please see previous review for the requested changes / comments.

Description of the changes: 1. Add create_shell_type_if_not_exists funcion which returns false if the type already exists; create_shell_type fails if the type already exists. 2. Add a new version 1.0.5 for the datatype functions. Co-authored-by: Jim Mlodgenski <jimmy76@gmail.com>

lyupan · 2023-06-09T20:59:27Z

Thanks for the review! Made some updates based on the comments, please take another look.

Co-authored-by: Jim Mlodgenski <jimmy76@gmail.com>

formalLogicGirl

Looks mostly good to me. I think its good to merge and work on the next functions. We can revisit if there are any updates we still need. I think you've already addressed all the comments raised so far.

lyupan force-pushed the main branch from 3773709 to 220b768 Compare June 8, 2023 21:11

Implement pgtle.create_shell_type function to support shell type crea…

66082db

…tion in pg_tle. Co-authored-by: Jim Mlodgenski <jimmy76@gmail.com>

lyupan force-pushed the main branch from 220b768 to 66082db Compare June 8, 2023 21:36

formalLogicGirl reviewed Jun 9, 2023

View reviewed changes

jkatz approved these changes Jun 9, 2023

View reviewed changes

jkatz self-requested a review June 9, 2023 15:53

jkatz requested changes Jun 9, 2023

View reviewed changes

lyupan requested review from jkatz and formalLogicGirl June 9, 2023 20:57

Use get_role_oid to simplify check_is_pgtle_admin code.

c4e2ced

Co-authored-by: Jim Mlodgenski <jimmy76@gmail.com>

lyupan requested a review from JohnHVancouver June 13, 2023 17:48

formalLogicGirl approved these changes Jun 13, 2023

View reviewed changes

formalLogicGirl merged commit 4ef921e into aws:main Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement pgtle.create_shell_type function to support shell type creation in pg_tle #205

Implement pgtle.create_shell_type function to support shell type creation in pg_tle #205

lyupan commented Jun 8, 2023 •

edited

Loading

formalLogicGirl Jun 9, 2023

lyupan Jun 9, 2023

formalLogicGirl Jun 9, 2023

jkatz Jun 9, 2023

lyupan Jun 9, 2023

formalLogicGirl Jun 9, 2023 •

edited

Loading

lyupan Jun 9, 2023

jkatz Jun 9, 2023

lyupan Jun 9, 2023

JohnHVancouver Jun 9, 2023

lyupan Jun 12, 2023

jkatz Jun 9, 2023

lyupan Jun 9, 2023

jkatz Jun 9, 2023

formalLogicGirl Jun 9, 2023

lyupan Jun 9, 2023

JohnHVancouver Jun 9, 2023

formalLogicGirl Jun 13, 2023

lyupan Jun 13, 2023

formalLogicGirl Jun 13, 2023

jkatz left a comment

lyupan commented Jun 9, 2023

formalLogicGirl left a comment

Implement pgtle.create_shell_type function to support shell type creation in pg_tle #205

Implement pgtle.create_shell_type function to support shell type creation in pg_tle #205

Conversation

lyupan commented Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

formalLogicGirl Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkatz left a comment

Choose a reason for hiding this comment

lyupan commented Jun 9, 2023

formalLogicGirl left a comment

Choose a reason for hiding this comment

lyupan commented Jun 8, 2023 •

edited

Loading

formalLogicGirl Jun 9, 2023 •

edited

Loading