Thread: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT
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.
--
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.
--
Attachment
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
Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT
From
Gurjeet Singh
Date:
On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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 <singh.gurjeet@gmail.com> writes:I think this is unacceptable on its face. It essentially supposes that
> 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.
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 <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
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
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
Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT
From
Heikki Linnakangas
Date:
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;
caseONCOMMIT_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;
caseONCOMMIT_DROP: {
We already have that MyXactAccessedTempRel global flag. Just checking
that should cover many common cases.
- Heikki
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
Re: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT
From
Heikki Linnakangas
Date:
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