Re: Transactions and temp tables

Hi,

I had the same problem as John with "could not open relation
1663/16384/16584: No such file or directory" in a specific combination
of transactions with temp tables
(http://archives.postgresql.org/pgsql-hackers/2008-02/msg01260.php). As
Heikki mentioned
(http://archives.postgresql.org/pgsql-hackers/2008-02/msg01277.php) we
should be able to allow CREATE+DROP in the same transaction.

I came up with a patch (currently based on 8.3.3) to address that issue.
Instead of relying on a boolean that tells if a temp table was accessed,
I keep a list of the Oid for the temp tables accessed in the transaction
and at prepare commit time, I check if the relations are still valid. I
also added a check to allow empty temp tables at prepare commit time
(this allows to use temp tables with 'on commit delete rows' options.

I am attaching the patch and the use cases I have been using to test it.
The test cases try to compile the various use cases that I have seen
reported on the list. Let me know what you think of the patch and if it
could be applied to 8.3 and 8.4?

Thanks in advance for your feedback,
manu

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
> Instead of relying on a boolean that tells if a temp table was accessed,
> I keep a list of the Oid for the temp tables accessed in the transaction
> and at prepare commit time, I check if the relations are still valid. I
> also added a check to allow empty temp tables at prepare commit time
> (this allows to use temp tables with 'on commit delete rows' options.
>
> I am attaching the patch and the use cases I have been using to test it.
> The test cases try to compile the various use cases that I have seen
> reported on the list.

Thanks for looking into this.

The patch allows preparing any transaction that has dropped the temp
table, even if it wasn't created in the same transaction. Is that sane?

Also, even if the table is created and dropped in the same transaction,
a subsequent transaction that tries to create and drop the table gets
blocked on the lock. I suppose we could just say that that's the way it
works, but I'm afraid it will come as a nasty surprise, making the
feature a lot less useful.

The fixed-size array of temp table oids is an unnecessary limitation. A
list or hash table would be better.

> Let me know what you think of the patch and if it
> could be applied to 8.3 and 8.4?

Not to 8.3. We only back-patch bug-fixes, and this isn't one.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hi Heikki,

> The patch allows preparing any transaction that has dropped the temp
> table, even if it wasn't created in the same transaction. Is that sane?
If you have a temp table created with an 'on commit delete rows' option
in another transaction, it would be fine to drop it in another
transaction. If the temp table was created without any on commit option,
it could only cross prepare commit if it is empty and then it could be
safely dropped in another transaction. That does not seem to insane for
me if you need temp tables for a session.
> Also, even if the table is created and dropped in the same
> transaction, a subsequent transaction that tries to create and drop
> the table gets blocked on the lock. I suppose we could just say that
> that's the way it works, but I'm afraid it will come as a nasty
> surprise, making the feature a lot less useful.
I do not get that one, if the table is dropped in the transaction the
lock is released. Why would another transaction be blocked when trying
to create/drop another temp table?
When I run my test cases (see attached file in previous mail), I
create/drop multiple times the same temp table in different transactions
and I do not experience any blocking.
> The fixed-size array of temp table oids is an unnecessary limitation.
> A list or hash table would be better.
You are right, I will fix that.
>> Let me know what you think of the patch and if it could be applied to
>> 8.3 and 8.4?
> Not to 8.3. We only back-patch bug-fixes, and this isn't one.
Ok understood.

Thanks for your feedback and don't hesitate to enlighten me on the
potential locking issue I did not understand.
Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Heikki,

Here is a new version of the patch using a hash table as you suggested.
I also include the tests that I have added to the regression test suite
to test the various scenarios.
All patches are based on Postgres 8.3.3, let me know if you want me to
generate patch for 8.4.

Thanks in advance for your feedback,
Emmanuel

Emmanuel Cecchet wrote:
> Hi Heikki,
>
>> The patch allows preparing any transaction that has dropped the temp
>> table, even if it wasn't created in the same transaction. Is that sane?
> If you have a temp table created with an 'on commit delete rows'
> option in another transaction, it would be fine to drop it in another
> transaction. If the temp table was created without any on commit
> option, it could only cross prepare commit if it is empty and then it
> could be safely dropped in another transaction. That does not seem to
> insane for me if you need temp tables for a session.
>> Also, even if the table is created and dropped in the same
>> transaction, a subsequent transaction that tries to create and drop
>> the table gets blocked on the lock. I suppose we could just say that
>> that's the way it works, but I'm afraid it will come as a nasty
>> surprise, making the feature a lot less useful.
> I do not get that one, if the table is dropped in the transaction the
> lock is released. Why would another transaction be blocked when trying
> to create/drop another temp table?
> When I run my test cases (see attached file in previous mail), I
> create/drop multiple times the same temp table in different
> transactions and I do not experience any blocking.
>> The fixed-size array of temp table oids is an unnecessary limitation.
>> A list or hash table would be better.
> You are right, I will fix that.
>>> Let me know what you think of the patch and if it could be applied
>>> to 8.3 and 8.4?
>> Not to 8.3. We only back-patch bug-fixes, and this isn't one.
> Ok understood.
>
> Thanks for your feedback and don't hesitate to enlighten me on the
> potential locking issue I did not understand.
> Emmanuel
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

On Tue, Oct 07, 2008 at 04:57:37PM -0400, Emmanuel Cecchet wrote:
> Heikki,
>
> Here is a new version of the patch using a hash table as you
> suggested. I also include the tests that I have added to the
> regression test suite to test the various scenarios. All patches
> are based on Postgres 8.3.3, let me know if you want me to generate
> patch for 8.4.

CVS TIP is the only place where new features, like this, go :)

I didn't see the attachment anyhow...

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hi,

> On Tue, Oct 07, 2008 at 04:57:37PM -0400, Emmanuel Cecchet wrote:
>
>> Heikki,
>>
>> Here is a new version of the patch using a hash table as you
>> suggested. I also include the tests that I have added to the
>> regression test suite to test the various scenarios. All patches
>> are based on Postgres 8.3.3, let me know if you want me to generate
>> patch for 8.4.
>>
>
> CVS TIP is the only place where new features, like this, go :)
>
I looked at the Wiki and it looks like I should add en entry (assuming I
get a patch for the current CVS HEAD) to CommitFest 2008-11. Is that
correct?
> I didn't see the attachment anyhow...
>
Good point! The same with the attachments now!

Thanks,
manu

On Tue, Oct 07, 2008 at 06:12:14PM -0400, Emmanuel Cecchet wrote:
> Hi,
>
>> On Tue, Oct 07, 2008 at 04:57:37PM -0400, Emmanuel Cecchet wrote:
>>
>>> Heikki,
>>>
>>> Here is a new version of the patch using a hash table as you
>>> suggested. I also include the tests that I have added to the
>>> regression test suite to test the various scenarios. All patches
>>> are based on Postgres 8.3.3, let me know if you want me to
>>> generate patch for 8.4.
>>
>> CVS TIP is the only place where new features, like this, go :)
>>
> I looked at the Wiki and it looks like I should add en entry
> (assuming I get a patch for the current CVS HEAD) to CommitFest
> 2008-11. Is that correct?
>
>> I didn't see the attachment anyhow...
>>
> Good point! The same with the attachments now!

Perhaps we did not make this clear. The patch is a new feature. New
features are not going into 8.3, as it has already been released.

Make a patch against CVS TIP aka 8.4, and re-submit.

Cheers,
David
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hi,

Here are the patches for 8.4 (1 patch for the code and 1 patch for the
regression tests).

Thanks in advance for your feedback,
Emmanuel

David Fetter wrote:
> On Tue, Oct 07, 2008 at 06:12:14PM -0400, Emmanuel Cecchet wrote:
>
>> Hi,
>>
>>
>>> On Tue, Oct 07, 2008 at 04:57:37PM -0400, Emmanuel Cecchet wrote:
>>>
>>>
>>>> Heikki,
>>>>
>>>> Here is a new version of the patch using a hash table as you
>>>> suggested. I also include the tests that I have added to the
>>>> regression test suite to test the various scenarios. All patches
>>>> are based on Postgres 8.3.3, let me know if you want me to
>>>> generate patch for 8.4.
>>>>
>>> CVS TIP is the only place where new features, like this, go :)
>>>
>>>
>> I looked at the Wiki and it looks like I should add en entry
>> (assuming I get a patch for the current CVS HEAD) to CommitFest
>> 2008-11. Is that correct?
>>
>>
>>> I didn't see the attachment anyhow...
>>>
>>>
>> Good point! The same with the attachments now!
>>
>
> Perhaps we did not make this clear. The patch is a new feature. New
> features are not going into 8.3, as it has already been released.
>
> Make a patch against CVS TIP aka 8.4, and re-submit.
>
> Cheers,
> David
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
>> Also, even if the table is created and dropped in the same
>> transaction, a subsequent transaction that tries to create and drop
>> the table gets blocked on the lock. I suppose we could just say that
>> that's the way it works, but I'm afraid it will come as a nasty
>> surprise, making the feature a lot less useful.
> I do not get that one, if the table is dropped in the transaction the
> lock is released. Why would another transaction be blocked when trying
> to create/drop another temp table?

