Re: [PATCH] Largeobject access controls

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.

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.

All that having been said, I have no intention of or desire to
foreclose discussion on this patch, or any desire to postpone the time
that it gets reviewed and committed. Besides the general desire to
let everyone get back to their own work if they so choose, the other
point of marking this as Returned with Feedback is to say that it
isn't going to get committed before alpha2 is stamped, which I think
is probably true even if a new version shows up in the next 5 minutes.
But that doesn't mean we can't keep talking about it, and I hope we
do.

...Robert