-
Notifications
You must be signed in to change notification settings - Fork 23
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
PG-857: use common JSON API for keyring #252
Conversation
6728b98
to
0fbca19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks working, and okay, but...
My main issue with this pull request is that now instead of a nice, simple and easy to understand JSON code we end up with a long and complex thing that works, but it is difficult to understand and/or modify.
I already had this issue with the previous JSON refactoring where we removed jsonc, because that already made the code more complex, but now it is even more so.
t/004_file_config.pl
Outdated
@@ -23,10 +23,6 @@ | |||
print $conf "shared_preload_libraries = 'pg_tde'\n"; | |||
close $conf; | |||
|
|||
open my $conf2, '>>', "/tmp/datafile-location"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I think the issue here is that "file" remote support is still missing from the code, plese read the documentation: https://percona-lab.github.io/pg_tde/main/external-parameters.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote is supported but with URL only and not path. It didn't make sense to me at first and I thought this is some leftovers (was about to clarify in it).
I'll do the "remote path" but the question is should the 'type' parameter be like in tests file
or like in docs remote
?
test L33:
json_object( 'type' VALUE 'file', 'path' VALUE '/tmp/datafile-location' )
docs:
json_object( 'type' VALUE 'remote', 'path' VALUE '/tmp/datafile-location' )
To the comment of complexity - I agree, but we don't have much choice for this code to be used with frontend tools. The other way would be get back to json-c, but it's an external dependency... |
02b520e
to
8ddbf68
Compare
This commit replaces dependency of the keyring code on JSON backend functions with common JSON API. Usage of the backend JSON funcs prevents the code to be used by frontend tools like pg_waldump. Besides it requiers extra conversions to Datums and DirectFunctionCall-s.
d58ee1b
to
bd85b4f
Compare
This commit replaces dependency of the keyring code on JSON backend functions with common JSON API.
Usage of the backend JSON funcs prevents the code to be used by frontend tools like pg_waldump. Besides it requiers extra conversions to Datums and DirectFunctionCall-s.
For: https://perconadev.atlassian.net/browse/PG-857