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

Try convert #10

Draft
wants to merge 16 commits into
base: OPENGPDB_STABLE
Choose a base branch
from
Draft

Conversation

robozmey
Copy link
Contributor

@robozmey robozmey commented Nov 12, 2024

TRY_CONVERT

We want function what casts value to type or returns NULL.

In this PR there are TRY_CONVERT as extension

Uses "soft" error handling (ereturn & safe calls) from #22

res = OidFunctionCall3Safe(funcId, value, typmod, true, &escontext);

if (escontext.error_occurred) {
*is_failed = true;
}

Need to rewrite some cast & in/out functions to ereturn() instead of ereport()

if (arg1 < SHRT_MIN || arg1 > SHRT_MAX)
		ereturn(fcinfo->context, 0,
				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
				 errmsg("smallint out of range")));

Beside we can catch erros by PG_TRY(); using #define USE_PG_TRY_CATCH

Type Soft handle PG_TRY handle
numric(int, float, numeric) ⬜️
time/date(time(tz),timestamp(tz),date, interval,abstime,reltime) ⬜️
character(text,char,varchar) ⬜️
json, jsonb ⬜️
xml ⬜️ ⬜️
complex ⬜️ ⬜️
point ⬜️
money ⬜️
network(inet,cidr,macaddr) ⬜️
bit string(bit, varbit) ⬜️
regtype ⬜️
uuid ⬜️
Cast features Supports?
pg_cast
cast via IO
array ⬜️
typmod
domain ⬜️

! No domain convert (source_type ---> base_type -X-> target_type)

! REPLACE FlushErrorState()

Due polymorphism have strange signature (some_type, anyelement) -> anyelement

Second parameter is DEFAULT_VALUE::TARGET_TYPE

TRY_CONVERT('42'::text, NULL::int2) -- returns 42::int2
TRY_CONVERT('42d'::text, NULL::int2) -- returns NULL::int2
TRY_CONVERT('42d'::text, 1234::int2) -- returns 1234::int2

We cannot have (any1, any2) -> any2 signature with different anyelement types, but we can create multiple (_, any) -> any functions in contrib/try_convert/try_convert--1.0.sql or use select add_type_for_try_convert('date'::regtype); to add types.

In init we can add all supported types, but with add_type_for_try_convert user can add whatever type he wants

SQL implementation of TRY_CONVERT

Standard implementation of try_convert on psql. Works only when have IO functions.

T1 -> text -> T1 -> T2

pgbench says sql version is 30x slower then standard CAST

!!! does not understand 'typmod's (char and char(10) are same)

CREATE OR REPLACE FUNCTION try_convert_by_sql(_in text, INOUT _out ANYELEMENT, source_type text default 'text')
  LANGUAGE plpgsql AS
$func$
BEGIN
   EXECUTE format('SELECT (%L::%s)::%s', $1, source_type, pg_typeof(_out))
   INTO  _out;
EXCEPTION WHEN others THEN
   -- do nothing: _out already carries default
END
$func$;

TESTS

!!! ADD Test what out try_convert_by_sql is works correctly

contrib/try_convert/generate_tests.py

Test category Supports?
pg_cast
to text
from text
from corrupted text ⬜️
with defaults
1 million via func
1 million via IO
1 million via func with errors
1 million via IO with errors
nested casts
nested casts with errors ⬜️(?)

BENCHMARK

Here are some reminders before you submit the pull request

  • Add tests for the change
  • Document changes
  • Communicate in the mailing list if needed
  • Pass make installcheck
  • Review a PR in return to support the community

@leborchuk leborchuk requested a review from reshke December 5, 2024 09:38
Copy link
Contributor

@reshke reshke Dec 5, 2024

Choose a reason for hiding this comment

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

remove generated data from git

Copy link
Contributor

Choose a reason for hiding this comment

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

ABI change is not allowed without pg_upgrade

@@ -327,6 +327,7 @@ typedef struct TypeCast
NodeTag type;
Node *arg; /* the expression being casted */
TypeName *typeName; /* the target type */
bool is_trycast; /* TODO */
Copy link
Contributor

Choose a reason for hiding this comment

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

This ABI change may lead to numerous failures, both instant(when gpstart) and deferred (executing query with view).
You need to reuse existing fields within TypeCast struct.
One option is to prepend typeName with __opengpdb_trycast__ prefix

@reshke
Copy link
Contributor

reshke commented Dec 5, 2024

This design is highly dubious.
I would like a simpler approach, without try/catch and plan interceptions at all:

  1. Take InputFunctionCallSafe from pg https://git.postgresql.org/cgit/postgresql.git/commit/?id=d9f7f5d32f201bec61fef8104aafcb77cecb4dcb
  2. call it withing our try_convert function.
  3. p.2 means no syntax changes, wee can define try_convert in extension, and then CREATE EXTENSION.

@robozmey robozmey force-pushed the try_convert branch 6 times, most recently from 0dfb55a to 86d18ff Compare December 12, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants