Extend ALTER DEFAULT PRIVILEGES for large objects
Hi,
Currently, ALTER DEFAULT PRIVILEGE doesn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created. One of our clients feels that this is annoying, so I would
like propose to extend ALTER DEFAULT PRIVILEGE to large objects.
Here are the new actions allowed in abbreviated_grant_or_revoke;
+GRANT { { SELECT | UPDATE }
+ [, ...] | ALL [ PRIVILEGES ] }
+ ON LARGE OBJECTS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]+REVOKE [ GRANT OPTION FOR ]
+ { { SELECT | UPDATE }
+ [, ...] | ALL [ PRIVILEGES ] }
+ ON LARGE OBJECTS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]A new keyword OBJECTS is introduced for using plural form in the syntax
as other supported objects. A schema name is not allowed to be specified
for large objects since any large objects don't belong to a schema.
The attached patch is originally proposed by Haruka Takatsuka
and some fixes and tests are made by me.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload+208-8
Yugo NAGATA <nagata@sraoss.co.jp> writes:
Currently, ALTER DEFAULT PRIVILEGE doesn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created. One of our clients feels that this is annoying, so I would
like propose to extend ALTER DEFAULT PRIVILEGE to large objects.
I wonder how this plays with pg_dump, and in particular whether it
breaks the optimizations that a45c78e32 installed for large numbers
of large objects. The added test cases seem to go out of their way
to leave no trace behind that the pg_dump/pg_upgrade tests might
encounter.
I think you broke psql's \ddp, too. And some other places; grepping
for DEFACLOBJ_NAMESPACE finds other oversights.
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.
regards, tom lane
On Tue, 23 Apr 2024 23:47:38 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yugo NAGATA <nagata@sraoss.co.jp> writes:
Currently, ALTER DEFAULT PRIVILEGE doesn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created. One of our clients feels that this is annoying, so I would
like propose to extend ALTER DEFAULT PRIVILEGE to large objects.I wonder how this plays with pg_dump, and in particular whether it
breaks the optimizations that a45c78e32 installed for large numbers
of large objects. The added test cases seem to go out of their way
to leave no trace behind that the pg_dump/pg_upgrade tests might
encounter.
Thank you for your comments.
The previous patch did not work with pg_dump since I forgot some fixes.
I attached a updated patch including fixes.
I believe a45c78e32 is about already-existing large objects and does
not directly related to default privileges, so will not be affected
by this patch.
I think you broke psql's \ddp, too. And some other places; grepping
for DEFACLOBJ_NAMESPACE finds other oversights.
Yes, I did. The attached patch include fixes for psql, too.
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.
I believe this feature is beneficial to some users allows because
this enables to omit GRANT that was necessary every large object
creation. It seems to me that implementation/maintenance cost is not
so high compared to other objects (e.g. default privileges on schemas)
unless I am still missing something wrong.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
v2-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=v2-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload+235-13
On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.
I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0]https://www.postgresql.org/docs/devel/lo-intro.html that might be worth examining.
[0]: https://www.postgresql.org/docs/devel/lo-intro.html
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.
If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.
Apart from that, extending TOAST to support more than 1GB data and
stream-style access seems a good challenge. I don't know if there was a
proposal for this in past. This is just a thought, for this purpose, we
will need a new type of varlena that can contains large size information,
and a new toast table schema that can store offset information or some way
to convert a offset to chunk_seq.
Regards,
Yugo Nagata
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA <nagata@sraoss.co.jp>
On Fri, 26 Apr 2024 at 10:54, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.Apart from that, extending TOAST to support more than 1GB data and
stream-style access seems a good challenge. I don't know if there was a
proposal for this in past. This is just a thought, for this purpose, we
will need a new type of varlena that can contains large size information,
and a new toast table schema that can store offset information or some way
to convert a offset to chunk_seq.
If you're interested in this, you may want to check out [0]/messages/by-id/flat/CAN-LCVMq2X=fhx7KLxfeDyb3P+BXuCkHC0g=9GF+JD4izfVa0Q@mail.gmail.com and [1]/messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com as
threads on the topic of improving TOAST handling of large values ([1]/messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com
being a thread where the limitations of our current external TOAST
pointer became clear once more), and maybe talk with Aleksander
Alekseev and Nikita Malakhov. They've been working closely with
systems that involve toast pointers and their limitations.
The most recent update on the work of Nikita (reworking TOAST
handling) [2]/messages/by-id/CAN-LCVOgMrda9hOdzGkCMdwY6dH0JQa13QvPsqUwY57TEn6jww@mail.gmail.com is that he got started adapting their externally
pluggable toast into type-internal methods only, though I've not yet
noticed any updated patches appear on the list.
As for other issues with creating larger TOAST values:
TOAST has a value limit of ~1GB, which means a single large value (or
two, for that matter) won't break anything in the wire protocol, as
DataRow messages have a message size field of uint32 [^3]. However, if
we're going to allow even larger values to be stored in table's
attributes, we'll have to figure out how we're going to transfer those
larger values to (and from) clients. For large objects, this is much
less of an issue because the IO operations are already chunked by
design, but this may not work well for types that you want to use in
your table's columns.
Kind regards,
Matthias van de Meent
[0]: /messages/by-id/flat/CAN-LCVMq2X=fhx7KLxfeDyb3P+BXuCkHC0g=9GF+JD4izfVa0Q@mail.gmail.com
[1]: /messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com
[2]: /messages/by-id/CAN-LCVOgMrda9hOdzGkCMdwY6dH0JQa13QvPsqUwY57TEn6jww@mail.gmail.com
[^3] Most, if not all PostgreSQL wire protocol messages have this
uint32 message size field, but the DataRow one is relevant here as
it's the one way users get their data out of the database.
On Fri, 26 Apr 2024 12:23:45 +0200
Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
On Fri, 26 Apr 2024 at 10:54, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.Apart from that, extending TOAST to support more than 1GB data and
stream-style access seems a good challenge. I don't know if there was a
proposal for this in past. This is just a thought, for this purpose, we
will need a new type of varlena that can contains large size information,
and a new toast table schema that can store offset information or some way
to convert a offset to chunk_seq.If you're interested in this, you may want to check out [0] and [1] as
threads on the topic of improving TOAST handling of large values ([1]
being a thread where the limitations of our current external TOAST
pointer became clear once more), and maybe talk with Aleksander
Alekseev and Nikita Malakhov. They've been working closely with
systems that involve toast pointers and their limitations.The most recent update on the work of Nikita (reworking TOAST
handling) [2] is that he got started adapting their externally
pluggable toast into type-internal methods only, though I've not yet
noticed any updated patches appear on the list.
Thank you for your information. I'll check the threads you mentioned.
As for other issues with creating larger TOAST values:
TOAST has a value limit of ~1GB, which means a single large value (or
two, for that matter) won't break anything in the wire protocol, as
DataRow messages have a message size field of uint32 [^3]. However, if
we're going to allow even larger values to be stored in table's
attributes, we'll have to figure out how we're going to transfer those
larger values to (and from) clients. For large objects, this is much
less of an issue because the IO operations are already chunked by
design, but this may not work well for types that you want to use in
your table's columns.
I overlooked this issue. I faced the similar issue when I tried to
pg_dump large text values, although the error was raised from
enlargeStringInfo() in that case....
Regards,
Yugo Nagata
Kind regards,
Matthias van de Meent
[0] /messages/by-id/flat/CAN-LCVMq2X=fhx7KLxfeDyb3P+BXuCkHC0g=9GF+JD4izfVa0Q@mail.gmail.com
[1] /messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com
[2] /messages/by-id/CAN-LCVOgMrda9hOdzGkCMdwY6dH0JQa13QvPsqUwY57TEn6jww@mail.gmail.com[^3] Most, if not all PostgreSQL wire protocol messages have this
uint32 message size field, but the DataRow one is relevant here as
it's the one way users get their data out of the database.
--
Yugo NAGATA <nagata@sraoss.co.jp>
On Fri, 26 Apr 2024 17:54:06 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:
On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:
On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.
I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachments:
v3-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=v3-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload+215-12
On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:
I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.
When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.
Attached is a rebased patch.
I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.
So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.
Yours,
Laurenz Albe
Attachments:
v4-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-patch; charset=UTF-8; name=v4-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload+215-12
On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:
I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.Attached is a rebased patch.
Thank you for updating the patch!
I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.
I confirmed the patch and I am fine with it.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
On 2025/01/23 19:22, Yugo NAGATA wrote:
On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:
I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.Attached is a rebased patch.
Thank you for updating the patch!
I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.I confirmed the patch and I am fine with it.
I've started reviewing this patch since it's marked as "ready for committer".
I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.
The latest patch looks good to me. I just have one minor comment:
only the privileges for schemas, tables (including views and foreign
tables), sequences, functions, and types (including domains) can be
altered.
In alter_default_privileges.sgml, this part should also mention large objects?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Wed, 2 Apr 2025 02:35:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2025/01/23 19:22, Yugo NAGATA wrote:
On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:
I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.Attached is a rebased patch.
Thank you for updating the patch!
I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.I confirmed the patch and I am fine with it.
I've started reviewing this patch since it's marked as "ready for committer".
Thank you for your reviewing this patch!
I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.The latest patch looks good to me. I just have one minor comment:
only the privileges for schemas, tables (including views and foreign
tables), sequences, functions, and types (including domains) can be
altered.In alter_default_privileges.sgml, this part should also mention large objects?
Agreed. I attached a updated patch.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
v5-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=v5-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload+217-14
On 2025/04/03 23:04, Yugo NAGATA wrote:
On Wed, 2 Apr 2025 02:35:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:On 2025/01/23 19:22, Yugo NAGATA wrote:
On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:
I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.Attached is a rebased patch.
Thank you for updating the patch!
I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.I confirmed the patch and I am fine with it.
I've started reviewing this patch since it's marked as "ready for committer".
Thank you for your reviewing this patch!
I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.The latest patch looks good to me. I just have one minor comment:
only the privileges for schemas, tables (including views and foreign
tables), sequences, functions, and types (including domains) can be
altered.In alter_default_privileges.sgml, this part should also mention large objects?
Agreed. I attached a updated patch.
Thanks for updating the patch!
If there are no objections, I'll proceed with committing it using the following commit log.
----------------
Extend ALTER DEFAULT PRIVILEGES to define default privileges for large objects.
Previously, ALTER DEFAULT PRIVILEGES did not support large objects.
This meant that to grant privileges to users other than the owner,
permissions had to be manually assigned each time a large object
was created, which was inconvenient.
This commit extends ALTER DEFAULT PRIVILEGES to allow defining default
access privileges for large objects. With this change, specified privileges
will automatically apply to newly created large objects, making privilege
management more efficient.
As a side effect, this commit introduces the new keyword OBJECTS
since it's used in the syntax of ALTER DEFAULT PRIVILEGES.
Original patch by Haruka Takatsuka, with some fixes and tests by Yugo Nagata,
and rebased by Laurenz Albe.
Author: Takatsuka Haruka <harukat@sraoss.co.jp>
Co-authored-by: Yugo Nagata <nagata@sraoss.co.jp>
Co-authored-by: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Masao Fujii <masao.fujii@gmail.com>
Discussion: /messages/by-id/20240424115242.236b499b2bed5b7a27f7a418@sraoss.co.jp
----------------
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2025/04/04 0:21, Fujii Masao wrote:
Thanks for updating the patch!
If there are no objections, I'll proceed with committing it using the following commit log.
I've pushed the patch. Thanks!
While testing the feature, I noticed that psql doesn't complete
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
"GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
patch adds tab-completion support for both cases.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v1-0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE.patchtext/plain; charset=UTF-8; name=v1-0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE.patchDownload+20-1
On Fri, Apr 04, 2025 at 07:18:11PM +0900, Fujii Masao wrote:
I've pushed the patch. Thanks!
Just a heads up, I fixed a pgindent issue in this commit (see commits
e1a8b1ad58 and 742317a80f). I'd ordinarily just report it, but since we're
nearing feature freeze, I just fixed it because my workflow (and presumably
others') involves running pgindent on the entire tree periodically.
--
nathan
On 2025/04/04 23:47, Nathan Bossart wrote:
On Fri, Apr 04, 2025 at 07:18:11PM +0900, Fujii Masao wrote:
I've pushed the patch. Thanks!
Just a heads up, I fixed a pgindent issue in this commit (see commits
e1a8b1ad58 and 742317a80f). I'd ordinarily just report it, but since we're
nearing feature freeze, I just fixed it because my workflow (and presumably
others') involves running pgindent on the entire tree periodically.
Thanks a lot!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, 4 Apr 2025 19:18:11 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2025/04/04 0:21, Fujii Masao wrote:
Thanks for updating the patch!
If there are no objections, I'll proceed with committing it using the following commit log.
I've pushed the patch. Thanks!
Thank you!
While testing the feature, I noticed that psql doesn't complete
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
"GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
patch adds tab-completion support for both cases.
This patch looks good to me. This works as expected.
While looking into this patch, I found that the tab completion suggests
TO/FROM even after "LARGE OBJECT", but it is not correct because
there should be largeobject id at that place. This is same for the
"FOREIGN SERVER", server names should be suggested ratar than TO/FROM
in this case.
The additional patch 0002 fixed to prevents to suggest TO or FROM right
after LARGE OBJECT or FOREIGN SERVER. Also, it allows to suggest list of
foreign server names after FOREIGN SERVER.
The 0001 patch is the same you proposed.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
0002-psql-Some-improvement-of-tab-completion-for-GRANT-RE.patchtext/x-diff; name=0002-psql-Some-improvement-of-tab-completion-for-GRANT-RE.patchDownload+12-5
0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE-on.patchtext/x-diff; name=0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE-on.patchDownload+20-1
On Tue, 8 Apr 2025 12:28:57 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:
On Fri, 4 Apr 2025 19:18:11 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:On 2025/04/04 0:21, Fujii Masao wrote:
Thanks for updating the patch!
If there are no objections, I'll proceed with committing it using the following commit log.
I've pushed the patch. Thanks!
Thank you!
While testing the feature, I noticed that psql doesn't complete
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
"GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
patch adds tab-completion support for both cases.This patch looks good to me. This works as expected.
While looking into this patch, I found that the tab completion suggests
TO/FROM even after "LARGE OBJECT", but it is not correct because
there should be largeobject id at that place. This is same for the
"FOREIGN SERVER", server names should be suggested ratar than TO/FROM
in this case.The additional patch 0002 fixed to prevents to suggest TO or FROM right
after LARGE OBJECT or FOREIGN SERVER. Also, it allows to suggest list of
foreign server names after FOREIGN SERVER.
While looking at the thread [1]/messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com, I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?
[1]: /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com
Best regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
On 2025/06/11 11:49, Yugo Nagata wrote:
While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com
I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.
For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.
That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.
Regarding the 0002 patch:
- else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
- COMPLETE_WITH("TO");
- else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
- COMPLETE_WITH("FROM");
+ else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+ {
+ if (TailMatches("FOREIGN", "SERVER"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+ else if (!TailMatches("LARGE", "OBJECT"))
+ {
+ if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+ COMPLETE_WITH("TO");
+ else
+ COMPLETE_WITH("FROM");
+ }
+ }Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
On Wed, 11 Jun 2025 13:33:07 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2025/06/11 11:49, Yugo Nagata wrote:
While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com
I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.Regarding the 0002 patch:
- else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny)) - COMPLETE_WITH("TO"); - else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny)) - COMPLETE_WITH("FROM"); + else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny)) + { + if (TailMatches("FOREIGN", "SERVER")) + COMPLETE_WITH_QUERY(Query_for_list_of_servers); + else if (!TailMatches("LARGE", "OBJECT")) + { + if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny)) + COMPLETE_WITH("TO"); + else + COMPLETE_WITH("FROM"); + } + }Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?
Sorry, I made a stupid mistake.
+ else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
This should be "GRANT|REVOKE".
I've attached update patches. (There is no change on 0001.)
Best regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>