I was thinking of a transaction that's just prepared (1st phase), but
not committed or rolled back:

postgres=# CREATE TEMP TABLE foo (bar int);
CREATE TABLE
postgres=# BEGIN;
BEGIN
postgres=# DROP TABLE foo;
DROP TABLE
postgres=# PREPARE TRANSACTION '2pc';
PREPARE TRANSACTION
postgres=# SELECT * FROM foo;
<blocks>

Furthermore, it looks like the backend refuses to shut down, even if you
end the psql session, because RemoveTempRelations() is called on backend
shutdown and it gets blocked on that lock.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> I was thinking of a transaction that's just prepared (1st phase), but
> not committed or rolled back:
>
> postgres=# CREATE TEMP TABLE foo (bar int);
> CREATE TABLE
> postgres=# BEGIN;
> BEGIN
> postgres=# DROP TABLE foo;
> DROP TABLE
> postgres=# PREPARE TRANSACTION '2pc';
> PREPARE TRANSACTION
> postgres=# SELECT * FROM foo;
> <blocks>
>
> Furthermore, it looks like the backend refuses to shut down, even if
> you end the psql session, because RemoveTempRelations() is called on
> backend shutdown and it gets blocked on that lock.
Thanks for the example, I get it now. Does it make sense to allow any
request execution between PREPARE TRANSACTION and the subsequent COMMIT
or ROLLBACK?
I did the same experiment with a regular table (not a temp table) and it
blocks exactly the same way, so I don't think that the problem is
specific to temp tables.
Once PREPARE has been executed, the transaction state is restored to
TRANS_DEFAULT, but I wonder if we should not have a specific
TRANS_PREPARED state in which we can only authorize a COMMIT or a
ROLLBACK. Is there any reasonable use case where someone would have to
execute requests between PREPARE and COMMIT/ROLLBACK?

Let me know what you think of the additional TRANS_PREPARED transaction
state. It looks like the behavior of what happens between PREPARE and
COMMIT/ROLLBACK is pretty much undefined.

Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet <manu(at)frogthinker(dot)org> writes:
> Thanks for the example, I get it now. Does it make sense to allow any
> request execution between PREPARE TRANSACTION and the subsequent COMMIT
> or ROLLBACK?

Yes. Don't even think of trying to disallow that. The COMMIT doesn't
even have to happen in the same session, anyway.

regards, tom lane

Tom Lane wrote:
> Emmanuel Cecchet <manu(at)frogthinker(dot)org> writes:
>
>> Thanks for the example, I get it now. Does it make sense to allow any
>> request execution between PREPARE TRANSACTION and the subsequent COMMIT
>> or ROLLBACK?
>>
>
> Yes. Don't even think of trying to disallow that. The COMMIT doesn't
> even have to happen in the same session, anyway.
>
Ok, so actually I don't see any different behavior between a temp table
or a regular table. The locking happens the same way and as long as the
commit prepared happens (potentially in another session), the lock is
released at commit time which seems to make sense.
The issue that Heikki was mentioning about the server not shutting down
seems to happen as soon as you have a single transaction that has
prepared commit but not commit/rollback yet. This seems also independent
of whether you are using a temp table or not.
It seems that the patch did not alter the behavior of PG in that regard.

