Thread

1. Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Gurjeet Singh <singh.gurjeet@gmail.com> — 2013-01-14T22:31:42Z

   TODO Item: Prevent temporary tables created with ON COMMIT DELETE ROWS from
   repeatedly truncating the table on every commit if the table is already
   empty
   
   Please find attached two patches, implementing two different approaches to
   fix the issue of COMMIT truncating empty TEMP tables that have the ON
   COMMIT DELETE ROWS attribute.
   
   v2.patch: This approach introduces a boolean 'rd_rows_inserted' in
   RelationData struct, and sets this struct to true for every TEMP table in
   which a row is inserted. During commit, we avoid truncating those OCDR temp
   tables that haven't been inserted into in this transaction.
   
   v3.patch: This is the original suggestion by Robert Haas, where we keep a
   global variable to indicate if any TEMP table has been a target of INSERT,
   and if not, we skip truncating all OCDR temp tables. The downside of this
   approach I see is that that if a transaction inserts a row into even one of
   the OCDR temp tables, we end up attempting truncating all temp tables, even
   those that are empty.
   
   I am attaching the test case, a psql script, I used to get the timing of
   BEGIN and COMMIT operations. I executed the test like this:
   
   $ for (( i = 1 ; i <= 4; ++i )) ; do pgsql -f ~/empty_temp_tables_test.psql
   | tee post_patchv2_run${i}.log; done
   
   And then extracted the timing info of BEGIN and COMMITs using this pipeline:
   
   $ grep -A 1 -E 'BEGIN|COMMIT' post_patchv2_run4.log | grep Time: | cut -d :
   -f 2 | cut -d ' ' -f 2
   
   Also attached is the PDF of the test runs. It includes the times, their
   averages and '% Change' across the averages. '% Change' column is derived
   as round((pre_patch_avg - post_patch_avg)/pre_patch_avg*100, 2).
   
   The tests start with a VACUUM FULL, of the database. This is to ensure that
   there are no dead rows in pg_class and other system tables, leftover from
   previous run. It also helps in bringing all the database tables into
   shared_buffers, so this also helps in decreasing variability of the test
   runs.
   
   I tried quite hard to eliminate any variability of the test environment,
   and for this I disabled Autovacuum, increased checkpoint_segments,
   increased shared_buffers, etc. I then isolated each type of test into
   session of its own, by disconnecting and reconnecting again. But during the
   last test I realized that the disconnection is not instantaneous, and the
   backend process from the previous process lingered around for a few
   seconds, for as log as 7-8 seconds, consuming nearly 100% CPU. And during
   this period then next connection running the test was also consuming about
   100% CPU.
   
   So even though I tried to isolate the tests, I am sure this delay in
   backend death and the CPU consumption by the dying process must be
   interfering with the results. So test results need to be taken with a pinch
   of salt.
   -- 
   Gurjeet Singh
   
   http://gurjeet.singh.im/
   

2. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-15T03:33:45Z

   Gurjeet Singh <singh.gurjeet@gmail.com> writes:
   > Please find attached two patches, implementing two different approaches to
   > fix the issue of COMMIT truncating empty TEMP tables that have the ON
   > COMMIT DELETE ROWS attribute.
   
   > v2.patch: This approach introduces a boolean 'rd_rows_inserted' in
   > RelationData struct, and sets this struct to true for every TEMP table in
   > which a row is inserted. During commit, we avoid truncating those OCDR temp
   > tables that haven't been inserted into in this transaction.
   
   I think this is unacceptable on its face.  It essentially supposes that
   relcache entries are reliable storage.  They are not.  There are some
   places where we rely on relcache entries preserving state information
   for optimization purposes --- but in this case, discarding a relcache
   entry would result in visibly incorrect behavior, not just some
   performance loss.
   
   			regards, tom lane
   
   
   

3. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Gurjeet Singh <singh.gurjeet@gmail.com> — 2013-01-15T04:13:19Z

   On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   
   > Gurjeet Singh <singh.gurjeet@gmail.com> writes:
   > > Please find attached two patches, implementing two different approaches
   > to
   > > fix the issue of COMMIT truncating empty TEMP tables that have the ON
   > > COMMIT DELETE ROWS attribute.
   >
   > > v2.patch: This approach introduces a boolean 'rd_rows_inserted' in
   > > RelationData struct, and sets this struct to true for every TEMP table in
   > > which a row is inserted. During commit, we avoid truncating those OCDR
   > temp
   > > tables that haven't been inserted into in this transaction.
   >
   > I think this is unacceptable on its face.  It essentially supposes that
   > relcache entries are reliable storage.  They are not.  There are some
   > places where we rely on relcache entries preserving state information
   > for optimization purposes --- but in this case, discarding a relcache
   > entry would result in visibly incorrect behavior, not just some
   > performance loss.
   >
   
   Would it be acceptable if we inverted the meaning of the struct member, and
   named it to  rd_rows_not_inserted. When registering an ON COMMIT action, we
   can set this member to true, and set it to false when inserting a row into
   it. The pre-commit hook will truncate any relation that doesn't have this
   member set to true.
   
   With that in place, even if the relcache entry is discarded midway through
   the transaction, the cleanup code will truncate the relation, preserving
   the correct behaviour.
   -- 
   Gurjeet Singh
   
   http://gurjeet.singh.im/
   

4. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-15T15:57:54Z

   Gurjeet Singh <singh.gurjeet@gmail.com> writes:
   > On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> I think this is unacceptable on its face.  It essentially supposes that
   >> relcache entries are reliable storage.  They are not.
   
   > Would it be acceptable if we inverted the meaning of the struct member, and
   > named it to  rd_rows_not_inserted. When registering an ON COMMIT action, we
   > can set this member to true, and set it to false when inserting a row into
   > it. The pre-commit hook will truncate any relation that doesn't have this
   > member set to true.
   
   > With that in place, even if the relcache entry is discarded midway through
   > the transaction, the cleanup code will truncate the relation, preserving
   > the correct behaviour.
   
   Well, that would fail in the safe direction, but it just seems
   excessively ugly and hard-to-understand.  Given the field demand for
   this optimization (which so far as I've noticed is nil), I'm not
   convinced we need to do this.
   
   			regards, tom lane
   
   
   

5. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Robert Haas <robertmhaas@gmail.com> — 2013-01-15T18:51:00Z

   On Tue, Jan 15, 2013 at 10:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Gurjeet Singh <singh.gurjeet@gmail.com> writes:
   >> On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >>> I think this is unacceptable on its face.  It essentially supposes that
   >>> relcache entries are reliable storage.  They are not.
   >
   >> Would it be acceptable if we inverted the meaning of the struct member, and
   >> named it to  rd_rows_not_inserted. When registering an ON COMMIT action, we
   >> can set this member to true, and set it to false when inserting a row into
   >> it. The pre-commit hook will truncate any relation that doesn't have this
   >> member set to true.
   >
   >> With that in place, even if the relcache entry is discarded midway through
   >> the transaction, the cleanup code will truncate the relation, preserving
   >> the correct behaviour.
   >
   > Well, that would fail in the safe direction, but it just seems
   > excessively ugly and hard-to-understand.  Given the field demand for
   > this optimization (which so far as I've noticed is nil), I'm not
   > convinced we need to do this.
   
   Yep, this is also why I prefer the approach of setting a global
   variable.  The demand for this is not *precisely* zero or it wouldn't
   be on the TODO list, but the global seems like it would head off the
   worst of the damage without requiring any fiddling with the relcache.
   
   On the third hand, the fact that a table is OCDR is recorded in
   backend-local storage somewhere, and that storage (unlike the
   relcache) had better be reliable.  So maybe there's some way to
   finesse it that way.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   
   

6. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-15T19:03:41Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > On the third hand, the fact that a table is OCDR is recorded in
   > backend-local storage somewhere, and that storage (unlike the
   > relcache) had better be reliable.  So maybe there's some way to
   > finesse it that way.
   
   Hm, keep a flag in that storage you mean?  Yeah, that could possibly
   work.
   
   			regards, tom lane
   
   
   

7. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-01-28T13:39:24Z

   On 15.01.2013 21:03, Tom Lane wrote:
   > Robert Haas<robertmhaas@gmail.com>  writes:
   >> On the third hand, the fact that a table is OCDR is recorded in
   >> backend-local storage somewhere, and that storage (unlike the
   >> relcache) had better be reliable.  So maybe there's some way to
   >> finesse it that way.
   >
   > Hm, keep a flag in that storage you mean?  Yeah, that could possibly
   > work.
   
   Sounds reasonable.
   
   Until someone gets around to write a patch along those lines, I'm 
   inclined to apply this one liner:
   
   *** a/src/backend/commands/tablecmds.c
   --- b/src/backend/commands/tablecmds.c
   ***************
   *** 10124,10130 **** PreCommit_on_commit_actions(void)
      				/* Do nothing (there shouldn't be such entries, actually) */
      				break;
      			case ONCOMMIT_DELETE_ROWS:
   ! 				oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
      				break;
      			case ONCOMMIT_DROP:
      				{
   --- 10124,10136 ----
      				/* Do nothing (there shouldn't be such entries, actually) */
      				break;
      			case ONCOMMIT_DELETE_ROWS:
   ! 				/*
   ! 				 * If this transaction hasn't accessed any temporary relations,
   ! 				 * we can skip truncating ON COMMIT DELETE ROWS tables, as
   ! 				 * they must still be empty.
   ! 				 */
   ! 				if (MyXactAccessedTempRel)
   ! 					oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
      				break;
      			case ONCOMMIT_DROP:
      				{
   
   We already have that MyXactAccessedTempRel global flag. Just checking 
   that should cover many common cases.
   
   - Heikki
   
   
   

8. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Robert Haas <robertmhaas@gmail.com> — 2013-01-29T02:41:16Z

   On Mon, Jan 28, 2013 at 8:39 AM, Heikki Linnakangas
   <hlinnakangas@vmware.com> wrote:
   > On 15.01.2013 21:03, Tom Lane wrote:
   >> Robert Haas<robertmhaas@gmail.com>  writes:
   >>>
   >>> On the third hand, the fact that a table is OCDR is recorded in
   >>> backend-local storage somewhere, and that storage (unlike the
   >>> relcache) had better be reliable.  So maybe there's some way to
   >>> finesse it that way.
   >>
   >> Hm, keep a flag in that storage you mean?  Yeah, that could possibly
   >> work.
   >
   > Sounds reasonable.
   >
   > Until someone gets around to write a patch along those lines, I'm inclined
   > to apply this one liner:
   >
   > *** a/src/backend/commands/tablecmds.c
   > --- b/src/backend/commands/tablecmds.c
   > ***************
   > *** 10124,10130 **** PreCommit_on_commit_actions(void)
   >                                 /* Do nothing (there shouldn't be such
   > entries, actually) */
   >                                 break;
   >                         case ONCOMMIT_DELETE_ROWS:
   > !                               oids_to_truncate =
   > lappend_oid(oids_to_truncate, oc->relid);
   >                                 break;
   >                         case ONCOMMIT_DROP:
   >                                 {
   > --- 10124,10136 ----
   >                                 /* Do nothing (there shouldn't be such
   > entries, actually) */
   >                                 break;
   >                         case ONCOMMIT_DELETE_ROWS:
   > !                               /*
   > !                                * If this transaction hasn't accessed any
   > temporary relations,
   > !                                * we can skip truncating ON COMMIT DELETE
   > ROWS tables, as
   > !                                * they must still be empty.
   > !                                */
   > !                               if (MyXactAccessedTempRel)
   > !                                       oids_to_truncate =
   > lappend_oid(oids_to_truncate, oc->relid);
   >                                 break;
   >                         case ONCOMMIT_DROP:
   >                                 {
   >
   > We already have that MyXactAccessedTempRel global flag. Just checking that
   > should cover many common cases.
   
   +1 for that.  I'm actually unconvinced that we need to do any better
   than that in general.  But certainly that seems like a good first
   step.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   
   

9. Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

   Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-01-29T08:45:42Z

   On 29.01.2013 04:41, Robert Haas wrote:
   > On Mon, Jan 28, 2013 at 8:39 AM, Heikki Linnakangas
   > <hlinnakangas@vmware.com>  wrote:
   >> We already have that MyXactAccessedTempRel global flag. Just checking that
   >> should cover many common cases.
   >
   > +1 for that.  I'm actually unconvinced that we need to do any better
   > than that in general.  But certainly that seems like a good first
   > step.
   
   Ok, committed that.
   
   - Heikki