Re: Simplify ACL handling for large objects and removal of superuser() checks

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Vaishnavi Prabakaran <vaishnaviprabakaran(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Simplify ACL handling for large objects and removal of superuser() checks
Date: 2017-11-09 01:33:19
Message-ID: CAB7nPqSWkFci82Gxw7dsAdY0YKBCEDfBZdEcTfTkKLyQigx_PA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran
>> <vaishnaviprabakaran(at)gmail(dot)com> wrote:
>>> I moved the cf entry to "ready for committer", and though my vote is for
>>> keeping the existing API behavior with write implying read, I let the
>>> committer decide whether the following behavior change is Ok or not.
>>> "Reading from a large-object descriptor opened with INV_WRITE is NOT
>>> possible"
>
>> Thanks for the review!
>
> After chewing on this some more, I'm inclined to agree that we should
> not change the behavior of the read/write flags. There's been no
> field requests for a true-write-only mode, so I think we're much more
> likely to get complaints from users whose code we broke than plaudits
> from people who think the change is helpful.

Thanks for the input. Then that's two people favoring no changes.

> I believe it would be easy enough to adjust the patch so that we can
> still have the refactoring benefits; we'd just need the bit that
> translates from external to internal flags to go more like
>
> if (flags & INV_WRITE)
> descflags |= IFS_WRLOCK | IFS_RDLOCK;
> if (flags & INV_READ)
> descflags |= IFS_RDLOCK;
>
> (Preferably with a comment about why it's like this.)

Sure, I have updated the patch with this comment:
+ /*
+ * Historically, no difference is made between (INV_WRITE) and
+ * (INV_WRITE | INV_READ), the caller being allowed to read the large
+ * object descriptor in either case.
+ */
Of course please feel free to reword if you find something better-suited.

> Another idea would be to invent a new external flag bit "INV_WRITE_ONLY",
> so that people who wanted true write-only could get it, without breaking
> backwards-compatible behavior. But I'm inclined to wait for some field
> demand to show up before adding even that little bit of complication.

Demand that may never show up, and the current behavior on HEAD does
not receive any complains either. I am keeping the patch simple for
now. That's less aspirin needed for everybody.
--
Michael

Attachment Content-Type Size
0003-Move-ACL-checks-for-large-objects-when-opening-them.patch application/octet-stream 14.8 KB
0001-Remove-ALLOW_DANGEROUS_LO_FUNCTIONS-for-LO-related-s.patch application/octet-stream 2.3 KB
0002-Replace-superuser-checks-of-large-object-import-expo.patch application/octet-stream 4.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2017-11-09 02:09:20 Re: [PATCH] A hook for session start
Previous Message Amit Langote 2017-11-09 01:02:03 Re: [PATCH] fix wrong create table statement in documentation