What do you think?
Emmanuel

Emmanuel Cecchet <manu(at)frogthinker(dot)org> writes:
> Ok, so actually I don't see any different behavior between a temp table
> or a regular table. The locking happens the same way and as long as the
> commit prepared happens (potentially in another session), the lock is
> released at commit time which seems to make sense.

Right, the problem is that you can't shut down the original backend
because it'll try to drop the temp table at exit, and then block on
the lock that the prepared xact is holding. From a database management
standpoint that is unacceptable --- it means for instance that you can't
shut down the database cleanly while such a prepared transaction is
pending. The difference from a regular table is that no such automatic
action is taken at backend exit for regular tables.

The whole business of having restrictions on temp table access is
annoying; I wish we could get rid of them not add complexity to
enforcing them. The local-buffer-management end of the issue seems
readily solvable: we need only decree that PREPARE has to flush out any
dirty local buffers (and maybe discard local buffers altogether, not
sure). But I've not been able to see a decent solution to the lock
behavior.

regards, tom lane

Hi,

I am attaching a new patch that deals with the issue of the locks on
temporary tables that have been accessed in transactions that have been
prepared but not committed.
I have added another list that keeps track of temp tables accessed by
transactions that are prepared but not committed. The RemoveTempTable
callback does not try to acquire locks on these tables. Now postmaster
can terminate even with transactions in the prepared state that accessed
temp tables.

Let me know what you think.
manu

Tom Lane wrote:
> Emmanuel Cecchet <manu(at)frogthinker(dot)org> writes:
>
>> Ok, so actually I don't see any different behavior between a temp table
>> or a regular table. The locking happens the same way and as long as the
>> commit prepared happens (potentially in another session), the lock is
>> released at commit time which seems to make sense.
>>
>
> Right, the problem is that you can't shut down the original backend
> because it'll try to drop the temp table at exit, and then block on
> the lock that the prepared xact is holding. From a database management
> standpoint that is unacceptable --- it means for instance that you can't
> shut down the database cleanly while such a prepared transaction is
> pending. The difference from a regular table is that no such automatic
> action is taken at backend exit for regular tables.
>
> The whole business of having restrictions on temp table access is
> annoying; I wish we could get rid of them not add complexity to
> enforcing them. The local-buffer-management end of the issue seems
> readily solvable: we need only decree that PREPARE has to flush out any
> dirty local buffers (and maybe discard local buffers altogether, not
> sure). But I've not been able to see a decent solution to the lock
> behavior.
>
> regards, tom lane
>
>

Hi all,

Here is the latest patch and the regression tests for the temp tables
and 2PC issue.
Is there a way to stop and restart postmaster between 2 tests?

Thanks for your feedback,
Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
> Here is the latest patch and the regression tests for the temp tables
> and 2PC issue.
> Is there a way to stop and restart postmaster between 2 tests?
>
> Thanks for your feedback,
> Emmanuel
I did not get any comment on that one.
How should I proceed so that the patch integration can be considered for
8.4?

Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
> Emmanuel Cecchet wrote:
>> Here is the latest patch and the regression tests for the temp tables
>> and 2PC issue.
>> Is there a way to stop and restart postmaster between 2 tests?
>>
>> Thanks for your feedback,
>> Emmanuel
> I did not get any comment on that one.
> How should I proceed so that the patch integration can be considered for
> 8.4?

http://wiki.postgresql.org/wiki/Submitting_a_Patch

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Emmanuel Cecchet wrote:
> Here is the latest patch and the regression tests for the temp tables
> and 2PC issue.

This fails:

postgres=# begin;
BEGIN
postgres=# CREATE TEMPORARY TABLE temp1 (id int4);
CREATE TABLE
postgres=# PREPARE TRANSACTION 'foo';
PREPARE TRANSACTION
postgres=# CREATE TEMPORARY TABLE temp2 (id int4);
ERROR: cannot insert into frozen hashtable "accessed temp tables"

I don't understand the bookkeeping of accessed and prepared temp tables
in general. What's it for?

The comments on preparedTempRel says that it keeps track of "accessed
temporary relations that have been prepared commit but not committed
yet". That's never going to work as a backend-private hash table,
because there's no way to remove entries from it when the prepared
transaction is committed or rolled back from another backend.

What's the purpose of checking that a table is empty on prepare? I think
I'd feel more comfortable with the approach of only accepting PREPARE
TRANSACTIOn if the accessed temp tables have been created and destroyed
in the same transaction, to avoid possibly surprising behavior when a
temp table is kept locked by a prepared transaction and you try to drop
it later in the sesssion, but the patch allows more than that. I guess
accessing an existing ON COMMIT DELETE ROWS temp table would also be OK,
but checking that there's no visible rows in the table doesn't achieve
that.

I don't think you can just ignore "prepared temp relations" in
findDependentObjects to avoid the lockup at backend exit. It's also used
for DROP CASCADE, for example.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hi Heikki,

