Re: [PATCH] Largeobject access controls

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.)

regards, tom lane