Re: 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.
--
Gurjeet Singh

http://gurjeet.singh.im/

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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

On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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/

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
> On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
>> On Mon, Jan 14, 2013 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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

On 15.01.2013 21:03, Tom Lane wrote:
> Robert Haas<robertmhaas(at)gmail(dot)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

On Mon, Jan 28, 2013 at 8:39 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 15.01.2013 21:03, Tom Lane wrote:
>> Robert Haas<robertmhaas(at)gmail(dot)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

On 29.01.2013 04:41, Robert Haas wrote:
> On Mon, Jan 28, 2013 at 8:39 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)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