> Emmanuel Cecchet wrote:
>> Here is the latest patch and the regression tests for the temp tables
>> and 2PC issue.
>
> This fails:
>
> postgres=# begin;
> BEGIN
> postgres=# CREATE TEMPORARY TABLE temp1 (id int4);
> CREATE TABLE
> postgres=# PREPARE TRANSACTION 'foo';
> PREPARE TRANSACTION
> postgres=# CREATE TEMPORARY TABLE temp2 (id int4);
> ERROR: cannot insert into frozen hashtable "accessed temp tables"
I will address that.
> I don't understand the bookkeeping of accessed and prepared temp tables
> in general. What's it for?
Right now (in 8.3) the bookkeeping prevents a transaction that has used
a temp table to prepare commit. As you mentioned earlier
(http://archives.postgresql.org/pgsql-hackers/2008-02/msg01277.php) we
should be able to allow CREATE+DROP in the same transaction.
> The comments on preparedTempRel says that it keeps track of "accessed
> temporary relations that have been prepared commit but not committed
> yet". That's never going to work as a backend-private hash table,
> because there's no way to remove entries from it when the prepared
> transaction is committed or rolled back from another backend.
It does not really matter since we only allow empty temp tables at
prepared time. And the transaction can only be prepared locally. If the
transaction is committed or rolled back from another backend, the only
thing that can happen is that tables that were created in the
transaction will remain in the list. They will be ignored at the next
prepare since the relation will not exist anymore. Once again, the
tables remaining in the list after prepare are empty.
> What's the purpose of checking that a table is empty on prepare? I think
> I'd feel more comfortable with the approach of only accepting PREPARE
> TRANSACTIOn if the accessed temp tables have been created and destroyed
> in the same transaction, to avoid possibly surprising behavior when a
> temp table is kept locked by a prepared transaction and you try to drop
> it later in the sesssion, but the patch allows more than that. I guess
> accessing an existing ON COMMIT DELETE ROWS temp table would also be OK,
Yes, I was trying to allow also ON COMMIT DROP and ON COMMIT DELETE ROW.
An empty temp table at PREPARE time would be similar to an ON COMMIT
DELETE ROW table.
> but checking that there's no visible rows in the table doesn't achieve
> that.
If the relation exist but contains no row, is it possible that the table
is not empty? What would I need to do to ensure that the table is empty?
> I don't think you can just ignore "prepared temp relations" in
> findDependentObjects to avoid the lockup at backend exit. It's also used
> for DROP CASCADE, for example.
Do you mean that it will break the DROP CASCADE behavior in general, or
that would break the behavior for master/child temp tables? By the way,
does Postgres support child temp tables?

Thanks for the feedback. I will address the problem of the frozen hash
list but let me know what you think of the other potential issues.

Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
>> What's the purpose of checking that a table is empty on prepare? I think
>> I'd feel more comfortable with the approach of only accepting PREPARE
>> TRANSACTIOn if the accessed temp tables have been created and destroyed
>> in the same transaction, to avoid possibly surprising behavior when a
>> temp table is kept locked by a prepared transaction and you try to drop
>> it later in the sesssion, but the patch allows more than that. I guess
>> accessing an existing ON COMMIT DELETE ROWS temp table would also be OK,
> Yes, I was trying to allow also ON COMMIT DROP and ON COMMIT DELETE ROW.
> An empty temp table at PREPARE time would be similar to an ON COMMIT
> DELETE ROW table.

I think you'll want to check explicitly that the table is defined with
ON COMMIT DELETE ROWS, instead of checking that it's empty.

>> but checking that there's no visible rows in the table doesn't achieve
>> that.
> If the relation exist but contains no row, is it possible that the table
> is not empty? What would I need to do to ensure that the table is empty?

Yeah, thanks to MVCC, it's possible that the table looks empty to the
transaction being prepared, using SnapshotNow, but there's some tuples
that are still visible to other transactions. For example:

CREATE TEMPORARY TABLE foo (id int4);
INSERT INTO foo VALUES (1);
begin;
DELETE FROM foo;
PREPARE TRANSACTION 'foo'; -- doesn't error, because the table is empty,
according to SnapshotNow
SELECT * FROM foo; -- Still shows the one row, because the deleting
transaction hasn't committed yet.

>> I don't think you can just ignore "prepared temp relations" in
>> findDependentObjects to avoid the lockup at backend exit. It's also used
>> for DROP CASCADE, for example.
> Do you mean that it will break the DROP CASCADE behavior in general, or
> that would break the behavior for master/child temp tables?

For temp tables, I suppose.

The hack in findDependentObjects still isn't enough, anyway. If you have
a prepared transaction that created a temp table, the database doesn't
shut down:

$ bin/pg_ctl -D data start
server starting
$ LOG: database system was shut down at 2008-11-04 10:27:27 EST
LOG: autovacuum launcher started
LOG: database system is ready to accept connections

$ bin/psql postgres -c "begin; CREATE TEMPORARY TABLE temp (id integer);
PREPARE TRANSACTION 'foo';"
PREPARE TRANSACTION
hlinnaka(at)heikkilaptop:~/pgsql.fsmfork$ bin/pg_ctl -D data stop
LOG: received smart shutdown request
LOG: autovacuum launcher shutting down
waiting for server to shut
down............................................................... failed
pg_ctl: server does not shut down

> By the way,
> does Postgres support child temp tables?

Yes.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
>> Yes, I was trying to allow also ON COMMIT DROP and ON COMMIT DELETE
>> ROW. An empty temp table at PREPARE time would be similar to an ON
>> COMMIT DELETE ROW table.
> I think you'll want to check explicitly that the table is defined with
> ON COMMIT DELETE ROWS, instead of checking that it's empty.
Where can I find the field containing the CREATE options for the temp table?
> Yeah, thanks to MVCC, it's possible that the table looks empty to the
> transaction being prepared, using SnapshotNow, but there's some tuples
> that are still visible to other transactions. For example:
>
> CREATE TEMPORARY TABLE foo (id int4);
> INSERT INTO foo VALUES (1);
> begin;
> DELETE FROM foo;
> PREPARE TRANSACTION 'foo'; -- doesn't error, because the table is
> empty, according to SnapshotNow
> SELECT * FROM foo; -- Still shows the one row, because the deleting
> transaction hasn't committed yet.
Is that a problem? If your transaction isolation level is not
serializable the SELECT will not block and return the current snapshot.
From the transaction standpoint, it is fine that the transaction can
prepare or am I missing something?
Actually, I did a test and if the temp table is created with 'on commit
delete rows' option, the select blocks until the transaction is
committed. This seems a normal behavior to me.
>>> I don't think you can just ignore "prepared temp relations" in
>>> findDependentObjects to avoid the lockup at backend exit. It's also
>>> used
>>> for DROP CASCADE, for example.
>> Do you mean that it will break the DROP CASCADE behavior in general,
>> or that would break the behavior for master/child temp tables?
>
> For temp tables, I suppose.
I confirm that doing a drop cascade on a master temp table after a
prepared transaction committed from another backend will not drop the
children for now.
>
> The hack in findDependentObjects still isn't enough, anyway. If you
> have a prepared transaction that created a temp table, the database
> doesn't shut down:
>
> $ bin/pg_ctl -D data start
> server starting
> $ LOG: database system was shut down at 2008-11-04 10:27:27 EST
> LOG: autovacuum launcher started
> LOG: database system is ready to accept connections
>
> $ bin/psql postgres -c "begin; CREATE TEMPORARY TABLE temp (id
> integer); PREPARE TRANSACTION 'foo';"
> PREPARE TRANSACTION
> hlinnaka(at)heikkilaptop:~/pgsql.fsmfork$ bin/pg_ctl -D data stop
> LOG: received smart shutdown request
> LOG: autovacuum launcher shutting down
> waiting for server to shut
> down...............................................................
> failed
> pg_ctl: server does not shut down
Interesting case, if the table is created but not accessed it is not
enlisted and then the shutdown does not catch this dependency. The table
should be enlisted at CREATE time as well.

