Re: [PATCH] Largeobject access controls

Robert Haas wrote:
> 2009/10/14 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Robert Haas wrote:
>>> On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>>> Tom Lane wrote:
>>>>>> The most serious problem is that you ripped out myLargeObjectExists(),
>>>>>> apparently because you didn't understand what it's there for. The reason
>>>>>> it's there is to ensure that pg_dump runs don't fail. pg_dump is expected
>>>>>> to produce a consistent dump of all large objects that existed at the
>>>>>> time of its transaction snapshot. With this code, pg_dump would get a
>>>>>> "large object doesn't exist" error on any LO that is deleted between the
>>>>>> time of the snapshot and the time pg_dump actually tries to dump it ---
>>>>>> which could be quite a large window in a large database.
>>>>> The origin of this matter is the basis of existence was changed.
>>>>> Our current basis is whether pg_largeobject has one or more data chunk for
>>>>> the given loid in the correct snapshot, or not.
>>>>> The newer one is whether pg_largeobject_metadata has the entry for the given
>>>>> loid in the SnapshotNow, or not.
>>>> Which is wrong. You can certainly switch your attention as to which
>>>> catalog to look in, but you can't change the snapshot that the test is
>>>> referenced to.
>>>>
>>>>> The newer basis is same as other database objects, such as functions.
>>>>> But why pg_dump performs correctly for these database objects?
>>>> The reason pg_dump works reasonably well is that it takes locks on
>>>> tables, and the other objects it dumps (such as functions) have
>>>> indivisible one-row representations in the catalogs. They're either
>>>> there when pg_dump looks, or they're not. What you would have here
>>>> is that pg_dump would see LO data chunks when it looks into
>>>> pg_largeobject using its transaction snapshot, and then its attempts to
>>>> open those LO OIDs would fail because the metadata is not there anymore
>>>> according to SnapshotNow.
>>>>
>>>> There are certainly plenty of corner-case issues in pg_dump; I've
>>>> complained before that it's generally a bad idea to be migrating pg_dump
>>>> functionality into internal backend routines that tend to rely on
>>>> SnapshotNow. But if we change LO dumping like this, it's not going to
>>>> be a corner case --- it's going to be a common race-condition failure
>>>> with a window measured in many minutes if not hours. That's more than
>>>> sufficient reason to reject this patch, because the added functionality
>>>> isn't worth it. And pg_dump isn't even the only client that depends
>>>> on the assumption that a read-only open LO is static.
>>>>
>>>>>> Moving on to lesser but still significant problems, you probably already
>>>>>> guessed my next one: there's no pg_dump support.
>>>>> Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.
>>>> We do not commit system changes that lack necessary pg_dump support.
>>>> There are some things you can leave till later, but pg_dump is not
>>>> negotiable. (Otherwise, testers would be left with databases they
>>>> can't dump/reload across the next forced initdb.)
>>> As part of closing out this CommitFest, I am marking this patch as
>>> Returned with Feedback. Because the amount of work that remains to be
>>> done is so substantial, that might have been a good idea anyway, but
>>> since the clock is expiring the point is moot.
>> Could you wait for the next 24 hours please?
>>
>> It is unproductive to break off the discussion for a month,
>> even if the patch will be actually commited at the December.
>
> Well, I don't know that I have the deciding vote here. I don't have
> any super-powers to make Tom - or anyone else - review the next
> version of this patch for this CommitFest, the next CommitFest, or at
> all. In my ideal world, Tom would review every patch in his area of
> expertise the day it came in and commit it right away, especially the
> ones that implement features I happen to like. But unless I offer to
> pay Tom's salary, and maybe not even then, that isn't going to happen.

I'm sorry for this a little bit emotional response.

Anyway, what I wanted to say was my wish that I wanted to conclude
or make a direction for the issue (what snapshot should be applied.)
It is unclear now, so I would like to continue the discussion a bit
more.

> Backing up a little bit, my understanding of the CommitFest process is
> that it is a time for everyone to stop working on their own patches
> and help review and commit the patches of others. Because everyone
> wants to get back to their own work, we try very hard to wrap
> CommitFests up in one month so that everyone can then have a full
> month to do their own work before the next CommitFest starts. I don't
> see any reason to believe that this patch is so important that we
> should make an exception to that policy on its behalf, and I think
> it's unfair of you to ask. Tom, I, and others have almost entirely
> put aside our own development work for the last month to focus on this
> CommitFest. You haven't offered to help, are now asking us to put
> aside our work for even longer for your benefit. Furthermore, you
> haven't offered to help with either of the previous two CommitFests in
> which I've been involved either, despite having submitted large,
> complex patches that required substantial reviewing effort.

I have to focus on my patches with highest priority in CommitFest,
but it may be possible to help reviewing the proposed patches in
the off-fest season. It is illegal/undesirable for the process?

We already can find a patch corresponding to security feature.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>