Re: Temporary tables prevent autovacuum, leading to XID wraparound

At Mon, 30 Jul 2018 16:59:16 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180730075916(dot)GB2878(at)paquier(dot)xyz>
> On Fri, Jul 27, 2018 at 08:27:26AM +0000, Tsunakawa, Takayuki wrote:
> > I don't have a strong opinion, but I wonder which of namespace.c or
> > autovacuum.c is suitable, because isTempNamespaceInUse is specific to
> > autovacuum.
>
> I think that there is also a point in allowing other backends to use it
> as well, so I left it in namespace.c. I have been doing more testing
> with this patch today. In order to catch code paths where this is

+1 for namespace.c from me.

> triggered I have for example added an infinite loop in do_autovacuum
> when finding a temp table which exits once a given on-disk file is
> found. This lets plenty of time to attach autovacuum to a debugger, and
> play with other sessions in parallel, so I have checked the
> transactional assumption this patch relied on, and tested down to v10 as
> that's where removal of orphaned temp relations has been made more
> aggressive. This patch can also be applied cleanly there.
>
> The check on MyDatabaseId does not actually matter much as the same
> temporary namespace OID would get reused only after an OID wraparound
> with a backend using a different database but I left it anyway as that's
> more consistent to me to check for database and namespace, and added a
> sanity check to make sure that the routine gets called only by a process
> connected to a database.
>
> I am going to be out of town for a couple of days, and the next minor
> release planned is close by, so I am not pushing that today, but I'd
> like to do so after the next minor release if there are no objections.
> Attached is a patch with a proposal of commit message.

"int backendID" is left in autovacuum.c as an unused variable.

+ * decide if a temporary file entry can be marked as orphaned or not. Even
+ * if this is an atomic operation, this can be safely done lock-less as no

"Even if this is *not* an atomic operation" ?

+ * Mark the proc entry as owning this namespace which autovacuum uses to
+ * decide if a temporary file entry can be marked as orphaned or not. Even
+ * if this is an atomic operation, this can be safely done lock-less as no
+ * temporary relations associated to it can be seen yet by autovacuum when
+ * scanning pg_class.
+ */
+ MyProc->tempNamespaceId = namespaceId;

The comment looks wrong. After a crash having temp tables,
pg_class has orphaned temp relations and the namespace can be of
reconnected backends especially for low-numbered backends. So
autovacuum may look the shared variable while the reconnected
backend is writing it. The only harmful misbehavior is false
"unused" judgement. This happens only when reusing the same
namespace ID but the temp namespace is already cleaned up by
InitTempTableNamespace in the case. In the case of false "used"
judgement, the later autovacuum rounds will do the clean up
correctly.

In short, I think it is safely done lock-less but the reason is
not no concurrent reads but transient reads do not harm
(currently..).

| * Mark the proc entry as owning this namespace which autovacuum uses to
| * decide if a temporary file entry can be marked as orphaned or not. Even
| * if this is not an atomic operation, this can be safely done lock-less as
| * a transient read doesn't let autovacuum go wrong.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center