Support customized alignment and storage for pg_tle create type API #266

lyupan · 2024-02-07T20:24:22Z

Issue #263.

In the previous version, we used default int4 alignment and plain storage (non-TOASTable). The data will always be stored in-line and not compressed. An error will be reported if the value exceeds the limit:
ERROR: row is too big: size xxx, maximum size xxx

With this change, users can customize the alignment and storage strategies. Compression or out-of-line storage can be used depending on the storage strategy (https://www.postgresql.org/docs/current/storage-toast.html).

The default alignment is int4 while the default storage is plain, compatible with previous versions.

A new version 1.3.5 is created along with this change.

In the previous version, we used default int4 alignment and plain storage (non-TOASTable). The data will always be stored in-line and not compressed. An error will be reported if the value exceeds the limit: ERROR: row is too big: size xxx, maximum size xxx. With this change, users can customize the alignment and storage strategies. Compression or out-of-line storage can be used depending on the storage strategy (https://www.postgresql.org/docs/current/storage-toast.html). The default alignment is int4 while the default storage is plain, compatible with previous versions. aws#263

…rove readability. When the dabase gets upgraded to a new version(solib is updated) but pg_tle version is not, the old 5-arg version SQL function is invoked by users, but the previous C function always 7 args. In this chagne, a new pg_tle_create_base_type_7args C function is created and used to create the create_base_type SQL function.

adamguo0 · 2024-02-15T16:16:07Z

src/datatype.c

+ */
+PG_FUNCTION_INFO_V1(pg_tle_create_base_type_7args);
+Datum
+pg_tle_create_base_type_7args(PG_FUNCTION_ARGS)


nit: is adding 7args to the name standard practice? Maybe pg_tle_create_base_type_with_storage would be more descriptive

I'm not sure if this is the standard practice, but I have seen this naming style in postgres source code, for example
gin_extract_tsquery_5args(PG_FUNCTION_ARGS) https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/tsginidx.c#L318

I'm not a fan of this naming scheme. While there's precedent in upstream, we don't have to mimic it from a user-exposed interface.

+1 to @adamguo0 suggestion: pg_tle_create_base_type_with_storage, or maybe pg_tle_create_base_type_with_storage_type

Also - to expose this, would we be able to rewrite the existing function to support 7 arguments, and have defaults fo rthe ones that are not necessarily set?

Updated function name to pg_tle_create_base_type_with_storage.

The C function pg_tle_create_base_type_7args/with_storage is not exposed to the users, only pg_tle_create_base_type function (used in the previous 1.3.4 version) is exposed to the user.

v1.3.4: create function pg_tle.create_base_type() .... AS 'pg_tle_create_base_type' v1.4.0+: create function pg_tle.create_base_type() .... AS 'pg_tle_create_base_type_with_storage'

Adding an internal pg_tle_create_base_type_with_storage C function (without modifying the exist one) makes it cleaner and easier to maintain.

Background: in my first commit, I modified the pg_tle_create_base_type C function to add these 2 arguments, however, it has a hidden bug for upgrade (See commit message in f56985b), this was discovered by Josh Tiefenbach and I made this update based on the suggestion, we think this approach creates less confusion and maintenance issues.

adamguo0 · 2024-02-15T16:42:18Z

Makefile

@@ -1,13 +1,13 @@
 EXTENSION = pg_tle
-EXTVERSION = 1.3.4
+EXTVERSION = 1.3.5


Let's get some opinions on whether to bump the version to 1.3.5 or 1.4.0. My initial thought was 1.3.5 since this is a small update to an existing feature, but it also introduces an API change which may warrant a minor version bump.

If we go strictly by semantic versioning, a new, backwards compatible change would require a minor version bump (1.4.0)

Thanks for the suggestion. Updated the new version number to 1.4.0

anth0nyleung · 2024-02-22T16:32:51Z

docs/09_datatypes.md

@@ -63,6 +63,8 @@ SELECT pgtle.create_shell_type_if_not_exists('public', 'test_citext');
 * `infunc`: The name of a previously defined function to convert the type's external textual representation to the internal representation (`bytea`). The function must take one argument of type `text` and return `bytea`. The function must also be declared as `IMMUTABLE` and `STRICT`. It will not be called with a NULL parameter.
 * `outfunc`: The name of a previously defined function to convert the type's internal binary representation (`bytea`) to the external textual representation. The function must take one argument of type `bytea` and return `text`. The function must also be declared as `IMMUTABLE` and `STRICT`. It will not be called with a NULL parameter.
 * `internallength`: Total length of the base data type as bytes. Base data types can be fixed-length, in which case internallength is a positive integer, or variable-length, in which case internallength is -1.
+* `alignment`: The optional alignment parameter specifies the storage alignment requirement of the data type. Allowed values are 'char', 'int2', 'int4', 'double'. The allowed values equate to alignment on 1, 2, 4, or 8 byte boundaries. The default is 'int4', alignment on 4 byte boundaries. Note that variable-length types must have an alignment of at least 4, since they necessarily contain an int4 as their first component.


Just a sanity check, are the allowed values the same for all major version we support?

I'd recommend giving more guidance on how this is used. Most builders are not going to be familiar with byte alignment.

Note that variable-length types must have an alignment of at least 4, since they necessarily contain an int4 as their first component.

I'd recommend rephrasing this:

A variable-length type (e.g., text) must have a byte-alignment of at least 4 due to the size of its header.

Just a sanity check, are the allowed values the same for all major version we support?

Yes, this didn't change at least since PG11, https://www.postgresql.org/docs/11/sql-createtype.html

alignment The storage alignment requirement of the data type. If specified, it must be char, int2, int4, or double; the default is int4. storage The storage strategy for the data type. If specified, must be plain, external, extended, or main; the default is plain.

anth0nyleung · 2024-02-22T16:59:27Z

pg_tle--1.3.4--1.3.5.sql

+DROP FUNCTION pgtle.create_base_type;
+DROP FUNCTION pgtle.create_base_type_if_not_exists;
+


Do we need CASCADE to drop the GRANT we did here https://github.com/aws/pg_tle/blob/93e330cb146c4055a983b38a47d73df6ac924b33/pg_tle--1.1.1.sql#L555C1-L571 for the old versions?

Good catch, updated!

anth0nyleung · 2024-02-22T17:01:13Z

could you also test upgrading to this version works and share the results?

jkatz · 2024-02-22T22:33:55Z

Makefile

@@ -1,13 +1,13 @@
 EXTENSION = pg_tle
-EXTVERSION = 1.3.4
+EXTVERSION = 1.3.5


If we go strictly by semantic versioning, a new, backwards compatible change would require a minor version bump (1.4.0)

docs/09_datatypes.md

jkatz · 2024-02-22T22:37:46Z

docs/09_datatypes.md

@@ -63,6 +63,8 @@ SELECT pgtle.create_shell_type_if_not_exists('public', 'test_citext');
 * `infunc`: The name of a previously defined function to convert the type's external textual representation to the internal representation (`bytea`). The function must take one argument of type `text` and return `bytea`. The function must also be declared as `IMMUTABLE` and `STRICT`. It will not be called with a NULL parameter.
 * `outfunc`: The name of a previously defined function to convert the type's internal binary representation (`bytea`) to the external textual representation. The function must take one argument of type `bytea` and return `text`. The function must also be declared as `IMMUTABLE` and `STRICT`. It will not be called with a NULL parameter.
 * `internallength`: Total length of the base data type as bytes. Base data types can be fixed-length, in which case internallength is a positive integer, or variable-length, in which case internallength is -1.
+* `alignment`: The optional alignment parameter specifies the storage alignment requirement of the data type. Allowed values are 'char', 'int2', 'int4', 'double'. The allowed values equate to alignment on 1, 2, 4, or 8 byte boundaries. The default is 'int4', alignment on 4 byte boundaries. Note that variable-length types must have an alignment of at least 4, since they necessarily contain an int4 as their first component.


I'd recommend giving more guidance on how this is used. Most builders are not going to be familiar with byte alignment.

Note that variable-length types must have an alignment of at least 4, since they necessarily contain an int4 as their first component.

I'd recommend rephrasing this:

A variable-length type (e.g., text) must have a byte-alignment of at least 4 due to the size of its header.

docs/09_datatypes.md

jkatz · 2024-02-22T22:42:31Z

src/datatype.c

+ */
+PG_FUNCTION_INFO_V1(pg_tle_create_base_type_7args);
+Datum
+pg_tle_create_base_type_7args(PG_FUNCTION_ARGS)


I'm not a fan of this naming scheme. While there's precedent in upstream, we don't have to mimic it from a user-exposed interface.

+1 to @adamguo0 suggestion: pg_tle_create_base_type_with_storage, or maybe pg_tle_create_base_type_with_storage_type

jkatz · 2024-02-22T22:43:01Z

src/datatype.c

+ */
+PG_FUNCTION_INFO_V1(pg_tle_create_base_type_7args);
+Datum
+pg_tle_create_base_type_7args(PG_FUNCTION_ARGS)


Also - to expose this, would we be able to rewrite the existing function to support 7 arguments, and have defaults fo rthe ones that are not necessarily set?

jkatz · 2024-02-22T22:43:36Z

src/datatype.c

+	if (pg_strcasecmp(alignmentStr, "double") == 0)
+		alignment = TYPALIGN_DOUBLE;
+	else if (pg_strcasecmp(alignmentStr, "int4") == 0)
+		alignment = TYPALIGN_INT;
+	else if (pg_strcasecmp(alignmentStr, "int2") == 0)
+		alignment = TYPALIGN_SHORT;
+	else if (pg_strcasecmp(alignmentStr, "char") == 0)
+		alignment = TYPALIGN_CHAR;
+	else
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("alignment \"%s\" not recognized", alignmentStr)));


