Thread: [PATCH] PREPARE TRANSACTION unexpected behavior with TEMP TABLE

On Tue, Apr 6, 2021 at 8:18 PM Himanshu Upadhyaya
<upadhyaya.himanshu@gmail.com> wrote:
>
> Hi,
>
> While working on one of the issue, I have noticed below unexpected behavior with "PREPARE TRANSACTION".
>
> We are getting this unexpected behavior with PREPARE TRANSACTION when it is mixed with Temporary Objects. Please
considerthe below setup and SQL block. 
>
> set max_prepared_transactions to 1 (or any non zero value), this is to enable the “prepare transaction”.
>
> Now please try to run the below set of statements.
> [BLOCK-1:]
> postgres=# create temp table fullname (first text, last text);
> CREATE TABLE
> postgres=# BEGIN;
> BEGIN
> postgres=*# create function longname(fullname) returns text language sql
> postgres-*# as $$select $1.first || ' ' || $1.last$$;
> CREATE FUNCTION
> postgres=*# prepare transaction 'mytran';
> ERROR:  cannot PREPARE a transaction that has operated on temporary objects
>
> Above error is expected.
>
> The problem is if we again try to create the same function in the “PREPARE TRANSACTION” as below.
>
> [BLOCK-2:]
> postgres=# BEGIN;
> BEGIN
> postgres=*# create function longname(fullname) returns text language sql
> as $$select $1.first || ' ' || $1.last$$;
> CREATE FUNCTION
> postgres=*# PREPARE transaction 'mytran';
> PREPARE TRANSACTION
>
> Now, this time we succeed and not getting the above error (”ERROR:  cannot PREPARE a transaction that has operated on
temporaryobjects), like the way we were getting with BLOCK-1 
>
> This is happening because we set MyXactFlags in relation_open function call, and here relation_open is getting called
fromload_typcache_tupdesc, but in the second run of “create function…” in the above #2 block will not call
load_typcache_tupdescbecause of the below condition(typentry->tupDesc == NULL) in  lookup_type_cache(). 
>
>         /*
>          * If it's a composite type (row type), get tupdesc if requested
>          */
>         if ((flags & TYPECACHE_TUPDESC) &&
>                 typentry->tupDesc == NULL &&
>                 typentry->typtype == TYPTYPE_COMPOSITE)
>         {
>                 load_typcache_tupdesc(typentry);
>         }
>
> We set typentry->tupDesc to non NULL(and populates it with proper tuple descriptor in the cache) value during our
firstcall to “create function…” in BLOCK-1. 
> We have logic in file xact.c::PrepareTransaction() to simply error out if we have accessed any temporary object in
thecurrent transaction but because of the above-described issue of not setting XACT_FLAGS_ACCESSEDTEMPNAMESPACE in
MyXactFlagssecond run of “create function..” Works and PREPARE TRANSACTION succeeds(but it should fail). 
>
> Please find attached the proposed patch to FIX this issue.

I was able to reproduce the issue with your patch and your patch fixes
the issue.

Few comments:
1) We can drop the table after this test.
+CREATE TEMP TABLE temp_tbl (first TEXT, last TEXT);
+BEGIN;
+CREATE FUNCTION longname(temp_tbl) RETURNS TEXT LANGUAGE SQL
+AS $$SELECT $1.first || ' ' || $1.last$$;
+PREPARE TRANSACTION 'temp_tbl_access';
+
+BEGIN;
+CREATE FUNCTION longname(temp_tbl) RETURNS TEXT LANGUAGE SQL
+AS $$SELECT $1.first || ' ' || $1.last$$;
+PREPARE TRANSACTION 'temp_tbl_access';

2) +-- Test for accessing Temporary table
+-- in prepare transaction.
can be changed to
-- Test for accessing cached temporary table in a prepared transaction.

3) +-- These cases must fail and generate errors about Temporary objects.
can be changed to
-- These cases should fail with cannot access temporary object error.

Regards,
Vignesh

On Wed, Apr 7, 2021 at 12:19 PM Himanshu Upadhyaya
<upadhyaya.himanshu@gmail.com> wrote:
> Please find attached the V2 patch.

Hi,

This patch is essentially taking the position that calling
load_typcache_tupdesc before using that tupdesc for anything is
required for correctness. I'm not sure whether that's a good
architectural decision: to me, it looks like whoever wrote this code
originally - I think it was Tom - had the idea that it would be OK to
skip calling that function whenever we already have the value.
Changing that has some small performance cost, and it also just looks
kind of weird, because you don't expect a function called
load_typcache_tupdesc() to have the side effect of preventing some
kind of bad thing from happening. You just expect it to be loading
stuff. The comments in this code are not exactly stellar as things
stand, but the patch also doesn't update them in a meaningful way.
Sure, it corrects a few comments that would be flat-out wrong
otherwise, but it doesn't add any kind of explanation that would help
the next person who looks at this code understand why they shouldn't
just put back the exact same performance optimization you're proposing
to rip out.

An alternative design would be to find some new place to set
XACT_FLAGS_ACCESSEDTEMPNAMESPACE. For example, we could set a flag in
the TypeCacheEntry indicating whether this flag ought to be set when
somebody looks up the entry.

But, before we get too deeply into what the design should be, I think
we need to be clear about what problem we're trying to fix. I agree
that the behavior you demonstrate in your example looks inconsistent,
but that doesn't necessarily mean that the code is wrong. What exactly
is the code trying to prohibit here, and does this test case really
show that principle being violated? The comments in
PrepareTransaction() justify this prohibition by saying that "Having
the prepared xact hold locks on another backend's temp table seems a
bad idea --- for instance it would prevent the backend from exiting.
There are other problems too, such as how to clean up the source
backend's local buffers and ON COMMIT state if the prepared xact
includes a DROP of a temp table." But, in this case, none of that
stuff actually happens. If I run your test case without the patch, the
backend has no problem exiting, and the prepared xact holds no lock on
the temp table, and the prepared xact does not include a DROP of a
temp table. That's not to say that everything is great, because after
starting a new session and committing mytran, this happens:

