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

Relax extension_update_paths argument to text #277

Merged
merged 4 commits into from
May 29, 2024
Merged

Conversation

Rajanpandey
Copy link
Contributor

@Rajanpandey Rajanpandey commented May 9, 2024

Issue #, if available: #126

Description of changes:
All of the other extension functions use text as name, so this appears to be an arbitrary restriction. Hence, relaxing the argument from name to text.

The name data (currently in extension_update_paths()) can hold 63 characters only (ref #define NAMEDATALEN 64). Whereas text data type (updated in this PR) has no limit at all.

Note that install_extension() already uses text instead of name data type for the extension-name's argument. So a user can create an extension with names that are >63 characters, but the operation will fail when trying to find its update paths using extension_update_paths() function because it's using a data type that limits the extension name to 63 chars!

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

All of the other extension functions use text as name, so this appears to be an arbitrary restriction.
Hence, relaxing the argument from name to text.
src/tleextension.c Outdated Show resolved Hide resolved
Apply pg_indent

Co-authored-by: Adam Guo <adamguo@amazon.com>
@adamguo0
Copy link
Contributor

adamguo0 commented May 9, 2024

To update the extension version you'll need to bump EXTVERSION in the Makefile as well. Not sure if that's causing these regression test failures though

All of the other extension functions use text as name, so this appears to be an arbitrary restriction.
Hence, relaxing the argument from name to text.
@Rajanpandey
Copy link
Contributor Author

Thanks @adamguo0, fixed it! 😄

@adamguo0
Copy link
Contributor

Sorry I haven't gotten to this in a while -- the change looks good to me. I did a manual sanity check to make sure that after updating pg_tle from 1.4.0 to 1.4.1, the function works as expected

@anth0nyleung
Copy link
Contributor

Hi, do you mind elaborating what are the restrictions that you're facing before this change?

@Rajanpandey
Copy link
Contributor Author

Hi @anth0nyleung 😃,
The name data type can hold 63 characters string only (ref #define NAMEDATALEN 64). Whereas text has no limit at all!

Note that install_extension already uses text instead of name data type for the extension-name's argument. So a user can create an extension with names that are >63 characters, but the operation will fail when trying to find its update paths using extension_update_paths() function because it's using a data type that limits the extension name to 63 chars!

This PR addresses a long time pending issue, #126 raised by @jkatz. 😄

@adamguo0
Copy link
Contributor

adamguo0 commented May 29, 2024

As a side-note there's another issue preventing users from creating extensions with almost-63 chars. Since function names are capped at 63 chars, I don't think our current implementation can handle this case. We'll need to address that separately

#100

@adamguo0 adamguo0 merged commit 4e8e0f4 into aws:main May 29, 2024
7 checks passed
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.

3 participants