Skip to content

Relax extension_update_paths argument to text#277

Merged
adamguo0 merged 4 commits intoaws:mainfrom
Rajanpandey:name
May 29, 2024
Merged

Relax extension_update_paths argument to text#277
adamguo0 merged 4 commits intoaws:mainfrom
Rajanpandey:name

Conversation

@Rajanpandey
Copy link
Copy Markdown
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.
Apply pg_indent

Co-authored-by: Adam Guo <adamguo@amazon.com>
@adamguo0
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Thanks @adamguo0, fixed it! 😄

@adamguo0
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@Rajanpandey
Copy link
Copy Markdown
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
Copy Markdown
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
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