rhaas=# \df longname
ERROR:  cache lookup failed for type 16432

But the patch doesn't actually prevent that from happening, because
even with the patch applied I can still recreate the same failure
using a different sequence of steps:

[ session 1 ]
rhaas=# create temp table fullname (first text, last text);
CREATE TABLE

[ session 2 ]
rhaas=# select oid::regclass from pg_class where relname = 'fullname';
        oid
--------------------
 pg_temp_3.fullname
(1 row)

rhaas=# begin;
BEGIN
rhaas=*# create function longname(pg_temp_3.fullname) returns text
language sql as $$select $1.first || ' ' || $1.last$$;
CREATE FUNCTION

[ session 1 ]
rhaas=# \q

[ session 2 ]
rhaas=*# commit;
COMMIT
rhaas=# \df longname
ERROR:  cache lookup failed for type 16448

To really fix this, you'd need CREATE FUNCTION to take a lock on the
containing namespace, whether permanent or temporary. You'd also need
every other CREATE statement that creates a schema-qualified object to
do the same thing. Maybe that's a good idea, but we've been reluctant
to go that far in the past due to performance consequences, and it's
not clear whether any of those problems are related to the issue that
prompted you to submit the patch. So, I'm kind of left wondering what
exactly you're trying to solve here. Can you clarify?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Apr 14, 2021 at 6:45 AM Himanshu Upadhyaya
<upadhyaya.himanshu@gmail.com> wrote:
> The purpose of this FIX is to mainly focus on getting consistent behavior
> with PREPARE TRANSACTION. With the case that I had mentioned
> previously, my expectation was either both PREPARE TRANSACTION should fail
> or both should succeed but here second same "PREPARE TRANSACTION" was
> successful however first one was failing with an error, which is kind of weird to me.

I agree that it's weird, but that doesn't mean that your patch is an
improvement, and I don't think it is. If we could make this thing more
consistent without incurring any negatives, I'd be in favor of that.
But the patch does have some negatives, which in my opinion are more
substantial than the problem you're trying to fix. Namely, possible
performance consequences, and undocumented and fragile assumptions
that, as it seems to me, may easily get broken in the future. I see
that you've repeatedly capitalized the word FIX in your reply, but
it's just not that simple. If this had really bad consequences like
corrupting data or crashing the server then it would be essential to
do something about it, but so far the worst consequence you've
indicated is that an obscure sequence of SQL commands that no real
user is likely to issue produces a slightly surprising result. That's
not great, but neither is it an emergency.

> I have also tried to reproduce the behavior.

Your test case isn't ideal for reproducing the problem that the
comment is worrying about. Normally, when we take a lock on a table,
we hold it until commit. But, that only applies when we run a query
that mentions the table, like a SELECT or an UPDATE. In your case, we
only end up opening the table to build a relcache entry for it, so
that we can look at the metadata. And, catalog scans used to build
syscache and relcache entries release locks immediately, rather than
waiting until the end of the transaction. So it might be that if we
failed to ever set XACT_FLAGS_ACCESSEDTEMPNAMESPACE in your test case,
everything would be fine.

That doesn't seem to be true in general though. I tried changing
"cannot PREPARE a transaction that has operated on temporary objects"
from an ERROR to a NOTICE and then ran 'make check'. It hung. I think
this test case is the same problem as the regression tests hit; in any
case, it also hangs:

rhaas=# begin;
BEGIN
rhaas=*# create temp table first ();
CREATE TABLE
rhaas=*# prepare transaction 'whatever';
NOTICE:  cannot PREPARE a transaction that has operated on temporary objects
PREPARE TRANSACTION
rhaas=# create temp table second ();
[ hangs ]

I haven't checked, but I think the problem here is that the first
transaction had to create this backend's pg_temp schema and the second
one can't see the results of the first one doing it so it wants to do
the same thing and that results in waiting for a lock the prepared
transaction already holds. I made a quick attempt to reproduce a hang
at backend exit time, but couldn't find a case where that happened.
That doesn't mean there isn't one, though. There's a very good chance
that the person who wrote that comment knew that a real problem
existed, and just didn't describe it well enough for you or I to
immediately know what it is. It is also possible that they were
completely wrong, or that things have changed since the comment was
written, but we can't assume that without considerably more research
and analysis than either of us has done so far.

I think one point to take away here is that question of whether a
temporary relation has been "accessed" is not all black and white. If
I ran a SELECT statement against a relation, I think it's clear that
I've accessed it. But, if I just used the name of that relation as a
type name in some other SQL command, did I really access it? The
current code's answer to that is that if we had to open and lock the
relation to get its metadata, then we accessed it, and if we already
had the details that we needed in cache, then we did not access it.
Now, again, I agree that looks a little weird from a user point of
view, but looking at the implementation, you can kind of see why it
ends up like that. From a certain point of view, it would be more
surprising if we never opened or locked the relation and yet ended up
deciding that we'd accessed it. Now maybe we should further explore
going the other direction and avoiding setting the flag at all in this
case, since I think we're neither retaining a lock nor touching any
relation buffers, but I think that needs more analysis. Even if we
decide that's safe, there's still the problem of finding a better
implementation that's not overly complex for what really feels like a
very minor issue.

--
Robert Haas
EDB: http://www.enterprisedb.com