From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: pgsql-hackers@postgreSQL.org
Subject: Rethinking autovacuum.c memory handling
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <13729.1506114327.0@sss.pgh.pa.us>
Date: Fri, 22 Sep 2017 17:09:03 -0400
Message-ID: <13849.1506114543@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <13729.1506114327.1@sss.pgh.pa.us>

I notice that autovacuum.c calls autovacuum_do_vac_analyze, and
thereby vacuum(), in TopTransactionContext.  This doesn't seem
like a terribly great idea, because it doesn't correspond to what
happens during a manually-invoked vacuum.  TopTransactionContext
will go away when vacuum() commits the outer transaction, whereas
in non-autovac usage, we call vacuum() in a PortalHeapMemory
context that is not a child of TopTransactionContext and is not
at risk of being reset multiple times during the vacuum().  This'd
be a hazard if autovacuum_do_vac_analyze or vacuum did any palloc's
before getting to the main loop.  More generally, I'm not aware of
other cases where we invoke a function in a context that we know
that function will destroy as it executes.

I don't see any live bug associated with this in HEAD, but this behavior
requires a rather ugly (and memory-leaking) workaround in the proposed
patch to allow multiple vacuum target rels.

What I think we should do instead is invoke autovacuum_do_vac_analyze
in the PortalContext that do_autovacuum has created, which we already
have a mechanism to reset once per table processed in do_autovacuum.

The attached patch does that, and also modifies perform_work_item()
to use the same approach.  Right now perform_work_item() has a
copied-and-pasted MemoryContextResetAndDeleteChildren(PortalContext)
call in its error recovery path, but that seems a bit out of place
given that perform_work_item() isn't using PortalContext otherwise.

Comments, objections?

			regards, tom lane

PS: I was disappointed to find out that perform_work_item() isn't
exercised at all in the standard regression tests.


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="use-portalcontext-in-autovacuum.patch";
	charset="us-ascii"
Content-ID: <13729.1506114327.2@sss.pgh.pa.us>
Content-Description: use-portalcontext-in-autovacuum.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/a=
utovacuum.c
index b745d89..1a32433 100644
*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
*************** do_autovacuum(void)
*** 2444,2451 ****
  		 */
  		PG_TRY();
  		{
  			/* have at it */
- 			MemoryContextSwitchTo(TopTransactionContext);
  			autovacuum_do_vac_analyze(tab, bstrategy);
=20=20
  			/*
--- 2444,2453 ----
  		 */
  		PG_TRY();
  		{
+ 			/* Use PortalContext for any per-table allocations */
+ 			MemoryContextSwitchTo(PortalContext);
+=20
  			/* have at it */
  			autovacuum_do_vac_analyze(tab, bstrategy);
=20=20
  			/*
*************** do_autovacuum(void)
*** 2482,2487 ****
--- 2484,2492 ----
  		}
  		PG_END_TRY();
=20=20
+ 		/* Make sure we're back in AutovacMemCxt */
+ 		MemoryContextSwitchTo(AutovacMemCxt);
+=20
  		did_vacuum =3D true;
=20=20
  		/* the PGXACT flags are reset at the next end of transaction */
*************** perform_work_item(AutoVacuumWorkItem *wo
*** 2614,2619 ****
--- 2619,2627 ----
=20=20
  	autovac_report_workitem(workitem, cur_nspname, cur_datname);
=20=20
+ 	/* clean up memory before each work item */
+ 	MemoryContextResetAndDeleteChildren(PortalContext);
+=20
  	/*
  	 * We will abort the current work item if something errors out, and
  	 * continue with the next one; in particular, this happens if we are
*************** perform_work_item(AutoVacuumWorkItem *wo
*** 2622,2630 ****
  	 */
  	PG_TRY();
  	{
! 		/* have at it */
! 		MemoryContextSwitchTo(TopTransactionContext);
=20=20
  		switch (workitem->avw_type)
  		{
  			case AVW_BRINSummarizeRange:
--- 2630,2639 ----
  	 */
  	PG_TRY();
  	{
! 		/* Use PortalContext for any per-work-item allocations */
! 		MemoryContextSwitchTo(PortalContext);
=20=20
+ 		/* have at it */
  		switch (workitem->avw_type)
  		{
  			case AVW_BRINSummarizeRange:
*************** perform_work_item(AutoVacuumWorkItem *wo
*** 2668,2673 ****
--- 2677,2685 ----
  	}
  	PG_END_TRY();
=20=20
+ 	/* Make sure we're back in AutovacMemCxt */
+ 	MemoryContextSwitchTo(AutovacMemCxt);
+=20
  	/* We intentionally do not set did_vacuum here */
=20=20
  	/* be tidy */

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

------- =_aaaaaaaaaa0--