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 NCLOB column type #563

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

intgr
Copy link

@intgr intgr commented Dec 23, 2022

This uses the TO_CLOB() Oracle function to convert NCLOB values into CLOB values.

Doc: https://docs.oracle.com/database/121/SQLRF/functions218.htm

Do you see any downsides to doing this?

Only tested with SELECT & VACUUM.

The comment in the NCLOB handling code previously stated:

 * We don't support NCLOB because Oracle cannot
 * transform it to the client character set automatically.

I have not verified whether this trick solves the character set conversion issue.

This uses the `TO_CLOB()` Oracle function to convert NCLOB values into CLOB values.

Doc: https://docs.oracle.com/database/121/SQLRF/functions218.htm
@laurenz
Copy link
Owner

laurenz commented Dec 23, 2022

Thanks for the initiative!
I think that this is missing write support. A regression test is also necessary, but I could help with that.

@intgr intgr force-pushed the add-nclob-type-support branch from 0e1bdd4 to 2a88233 Compare December 23, 2022 11:42
@intgr
Copy link
Author

intgr commented Dec 30, 2022

Added tests. Inserting seems to work fine using the same logic as CLOB type.

Or are there any gotchas that didn't occur to me?

expected/oracle_fdw.out Outdated Show resolved Hide resolved
@intgr intgr force-pushed the add-nclob-type-support branch from d19047d to 8bd1a2c Compare December 30, 2022 14:23
oracle_fdw.c Outdated
@@ -2855,6 +2856,8 @@ char
else if (fdwState->oraTable->cols[i]->oratype == ORA_TYPE_TIMESTAMPLTZ)
/* convert TIMESTAMP WITH LOCAL TIME ZONE to TIMESTAMP WITH TIME ZONE */
format = "%s(%s%s AT TIME ZONE sessiontimezone)";
else if (fdwState->oraTable->cols[i]->oratype == ORA_TYPE_NCLOB)
format = "%s(TO_CLOB(%s%s))";
Copy link
Author

Choose a reason for hiding this comment

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

Seems like the parenthesis around TO_CLOB() aren't really needed, I will get rid of them.

@laurenz
Copy link
Owner

laurenz commented Dec 30, 2022

Thanks! I'll take a close look at that when I come back from vacation on Jan 9.

Copy link
Owner

@laurenz laurenz left a comment

Choose a reason for hiding this comment

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

Your patch looks fine.
I am somewhat surprised that data modifications work without any further ado.

Since you would like me to pull your change request, would you accommodate me in these things:

  • please provide a single commit with all your changes, rebased to current git HEAD

  • please update the documentation: remove the note that NCLOB is not supported and add a line to the conversion table

If you feel like it, you can also create a CHANGELOG entry. Otherwise, I will add that in a separate commit.

@@ -5057,7 +5063,8 @@ checkDataType(oraType oratype, int scale, Oid pgtype, const char *tablename, con

/* VARCHAR2 and CLOB can be converted to json */
Copy link
Owner

Choose a reason for hiding this comment

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

That comment should be updated.

@laurenz
Copy link
Owner

laurenz commented Apr 11, 2023

Are you going to send an updated patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants