ON COMMIT temp table handling

Bruce Momjian <pgman@candle.pha.pa.us> writes:

+ 	if ((iscommit && bstate != TBLOCK_END) ||
+ 		(!iscommit && bstate != TBLOCK_ABORT))
+ 		return;

Why is temp table handling in need of looking into xact.c's private
state? There is no other AtEOXact routine anywhere that does this.
ISTM either the above code is wrong, or every other AtEOXact routine
is wrong.

There are some other things I don't like about the patch, but I can
fix them myself. This one I thought I'd better ask about.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

+ 	if ((iscommit && bstate != TBLOCK_END) ||
+ 		(!iscommit && bstate != TBLOCK_ABORT))
+ 		return;

Why is temp table handling in need of looking into xact.c's private
state? There is no other AtEOXact routine anywhere that does this.
ISTM either the above code is wrong, or every other AtEOXact routine
is wrong.

There are some other things I don't like about the patch, but I can
fix them myself. This one I thought I'd better ask about.

I looked at that. The basic issue is that Gavin wants special handling
for ON COMMIT, meaning he only wants to activate the ON COMMIT code when
the transaction is committed and it is an end block, or it is not a
commit but it is an abort.

What I don't understand is how he would get into this function unless
either is true. The two call locations would seem to be in transaction
commit/abort routines. I have to say I am confused by the various
TBLOCK values and their progression. I can't find any comments on them
and the code seems contorted.

Tom, can you guess on why they are there? I hear Gavin is traveling in
Europe for the next week or two.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Why is temp table handling in need of looking into xact.c's private
state? There is no other AtEOXact routine anywhere that does this.
ISTM either the above code is wrong, or every other AtEOXact routine
is wrong.

I looked at that. The basic issue is that Gavin wants special handling
for ON COMMIT, meaning he only wants to activate the ON COMMIT code when
the transaction is committed and it is an end block, or it is not a
commit but it is an abort.

I eventually realized that the problem is that his AtEOXact routine
needs to be split in two pieces. The actual ON COMMIT DROP or DELETE
ROWS action has to happen *before* we record transaction commit.
(Imagine what happens if we get an error while doing that part.)
But the cleanup of the list of on-commit objects should happen
afterwards. I think if you look at the version I committed this
afternoon, you'll be much happier.

I have to say I am confused by the various
TBLOCK values and their progression. I can't find any comments on them
and the code seems contorted.

I think xact.c has more generality than it actually needs --- some of
the TBLOCK states could probably be eliminated. I'm not really excited
about rewriting it though; it works and has worked for years.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Why is temp table handling in need of looking into xact.c's private
state? There is no other AtEOXact routine anywhere that does this.
ISTM either the above code is wrong, or every other AtEOXact routine
is wrong.

I looked at that. The basic issue is that Gavin wants special handling
for ON COMMIT, meaning he only wants to activate the ON COMMIT code when
the transaction is committed and it is an end block, or it is not a
commit but it is an abort.

I eventually realized that the problem is that his AtEOXact routine
needs to be split in two pieces. The actual ON COMMIT DROP or DELETE
ROWS action has to happen *before* we record transaction commit.
(Imagine what happens if we get an error while doing that part.)
But the cleanup of the list of on-commit objects should happen
afterwards. I think if you look at the version I committed this
afternoon, you'll be much happier.

Now, that makes sense.

I have to say I am confused by the various
TBLOCK values and their progression. I can't find any comments on them
and the code seems contorted.

I think xact.c has more generality than it actually needs --- some of
the TBLOCK states could probably be eliminated. I'm not really excited
about rewriting it though; it works and has worked for years.

Yep, over generalized is the word for it. Let me see if I can add some
comments.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Thanks Bruce,

I've been in France/Germany and haven't had internet access :-(. Hence
slow response, below.

On Sat, 9 Nov 2002, Bruce Momjian wrote:

o The code has drifted since last August, so I had to apply
some of it by hand.

Thanks.

o Your code in cluster.c called heap_create_with_catalog() using:

isTempNamespace(RelationGetNamespace(OldHeap)),

which returns a boolean, and passes it as the value for 'ateoxact',
which is not a boolean. This call is used to create the new heap file
for the cluster operation. I changed it to ATEOXACTNOOP.

I haven't looked at the patch because its a bit chaotic here after getting
back, but it occurs to me that if the cluster is on a temp table we want
to tell heap_create_with_catalog().

o Your handling of the temprels List was incorrect. You can't use
foreach() to traverse a List when you are removing entries from the
list. (The loop variable pointer is removed and causes havoc.) I
changed the loop to while() and added the necessary loop control
statements. I thought you used my temprel.c code for this. It had the
proper control statements. Did you remove them by mistake:

http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/utils/cache/Attic/temprel.c

I must have. Oops.

Gavin

Gavin Sherry wrote:

o Your code in cluster.c called heap_create_with_catalog() using:

isTempNamespace(RelationGetNamespace(OldHeap)),

which returns a boolean, and passes it as the value for 'ateoxact',
which is not a boolean. This call is used to create the new heap file
for the cluster operation. I changed it to ATEOXACTNOOP.

I haven't looked at the patch because its a bit chaotic here after getting
back, but it occurs to me that if the cluster is on a temp table we want
to tell heap_create_with_catalog().

The flag we being passed for the creation of the new heap, which was
going to be deleted anyway when the transaction finished. Why bother
passing anything special for it?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

The flag we being passed for the creation of the new heap, which was
going to be deleted anyway when the transaction finished. Why bother
passing anything special for it?

I see. I should have looked at the code itself. Sorry.

Gavin

Gavin Sherry wrote:

The flag we being passed for the creation of the new heap, which was
going to be deleted anyway when the transaction finished. Why bother
passing anything special for it?

I see. I should have looked at the code itself. Sorry.

The problem is that the flag to heap_create_with_catalog now has so many
possible values that it isn't easy to find the value that exists for
the table being clustered, and because the temp heap goes away anyway
after the command, it isn't worth trying to figure out what it is.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073