Re: Temporary tables prevent autovacuum, leading to XID wraparound

On Thu, Jul 26, 2018 at 07:05:11PM +0900, Kyotaro HORIGUCHI wrote:
> At Wed, 18 Jul 2018 07:34:10 +0000, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote in <0A3221C70F24FB45833433255569204D1FA538FD(at)G01JPEXMBYT05>
>> From: Michael Paquier [mailto:michael(at)paquier(dot)xyz]
>>> + /* Does the backend own the temp schema? */
>>> + if (proc->tempNamespaceId != namespaceID)
>>> + return false;
>>> I have a very hard time believing that this is safe lock-less, and a spin
>>> lock would be enough it seems.
>>
>> The lwlock in BackendIdGetProc() flushes the CPU cache so that
>> proc->tempNamespaceId reflects the latest value. Or, do you mean
>> another spinlock elsewhere?
>
> It seems to be allowed that the series of checks on *proc results
> in false-positive, which is the safer side for the usage, even it
> is not atomically updated. Actually ->databaseId is written
> without taking a lock.

Well, from postinit.c there are a couple of assumptions in this case, so
it is neither white nor black:
/*
* Now we can mark our PGPROC entry with the database ID.
*
* We assume this is an atomic store so no lock is needed; though actually
* things would work fine even if it weren't atomic. Anyone searching the
* ProcArray for this database's ID should hold the database lock, so they
* would not be executing concurrently with this store. A process looking
* for another database's ID could in theory see a chance match if it read
* a partially-updated databaseId value; but as long as all such searches
* wait and retry, as in CountOtherDBBackends(), they will certainly see
* the correct value on their next try.
*/
MyProc->databaseId = MyDatabaseId;

Anyway, I have spent some time on this patch, and the thing is doing a
rather bad job about why it is fine to assume that the update can happen
lock-less, and the central part of it seems to be that autovacuum cannot
see the temporary schema created, and any objects on it when it scans
pg_class until it gets committed, and the tracking field is updated in
PGPROC before the commit.

> backend_uses_temp_namespace is taking two parameters but the
> first one is always derived from the second. backendID doesn't
> seem to be needed outside so just one parameter namespaceID is
> needed.

Yes, that reduces the number of calls to GetTempNamespaceBackendId.
autovacuum.c is a pretty bad place for stuff as namespace.c holds all
the logic related to temporary tablespaces, so I renamed the routine to
isTempNamespaceInUse and moved it there.

The patch I have now is attached. I have not been able to test it much,
particularly with orphaned temp tables and autovacuum, which is
something I'll try to look at this week end or perhaps at the beginning
of next week, heading toward a commit if I am fine enough with it.
Please feel free to look at it for the time being.

Thanks,
--
Michael