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

On Wed, Apr 7, 2021 at 12:19 PM Himanshu Upadhyaya
<upadhyaya(dot)himanshu(at)gmail(dot)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