The bookkeeping of prepared commit tables is just for the shutdown case
right now. If you think it is a bad idea altogether to have session temp
tables (even with delete rows on commit) that can cross commit
boundaries, then we can remove that second bookkeeping and only allow
temp tables that have been created withing the scope of the transaction.

I fixed the hash_freeze problem but this drop cascade on temp table
seems to be an issue (if anyone uses that feature).

Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Hi,

As I have not found yet an elegant solution to deal with the DROP
CASCADE issue, here is a simpler patch that handles temp tables that are
dropped at commit time. This is a good first step and we will try to
elaborate further to support ON COMMIT DELETE ROWS.
I have also added a compilation of the tests I have even if some are not
really relevant anymore without the support for empty temp tables but we
will probably reuse them later.

Thanks in advance for the feedback,
Emmanuel

Emmanuel Cecchet wrote:
> Heikki Linnakangas wrote:
>>> Yes, I was trying to allow also ON COMMIT DROP and ON COMMIT DELETE
>>> ROW. An empty temp table at PREPARE time would be similar to an ON
>>> COMMIT DELETE ROW table.
>> I think you'll want to check explicitly that the table is defined
>> with ON COMMIT DELETE ROWS, instead of checking that it's empty.
> Where can I find the field containing the CREATE options for the temp
> table?
>> Yeah, thanks to MVCC, it's possible that the table looks empty to the
>> transaction being prepared, using SnapshotNow, but there's some
>> tuples that are still visible to other transactions. For example:
>>
>> CREATE TEMPORARY TABLE foo (id int4);
>> INSERT INTO foo VALUES (1);
>> begin;
>> DELETE FROM foo;
>> PREPARE TRANSACTION 'foo'; -- doesn't error, because the table is
>> empty, according to SnapshotNow
>> SELECT * FROM foo; -- Still shows the one row, because the deleting
>> transaction hasn't committed yet.
> Is that a problem? If your transaction isolation level is not
> serializable the SELECT will not block and return the current
> snapshot. From the transaction standpoint, it is fine that the
> transaction can prepare or am I missing something?
> Actually, I did a test and if the temp table is created with 'on
> commit delete rows' option, the select blocks until the transaction is
> committed. This seems a normal behavior to me.
>>>> I don't think you can just ignore "prepared temp relations" in
>>>> findDependentObjects to avoid the lockup at backend exit. It's also
>>>> used
>>>> for DROP CASCADE, for example.
>>> Do you mean that it will break the DROP CASCADE behavior in general,
>>> or that would break the behavior for master/child temp tables?
>>
>> For temp tables, I suppose.
> I confirm that doing a drop cascade on a master temp table after a
> prepared transaction committed from another backend will not drop the
> children for now.
>>
>> The hack in findDependentObjects still isn't enough, anyway. If you
>> have a prepared transaction that created a temp table, the database
>> doesn't shut down:
>>
>> $ bin/pg_ctl -D data start
>> server starting
>> $ LOG: database system was shut down at 2008-11-04 10:27:27 EST
>> LOG: autovacuum launcher started
>> LOG: database system is ready to accept connections
>>
>> $ bin/psql postgres -c "begin; CREATE TEMPORARY TABLE temp (id
>> integer); PREPARE TRANSACTION 'foo';"
>> PREPARE TRANSACTION
>> hlinnaka(at)heikkilaptop:~/pgsql.fsmfork$ bin/pg_ctl -D data stop
>> LOG: received smart shutdown request
>> LOG: autovacuum launcher shutting down
>> waiting for server to shut
>> down...............................................................
>> failed
>> pg_ctl: server does not shut down
> Interesting case, if the table is created but not accessed it is not
> enlisted and then the shutdown does not catch this dependency. The
> table should be enlisted at CREATE time as well.
>
> The bookkeeping of prepared commit tables is just for the shutdown
> case right now. If you think it is a bad idea altogether to have
> session temp tables (even with delete rows on commit) that can cross
> commit boundaries, then we can remove that second bookkeeping and only
> allow temp tables that have been created withing the scope of the
> transaction.
>
> I fixed the hash_freeze problem but this drop cascade on temp table
> seems to be an issue (if anyone uses that feature).
>
> Emmanuel
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
> As I have not found yet an elegant solution to deal with the DROP
> CASCADE issue, here is a simpler patch that handles temp tables that are
> dropped at commit time. This is a good first step and we will try to
> elaborate further to support ON COMMIT DELETE ROWS.

The problem with stopping the postmaster seems to still be there..

All the problems are centered around locking. We need to address that
and decide what is sane locking behavior wrt. temp tables and 2PC.

First, there's the case where a temp table is created and dropped in the
same transaction. It seems perfectly sane to me to simply drop all locks
on the dropped table at PREPARE TRANSACTION. Does anyone see a problem
with that? If not, we might as well do that for all tables, not just
temporary ones. It seems just as safe for non-temporary tables.

