Horton

Thread

Commits

  1. Fix bugs in manipulation of large objects.

  2. Refactor ownercheck functions

  1. LargeObjectRelationId vs LargeObjectMetadataRelationId, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-13T19:53:29Z

    By chance I discovered that checks on large object ownership
are broken in v16+.  For example,

regression=# create user alice;
CREATE ROLE
regression=# \c - alice
You are now connected to database "regression" as user "alice".
regression=> \lo_import test
lo_import 40378
regression=> comment on large object 40378 is 'test';
ERROR:  unrecognized class ID: 2613

This has been failing since commit afbfc0298, which refactored
ownership checks, replacing pg_largeobject_ownercheck and
allied routines with object_ownercheck.  That function lacks
the little dance that's been stuck into assorted crannies:

                if (classId == LargeObjectRelationId)
                    classId = LargeObjectMetadataRelationId;

which translates from the object-address representation with
classId LargeObjectRelationId to the catalog we actually need
to look at.

The proximate cause of the failure is in get_object_property_data,
so I first thought of making that function do this transposition.
That might be a good thing to do, but it wouldn't be enough to
fix the problem, because we'd then reach this in object_ownercheck:

		rel = table_open(classid, AccessShareLock);

which is going to examine the wrong catalog.  So AFAICS what
we have to do is put this substitution into object_ownercheck,
adding to the four or five places that know about it already.

This is an absolutely horrid mess, of course.  The big problem
is that at this point I have exactly zero confidence that there
are not other places with the same bug; and it's not apparent
how to find them.

There seems little choice but to make the hacky fix in v16,
but I wonder whether we shouldn't be more ambitious and try
to fix this permanently in HEAD, by getting rid of the
discrepancy in which OID to use.  ISTM the correct fix
is to change the ObjectAddress representation of large
objects to use classid LargeObjectMetadataRelationId.
Somebody seems to have felt that that would create more
problems than it solves, but I have to disagree.  If we
stick with the current way, we are going to be hitting
problems of this ilk forevermore.

Thoughts?

			regards, tom lane

  2. Re: LargeObjectRelationId vs LargeObjectMetadataRelationId, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-15T19:01:07Z

    I wrote:
> This is an absolutely horrid mess, of course.  The big problem
> is that at this point I have exactly zero confidence that there
> are not other places with the same bug; and it's not apparent
> how to find them.

I took a look at every reference to LargeObjectRelationId and
LargeObjectMetadataRelationId, and indeed found two more bugs
(one very minor, and one only latent, but bugs nonetheless).
I'm not entirely convinced that there are no others, but
this is the best I can do for now.

> There seems little choice but to make the hacky fix in v16,
> but I wonder whether we shouldn't be more ambitious and try
> to fix this permanently in HEAD, by getting rid of the
> discrepancy in which OID to use.  ISTM the correct fix
> is to change the ObjectAddress representation of large
> objects to use classid LargeObjectMetadataRelationId.
> Somebody seems to have felt that that would create more
> problems than it solves, but I have to disagree.  If we
> stick with the current way, we are going to be hitting
> problems of this ilk forevermore.

I still kind of feel that way, but I realized that making
such a change would be rather unpleasant for pg_dump:
it'd have to cope with pg_depend contents that vary across
versions, and probably do some translation if we'd like
dump archive files to stay consistent.  We'd also break
post-create and post-alter hooks, and likely some other
third-party code.  So maybe best to leave it alone.

I've pushed fixes for the bugs I was able to find.

			regards, tom lane