Thread: Okay to change TypeCreate() signature in back branches?

I looked into the bug reported by Cott Lang that pg_type.typowner is
incorrect for a table's toast table after a rewriting ALTER TYPE
command.  The problem occurs because an entirely new toast table
is built during the rewrite.  The correct table owner is passed to
heap_create_with_catalog(), but it doesn't get passed down to
TypeCreate(), which instead uses the current userid.

The same problem exists in CLUSTER, since it uses the same
table-rewriting logic.  In the CLUSTER form it can be demonstrated clear
back to 7.4.  The CLUSTER form seems particularly nasty since that's
much more likely to be executed as a superuser rather than the table
owner.

Now the ownership of a table rowtype isn't usually that important;
I'm not sure if there are any bad consequences other than the one
Cott reported, ie, pg_dump starting to complain if you drop the
superuser role that did the ALTER or CLUSTER.  Still, it seems like
a must-fix issue.

The obvious fix involves adding an ownerid parameter to TypeCreate,
but I'm a tad worried about whether this will break any third-party
add-on code.  Does anyone know of non-core code that calls TypeCreate?
        regards, tom lane

I wrote:
> I looked into the bug reported by Cott Lang that pg_type.typowner is
> incorrect for a table's toast table after a rewriting ALTER TYPE
> command.
> ...
> The obvious fix involves adding an ownerid parameter to TypeCreate,
> but I'm a tad worried about whether this will break any third-party
> add-on code.  Does anyone know of non-core code that calls TypeCreate?

I started back-patching this and found out that 8.0 and 7.4 actually
have a worse form of the problem: the toast table's pg_class row also
shows the altering user's ID instead of the table owner's ID.  (Hey,
at least it's consistent with the pg_type row ;-).)  The reason for
the change is that we noticed a slight variant on the issue here:
http://archives.postgresql.org/pgsql-hackers/2005-08/msg00906.php
at which point we fixed things so that indexes and toast tables were
certain to inherit their ownership from the parent table.  But we
(I) forgot that the toast table also has a pg_type row with that info.

To fix this via a straightforward backport would mean also altering the
signature of heap_create_with_catalog() in 8.0/7.4 --- it takes an
ownerid parameter in 8.1+ but not in the older branches.  That seems
like it increases the risk of breaking third-party code noticeably.

There are a number of options at this point, including fixing the
problem only in HEAD, fixing back to 8.1 but no further, or making
wrapper functions in the back branches to preserve the existing
argument lists of heap_create_with_catalog and/or TypeCreate.

I'm not really eager to do the wrapper-function bit; it doesn't quite
seem like this bug is worth that, since it's been there basically
forever with few complaints.

Comments?
        regards, tom lane

On Mon, Feb 23, 2009 at 8:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> There are a number of options at this point, including fixing the
> problem only in HEAD, fixing back to 8.1 but no further, or making
> wrapper functions in the back branches to preserve the existing
> argument lists of heap_create_with_catalog and/or TypeCreate.

I'd go for fixing it properly back to 8.1. 8.1 is the oldest version
people still put into production with new applications IMHO (due
mainly to its inclusion in current versions of RHEL and SLES).

I'm not sure it's worth it to backport it to older versions. It seems
like a minor problem and older versions I still see in production
usually aren't running the latest minor version anyway. I can't
measure the time needed to write the wrapper functions though. If it's
just a matter of a couple of minutes and it's not risky, perhaps it's
better to fix it right now.

-- 
Guillaume

Guillaume Smet <guillaume.smet@gmail.com> writes:
> On Mon, Feb 23, 2009 at 8:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There are a number of options at this point, including fixing the
>> problem only in HEAD, fixing back to 8.1 but no further, or making
>> wrapper functions in the back branches to preserve the existing
>> argument lists of heap_create_with_catalog and/or TypeCreate.

> I'd go for fixing it properly back to 8.1. 8.1 is the oldest version
> people still put into production with new applications IMHO (due
> mainly to its inclusion in current versions of RHEL and SLES).

I found another reason to do it that way: 8.1 and 8.2 actually create
an owner dependency for the pg_toast rowtype, meaning you *can't*
drop the role that issued the command unless you hack around the bug.
(8.3 and HEAD don't do that because they figure a rowtype must have
the same owner as its parent table...)  So the problem is non-cosmetic
in those branches.  It is cosmetic, in the sense that the only known
consequence is a harmless warning from pg_dump, in earlier and later
branches.

So, applied back to 8.1.
        regards, tom lane