Re: including backend ID in relpath of temp rels - updated patch

On Thu, Aug 12, 2010 at 1:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Lastly, it bothers me that you've put in code to delete files belonging
>> to temp rels during crash restart, without any code to clean up their
>> catalog entries.  This will therefore lead to dangling pg_class
>> references, with uncertain but probably not very nice consequences.
>> I think that probably shouldn't go in until there's code to drop the
>> catalog entries too.
>
> I thought about this pretty carefully, and I don't believe that there
> are any unpleasant consequences.  The code that assigns relfilenode
> numbers is pretty careful to check that the newly assigned value is
> unused BOTH in pg_class and in the directory where the file will be
> created, so there should be no danger of a number getting used over
> again while the catalog entries remain.  Also, the drop-object code
> doesn't mind that the physical storage doesn't exist; it's perfectly
> happy with that situation.  It is true that you will get an ERROR if
> you try to SELECT from a table whose physical storage has been
> removed, but that seems OK.

After further reflection, I think that the above reasoning is wrong.
GetNewRelFileNode() guarantees that the OID doesn't collide with the
OID column of pg_class, not the relfilenode column of pg_class; and,
per the comments to that function, it only guarantees this when
creating a new relation (to which we're therefore assigning an OID)
and not when rewriting an existing relation. So it provides no
protection against the scenario where backend #1 drops a table that
lacks physical storage just as backend #2 assigns that OID to some
other relation and begins creating files, which backend #1 then blows
away.

However, I think we're safe for a different reason. The only time we
unlink the physical storage of a relation without nuking its catalog
entries is during the startup sequence, when we wipe out the physical
storage for any leftover temp tables. So if there's a race condition,
it can only apply to temp tables. But temp tables for a particular
backend ID can only be created by that backend, and before a backend
will create any temp tables it will drop any previously existing temp
schema. Thus, by the time a temp table can get created, there CAN'T
be any leftover catalog entries from previous sessions for it to
potentially collide with.

I think the reasoning above probably should be added to the comment at
the beginning of GetNewRelFileNode(), and I'll go do that unless
someone thinks it's incorrect. The first sentence of that comment is
also now technically incorrect: what it now does is generate a
relfilenode such that <tablespace, relfilenode, backendid> is unique.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company