Secondly, there's the case of accessing a ON COMMIT DELETE ROWS table.
There too, could we simply drop the locks at PREPARE TRANSACTION? I
can't immediately see anything wrong with that.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hi Heikki,
> Emmanuel Cecchet wrote:
>> As I have not found yet an elegant solution to deal with the DROP
>> CASCADE issue, here is a simpler patch that handles temp tables that
>> are dropped at commit time. This is a good first step and we will try
>> to elaborate further to support ON COMMIT DELETE ROWS.
>
> The problem with stopping the postmaster seems to still be there..
>
> All the problems are centered around locking. We need to address that
> and decide what is sane locking behavior wrt. temp tables and 2PC.
>
> First, there's the case where a temp table is created and dropped in
> the same transaction. It seems perfectly sane to me to simply drop all
> locks on the dropped table at PREPARE TRANSACTION. Does anyone see a
> problem with that? If not, we might as well do that for all tables,
> not just temporary ones. It seems just as safe for non-temporary tables.
This seems good to me. Any access to the table after PREPARE TRANSACTION
but before COMMIT on that backend would return a relation not found
which is what we expect. For a regular table, I don't know if that
makes a difference if the prepared transaction rollbacks?
> Secondly, there's the case of accessing a ON COMMIT DELETE ROWS table.
> There too, could we simply drop the locks at PREPARE TRANSACTION? I
> can't immediately see anything wrong with that.
As there is no data anyway, I don't think the locks are going to change
anything. But in the most recent stripped-down version of the patch, on
delete rows is no more supported, I only allow on commit drop. I did not
find the flag to see if a temp table was created with the on delete rows
option.

Do you want me to look at the locking code or will you have time to do
it? Hints will be welcome if you want me to do it.

Thanks,
Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
>> Emmanuel Cecchet wrote:
>>> As I have not found yet an elegant solution to deal with the DROP
>>> CASCADE issue, here is a simpler patch that handles temp tables that
>>> are dropped at commit time. This is a good first step and we will try
>>> to elaborate further to support ON COMMIT DELETE ROWS.
>>
>> The problem with stopping the postmaster seems to still be there..
>>
>> All the problems are centered around locking. We need to address that
>> and decide what is sane locking behavior wrt. temp tables and 2PC.
>>
>> First, there's the case where a temp table is created and dropped in
>> the same transaction. It seems perfectly sane to me to simply drop all
>> locks on the dropped table at PREPARE TRANSACTION. Does anyone see a
>> problem with that? If not, we might as well do that for all tables,
>> not just temporary ones. It seems just as safe for non-temporary tables.
> This seems good to me. Any access to the table after PREPARE TRANSACTION
> but before COMMIT on that backend would return a relation not found
> which is what we expect. For a regular table, I don't know if that
> makes a difference if the prepared transaction rollbacks?

I don't think there's any difference with temp tables and regular ones
from locking point of view.

>> Secondly, there's the case of accessing a ON COMMIT DELETE ROWS table.
>> There too, could we simply drop the locks at PREPARE TRANSACTION? I
>> can't immediately see anything wrong with that.
> As there is no data anyway, I don't think the locks are going to change
> anything. But in the most recent stripped-down version of the patch, on
> delete rows is no more supported, I only allow on commit drop. I did not
> find the flag to see if a temp table was created with the on delete rows
> option.

Hmm. I think we can use the on_commits list in tablecmds.c for that.

> Do you want me to look at the locking code or will you have time to do
> it? Hints will be welcome if you want me to do it.

I can give it a shot for change. Attached is a patch that allows the ON
COMMIT DELETE ROWS case. The beef of the patch is:

- An entry is made into the on_commits list in tablecmds.c for all temp
tables, even if there's no ON COMMIT action
- There's a new function, check_prepare_safe_temp_table(Oid relid) in
tablecmds.c, that uses the on_commits list to determine if the access to
the given relation is "PREPARE-safe". That is, it's not a temp table, or
it's an access to an ON COMMIT DELETE ROWS temp table and the temp table
wasn't created or dropped in the same transaction.
- MyXactMadeTempRelUpdate variable is gone. The check is driven from the
lock manager again, like it was in 8.1, by calling the new
check_prepare_sage_temp_table function for all relation locks in
AtPrepare_Locks().
- changed the on_commits linked list in tablecmds.c into a hash table
for performance

Somehow this feels pretty baroque, though. Perhaps a better approach
would be to add a new AtPrepare_OnCommitActions function to tablecmds.c,
that gets called before AtPrepare_Locks. It would scan through the
on_commits list, and release all locks for the "PREPARE-safe" temp
tables, and throw the error if necessary. I'll try that next.

BTW, there's a very relevant thread here:

http://archives.postgresql.org/pgsql-hackers/2008-03/msg00063.php

if you haven't read it already.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Somehow this feels pretty baroque, though. Perhaps a better approach
> would be to add a new AtPrepare_OnCommitActions function to tablecmds.c,
> that gets called before AtPrepare_Locks. It would scan through the
> on_commits list, and release all locks for the "PREPARE-safe" temp
> tables, and throw the error if necessary. I'll try that next.

Here's what I ended up with. I morphed the on commit action registration
into tracking of all temporary relations.

This only allows access to ON COMMIT DELETE ROWS temp tables. Accessing
other temporary tables, and creating or dropping tables in the
transaction is still forbidden.

It took me a couple of iterations to handle toast tables and indexes
correctly. More testing would be appreciated with more complex cases
like VACUUM FULL, subtransactions etc.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hi Heikki,

I will try to make more tests.
I still quite did not get what the big deal was if an ON COMMIT DELETE
ROWS temp table was created inside a transaction.
Why the new checks you are doing in lock.c would not work with dropped
temp tables? Could it be possible to drop the lock as soon as the temp
table is dropped inside a transaction?

I will try to find more time to review the patch tonight.

Best,
Emmanuel

> Heikki Linnakangas wrote:
>> Somehow this feels pretty baroque, though. Perhaps a better approach
>> would be to add a new AtPrepare_OnCommitActions function to
>> tablecmds.c, that gets called before AtPrepare_Locks. It would scan
>> through the on_commits list, and release all locks for the
>> "PREPARE-safe" temp tables, and throw the error if necessary. I'll
>> try that next.
>
> Here's what I ended up with. I morphed the on commit action
> registration into tracking of all temporary relations.
>
> This only allows access to ON COMMIT DELETE ROWS temp tables.
> Accessing other temporary tables, and creating or dropping tables in
> the transaction is still forbidden.
>
> It took me a couple of iterations to handle toast tables and indexes
> correctly. More testing would be appreciated with more complex cases
> like VACUUM FULL, subtransactions etc.
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
> I still quite did not get what the big deal was if an ON COMMIT DELETE
> ROWS temp table was created inside a transaction.

