Robert Haas wrote: > 2009/10/14 KaiGai Kohei <kai...@ak.jp.nec.com>: >> Robert Haas wrote: >>> On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: >>>> KaiGai Kohei <kai...@ak.jp.nec.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 <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