Suggestion: put this lookup into its own function.

Thanks for the suggestion, updated!

jkatz · 2024-02-22T22:43:42Z

src/datatype.c

+	if (pg_strcasecmp(storageStr, "plain") == 0)
+		storage = TYPSTORAGE_PLAIN;
+	else if (pg_strcasecmp(storageStr, "external") == 0)
+		storage = TYPSTORAGE_EXTERNAL;
+	else if (pg_strcasecmp(storageStr, "extended") == 0)
+		storage = TYPSTORAGE_EXTENDED;
+	else if (pg_strcasecmp(storageStr, "main") == 0)
+		storage = TYPSTORAGE_MAIN;
+	else
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("storage \"%s\" not recognized", storageStr)));


Suggestion: put this lookup into its own function.

lyupan · 2024-02-23T19:00:55Z

could you also test upgrading to this version works and share the results?

Yes, tested locally. See results below. (This PR only changes the API to allow extra type options, it doesn't change the type storage/alignment if already defined)

Create pg_tle with old version library and version

postgres=# create extension pg_tle;
CREATE EXTENSION
postgres=# \dx
                        List of installed extensions
  Name   | Version |   Schema   |                Description                 
---------+---------+------------+--------------------------------------------
 pg_tle  | 1.3.4   | pgtle      | Trusted Language Extensions for PostgreSQL
 plpgsql | 1.0     | pg_catalog | PL/pgSQL procedural language
(2 rows)

Create custom types and data

...
-- Variable-length
postgres=# SELECT pgtle.create_base_type('public', 'test_citext', 'test_citext_in(text)'::regprocedure, 'test_citext_out(bytea)'::regprocedure, -1);
 create_base_type 
------------------
 
(1 row)

-- fixed length
postgres=# SELECT pgtle.create_base_type('public', 'test_int2', 'test_int2_in(text)'::regprocedure, 'test_int2_out(bytea)'::regprocedure, 2);
 create_base_type 
------------------
 
(1 row)
postgres=# INSERT INTO test_dt_int2 VALUES ('11,22');
INSERT 0 1
postgres=# SELECT * FROM test_dt_int2;
  c1   
-------
 11,22
(1 row)

postgres=# INSERT INTO test_dt VALUES ('SELECT'), ('INSERT'), ('UPDATE'), ('DELETE');
INSERT 0 4
postgres=# select * from test_dt;
   c1   
--------
 SELECT
 INSERT
 UPDATE
 DELETE
(4 rows)

Install the new extension library (1.4.0), restart postgres and upgrade

postgres=# ALTER EXTENSION pg_tle UPDATE;
ALTER EXTENSION
postgres=# \dx
                        List of installed extensions
  Name   | Version |   Schema   |                Description                 
---------+---------+------------+--------------------------------------------
 pg_tle  | 1.4.0   | pgtle      | Trusted Language Extensions for PostgreSQL
 plpgsql | 1.0     | pg_catalog | PL/pgSQL procedural language
(2 rows)

Check the old types (tables) can still read/write

postgres=# select * from test_dt;
   c1   
--------
 SELECT
 INSERT
 UPDATE
 DELETE
(4 rows)

postgres=# select * from test_dt_int2;
  c1   
-------
 11,22
(1 row)

postgres=# delete from test_dt;
DELETE 4
postgres=# delete from test_dt_int2;
DELETE 1
postgres=# INSERT INTO test_dt VALUES ('SELECT'), ('INSERT'), ('UPDATE'), ('DELETE');
INSERT 0 4
postgres=# INSERT INTO test_dt_int2 VALUES ('11,22');
INSERT 0 1
postgres=# select * from test_dt;
   c1   
--------
 SELECT
 INSERT
 UPDATE
 DELETE
(4 rows)

postgres=# select * from test_dt_int2;
  c1   
-------
 11,22
(1 row)

jkatz

LGTM, a couple of minor comments. Nice job!

docs/09_datatypes.md

jkatz · 2024-02-27T20:16:59Z

src/datatype.c

+static Datum
+			pg_tle_create_base_type_internal(Oid typeNamespace,
+											 char *typeName,
+											 Oid inputFuncId,
+											 Oid outputFuncId,
+											 int16 internalLength,
+											 char *alignmentStr,
+											 char *storageStr,
+											 char *funcProbin);


Was this the output from pgindent?

Yes, it is.

src/datatype.c

Co-authored-by: Jonathan S. Katz <jkatz@users.noreply.github.com>

lyupan · 2024-02-27T21:20:06Z

Thank you all for the review!

Lyu Pan added 5 commits February 7, 2024 19:25

Small indentation update.

4fb7e55

Fix indentation error.

3a40ce3

Remove unnecessary GRANT pg_read_server_files xxx from datatype tests.

3207751

Add alignment and storage parameters in datatypes doc.

67358b2

lyupan marked this pull request as draft February 8, 2024 00:58

Lyu Pan added 2 commits February 9, 2024 00:05

Update TOAST type tests.

d70052d

Fix datatype test failures in PG11.

1f64419

lyupan marked this pull request as ready for review February 9, 2024 00:26

Lyu Pan added 2 commits February 14, 2024 21:52

Fix build errors

7c224ff

adamguo0 reviewed Feb 15, 2024

View reviewed changes

lyupan mentioned this pull request Feb 20, 2024

size limitation on custom type. #263

Closed

anth0nyleung reviewed Feb 22, 2024

View reviewed changes

jkatz reviewed Feb 22, 2024

View reviewed changes

Use minor version number of 1.4.0 instead of 1.3.5

9e921ce

lyupan requested review from jkatz, adamguo0 and anth0nyleung February 23, 2024 19:09

anth0nyleung approved these changes Feb 26, 2024

View reviewed changes

adamguo0 approved these changes Feb 26, 2024

View reviewed changes

jkatz approved these changes Feb 27, 2024

View reviewed changes

Update src/datatype.c

09d10a9

Co-authored-by: Jonathan S. Katz <jkatz@users.noreply.github.com>

adamguo0 merged commit 4e18a50 into aws:main Feb 27, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support customized alignment and storage for pg_tle create type API #266

Support customized alignment and storage for pg_tle create type API #266

lyupan commented Feb 7, 2024 •

edited

Loading

adamguo0 Feb 15, 2024

lyupan Feb 20, 2024

jkatz Feb 22, 2024

jkatz Feb 22, 2024

lyupan Feb 23, 2024 •

edited

Loading

adamguo0 Feb 15, 2024

jkatz Feb 22, 2024

lyupan Feb 23, 2024

anth0nyleung Feb 22, 2024 •

edited

Loading

jkatz Feb 22, 2024

lyupan Feb 23, 2024 •

edited

Loading

anth0nyleung Feb 22, 2024

lyupan Feb 23, 2024

anth0nyleung commented Feb 22, 2024

jkatz Feb 22, 2024

jkatz Feb 22, 2024

jkatz Feb 22, 2024

jkatz Feb 22, 2024

jkatz Feb 22, 2024

lyupan Feb 23, 2024

jkatz Feb 22, 2024

lyupan Feb 23, 2024

lyupan commented Feb 23, 2024 •

edited

Loading

jkatz left a comment

jkatz Feb 27, 2024

lyupan Feb 27, 2024

lyupan commented Feb 27, 2024

		DROP FUNCTION pgtle.create_base_type;
		DROP FUNCTION pgtle.create_base_type_if_not_exists;

Support customized alignment and storage for pg_tle create type API #266

Support customized alignment and storage for pg_tle create type API #266

Conversation

lyupan commented Feb 7, 2024 • 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

lyupan Feb 23, 2024 • 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

anth0nyleung Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyupan Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anth0nyleung commented Feb 22, 2024

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

lyupan commented Feb 23, 2024 • edited Loading

jkatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyupan commented Feb 27, 2024

lyupan commented Feb 7, 2024 •

edited

Loading

lyupan Feb 23, 2024 •

edited

Loading

anth0nyleung Feb 22, 2024 •

edited

Loading

lyupan Feb 23, 2024 •

edited

Loading

lyupan commented Feb 23, 2024 •

edited

Loading