In case the transaction that created a temp table rolls back, the table
needs to be removed. Removing a temporary table belonging to another
backend is problematic; the local buffers in the original backend need
to be dropped, as well as the entry in the on commit actions list.

> Why the new checks you are doing in lock.c would not work with dropped
> temp tables? Could it be possible to drop the lock as soon as the temp
> table is dropped inside a transaction?

If you release the lock early on a table that you drop, another
transactions would be free to access the table, even though it's about
to be dropped.

> I will try to find more time to review the patch tonight.

Thanks!

Thinking about this whole thing yet more, I wonder if we could have a
more holistic approach and make temporary tables work just like regular
ones. The problems we've identified this far are:

1. If the prepared transaction keeps the temp table locked, the backend
can't exit, because the shutdown hook tries to drop all temp tables.

2. When a prepared transaction that has deleted a temporary table
commits (or one that created one aborts), we need to drop all the local
buffers from the original backend's private buffer cache.

3. When a prepared transaction that has deleted a temporary table
commits (or one that created one aborts), we need to remove the
on-commit entry from the original backend's private list.

Is there more? I think we can solve all the above problems:

1. Modify RemoveTempRelations so that it doesn't block if it can't
immediately acquire lock on the to-be-removed object. That way the
original backend can exit even if a prepared transaction is holding a
lock on a temporary object.

To avoid conflict with a new backend that's assigned the same backendid,
divorce the temporary namespace naming from backendid so that the
temporary namespace name stays reserved for the prepared transaction.

2. Flush and drop all local buffers on temporary tables that have been
created or dropped in the transaction at PREPARE TRANSACTION already.

3. Add on-commit field to pg_class, and only keep a list of temporary
tables that have been accessed in the current transaction in
backend-private memory.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Emmanuel Cecchet wrote:
>> I still quite did not get what the big deal was if an ON COMMIT
>> DELETE ROWS temp table was created inside a transaction.
>
> In case the transaction that created a temp table rolls back, the
> table needs to be removed. Removing a temporary table belonging to
> another backend is problematic; the local buffers in the original
> backend need to be dropped, as well as the entry in the on commit
> actions list.
>
>> Why the new checks you are doing in lock.c would not work with
>> dropped temp tables? Could it be possible to drop the lock as soon as
>> the temp table is dropped inside a transaction?
>
> If you release the lock early on a table that you drop, another
> transactions would be free to access the table, even though it's about
> to be dropped.
>
>> I will try to find more time to review the patch tonight.
>
> Thanks!
>
> Thinking about this whole thing yet more, I wonder if we could have a
> more holistic approach and make temporary tables work just like
> regular ones. The problems we've identified this far are:
>
> 1. If the prepared transaction keeps the temp table locked, the
> backend can't exit, because the shutdown hook tries to drop all temp
> tables.
>
> 2. When a prepared transaction that has deleted a temporary table
> commits (or one that created one aborts), we need to drop all the
> local buffers from the original backend's private buffer cache.
>
> 3. When a prepared transaction that has deleted a temporary table
> commits (or one that created one aborts), we need to remove the
> on-commit entry from the original backend's private list.
>
> Is there more? I think we can solve all the above problems:
>
> 1. Modify RemoveTempRelations so that it doesn't block if it can't
> immediately acquire lock on the to-be-removed object. That way the
> original backend can exit even if a prepared transaction is holding a
> lock on a temporary object.
>
> To avoid conflict with a new backend that's assigned the same
> backendid, divorce the temporary namespace naming from backendid so
> that the temporary namespace name stays reserved for the prepared
> transaction.
Is that going to cause any problem with DROP CASCADE operations or
trying to later drop a child table if the parent table is locked? I did
hit that issue when I tried to modify RemoveTempRelations but I was
probably not very smart at it.
> 2. Flush and drop all local buffers on temporary tables that have been
> created or dropped in the transaction at PREPARE TRANSACTION already.
Would there be any issue if someone was trying to use a READ_UNCOMMITTED
isolation level to access the temp table data?
> 3. Add on-commit field to pg_class, and only keep a list of temporary
> tables that have been accessed in the current transaction in
> backend-private memory.
Yes, this seems doable. We will certainly have to keep a list per
transaction id in case multiple prepared but uncommitted transactions
have accessed different temp table on that backend.

Have you already started to code some of this?
I am looking at adding new tests to check all cases including all types
of temp tables (normal/on delete/on drop, created inside or outside a
transaction, prepare/postmaster stop/commit/rollback,
inherit/index/vaccuum). That should get all the use cases covered.

Let me know what you think.
Thanks,
Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Heikki,

The following test fails with your patch on my system. Could you check
if you can reproduce?

psql (8.4devel)
Type "help" for help.

test=# begin;
BEGIN
test=# create table paul(x int);
CREATE TABLE
test=# insert into paul values(1);
INSERT 0 1
test=# prepare transaction 'persistentTableShouldSucceed';
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

---

LOG: database system is ready to accept connections
TRAP: FailedAssertion("!(on_commits != ((void *)0))", File:
"tablecmds.c", Line: 7823)
LOG: server process (PID 15969) was terminated by signal 6: Aborted
LOG: terminating any other active server processes
FATAL: the database system is in recovery mode

Thanks,
manu

Heikki,

I think that the Assert in is_temp_rel(Oid) in tablecmds.c should be
replaced by if (on_commits == NULL) return false;
As the use case below shows, a regular table can be created and hold a
LOCKTAG_RELATION lock that will trigger the call to is_temp_rel in
is_preparable_locktag. The assert will break if no temp table was accessed.

As we were also trying to list potential issues, if the temp table uses
a SERIAL type, will there be potentially a problem with the sequence at
prepare time?

Emmanuel

> The following test fails with your patch on my system. Could you check
> if you can reproduce?
>
> psql (8.4devel)
> Type "help" for help.
>
> test=# begin;
> BEGIN
> test=# create table paul(x int);
> CREATE TABLE
> test=# insert into paul values(1);
> INSERT 0 1
> test=# prepare transaction 'persistentTableShouldSucceed';
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> ---
>
> LOG: database system is ready to accept connections
> TRAP: FailedAssertion("!(on_commits != ((void *)0))", File:
> "tablecmds.c", Line: 7823)
> LOG: server process (PID 15969) was terminated by signal 6: Aborted
> LOG: terminating any other active server processes
> FATAL: the database system is in recovery mode
>
>
> Thanks,
> manu
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Heikki,

I have extended the patch to allow temp tables that have been
created/dropped within the same transaction (and also on commit drop).
There is a problem with temp tables with on delete rows that are created
inside a transaction.
Take the 2pc_on_delete_rows_transaction.sql test case and change the
creation statement, instead of
create temp table foo(x int) on commit delete rows;
try
create temp table foo(x serial primary key) on commit delete rows;

The test will fail. It looks like the onCommit field is not properly
updated when serial or primary key is used in that context. I did not
figure out why.

Waiting for your feedback
Emmanuel

Emmanuel Cecchet wrote:
> I think that the Assert in is_temp_rel(Oid) in tablecmds.c should be
> replaced by if (on_commits == NULL) return false;
> As the use case below shows, a regular table can be created and hold a
> LOCKTAG_RELATION lock that will trigger the call to is_temp_rel in
> is_preparable_locktag. The assert will break if no temp table was
> accessed.
>
> As we were also trying to list potential issues, if the temp table
> uses a SERIAL type, will there be potentially a problem with the
> sequence at prepare time?
>
> Emmanuel
>
>
>> The following test fails with your patch on my system. Could you
>> check if you can reproduce?
>>
>> psql (8.4devel)
>> Type "help" for help.
>>
>> test=# begin;
>> BEGIN
>> test=# create table paul(x int);
>> CREATE TABLE
>> test=# insert into paul values(1);
>> INSERT 0 1
>> test=# prepare transaction 'persistentTableShouldSucceed';
>> server closed the connection unexpectedly
>> This probably means the server terminated abnormally
>> before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> ---
>>
>> LOG: database system is ready to accept connections
>> TRAP: FailedAssertion("!(on_commits != ((void *)0))", File:
>> "tablecmds.c", Line: 7823)
>> LOG: server process (PID 15969) was terminated by signal 6: Aborted
>> LOG: terminating any other active server processes
>> FATAL: the database system is in recovery mode
>>
>>
>> Thanks,
>> manu
>>
>
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet wrote:
> I think that the Assert in is_temp_rel(Oid) in tablecmds.c should be
> replaced by if (on_commits == NULL) return false;
> As the use case below shows, a regular table can be created and hold a
> LOCKTAG_RELATION lock that will trigger the call to is_temp_rel in
> is_preparable_locktag. The assert will break if no temp table was accessed.

Yes, you're right.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Emmanuel Cecchet wrote:
> There is a problem with temp tables with on delete rows that are created
> inside a transaction.
> Take the 2pc_on_delete_rows_transaction.sql test case and change the
> creation statement, instead of
> create temp table foo(x int) on commit delete rows;
> try
> create temp table foo(x serial primary key) on commit delete rows;
>
> The test will fail. It looks like the onCommit field is not properly
> updated when serial or primary key is used in that context. I did not
> figure out why.

A serial column uses a sequence behind the scenes.

Hmm. Seems like we would need to treat sequences and indexes the same as
tables with ON COMMIT DELETE ROWS, i.e release the locks early and don't
error out.

All in all, this is getting pretty messy. My patch felt a bit hackish to
begin with, and having to add special cases for sequences and indexes
would make it even more so. And what about temporary views? I'm starting
to feel that instead of special-casing temp relations, we need to move
into the opposite direction and make temp relations more like regular
relations. Unfortunately, that's not going to happen in the 8.4
timeframe :-(. Let's try the other approach in 8.5.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

I would really like to have support for temp tables at least for the
case where the table is created and dropped in the same transaction. But
I guess that the other limitations on index, sequences and views would
still hold, right?

manu

Heikki Linnakangas wrote:
> Emmanuel Cecchet wrote:
>> There is a problem with temp tables with on delete rows that are
>> created inside a transaction.
>> Take the 2pc_on_delete_rows_transaction.sql test case and change the
>> creation statement, instead of
>> create temp table foo(x int) on commit delete rows;
>> try
>> create temp table foo(x serial primary key) on commit delete rows;
>>
>> The test will fail. It looks like the onCommit field is not properly
>> updated when serial or primary key is used in that context. I did not
>> figure out why.
> A serial column uses a sequence behind the scenes.
>
> Hmm. Seems like we would need to treat sequences and indexes the same
> as tables with ON COMMIT DELETE ROWS, i.e release the locks early and
> don't error out.
>
> All in all, this is getting pretty messy. My patch felt a bit hackish
> to begin with, and having to add special cases for sequences and
> indexes would make it even more so. And what about temporary views?
> I'm starting to feel that instead of special-casing temp relations, we
> need to move into the opposite direction and make temp relations more
> like regular relations. Unfortunately, that's not going to happen in
> the 8.4 timeframe :-(. Let's try the other approach in 8.5.
>

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Hi all,

Here is a new version of the patch still based on the last proposal by
Heikki.
This patch only support temp tables that are created AND dropped in the
same transaction. This works with any kind of temp table (with on commit
options or not).
This simplifies the problem since the temp tables we consider here
cannot be accessed by another backend anyway since their scope is the
one of the transaction.
We do NOT allow:
- access to temp tables that were not created AND dropped in the transaction
- created temp tables that are not dropped at prepare time
- dropped temp tables that were not created in the transaction

The attached patch has a number of tests for all these cases including
sequences and index on temp tables.

Let me know what you think
Emmanuel

--
Emmanuel Cecchet
FTO @ Frog Thinker
Open Source Development & Consulting
--
Web: http://www.frogthinker.org
email: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet