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

Hi Robert,

Thanks for sharing your thoughts.
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 have also tried to reproduce the behavior.

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

[session:2]
postgres=# select oid::regclass from pg_class where relname = 'fullname';
oid
--------------------
pg_temp_3.fullname

postgres=# BEGIN;
create function longname1(pg_temp_3.fullname) returns text language sql
as $$select $1.first || ' ' || $1.last$$;
BEGIN
CREATE FUNCTION
postgres=*# prepare transaction 'mytran2';
ERROR: cannot PREPARE a transaction that has operated on temporary objects
postgres=# BEGIN;
create function longname1(pg_temp_3.fullname) returns text language sql
as $$select $1.first || ' ' || $1.last$$;
BEGIN
CREATE FUNCTION

[session:1]
postgres=# \q // no problem in exiting

[session:2]

postgres=*# prepare transaction 'mytran2';
PREPARE TRANSACTION
postgres=# \df
ERROR: cache lookup failed for type 16429

looking at the comment in the code [session:1] should hang while exiting
but
I don't see a problem here, you have already explained that in your reply.
Even then I feel that behavior should be consistent when we mix temporary
objects in PREPARE TRANSACTION.

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."
>
I can see from the above experiment that there is no problem with the
lock in the above case but not sure if there is any issue with "clean up
the source backend's local buffers", if not then we don't even need this
ERROR (ERROR: cannot PREPARE a transaction that has operated
on temporary objects) in PREPARE TRANSACTION?

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.

Yes, the purpose of this patch is to actually have a valid value in
XACT_FLAGS_ACCESSEDTEMPNAMESPACE, having said that it should
always be true if we access temporary object else false.
Even if we do changes to have lock in case of "CREATE FUNCTION", we
also need to have this FIX in place so that "PREPARE TRANSACTION"
mixed with TEMPORARY OBJECT will always be restricted and will not
cause any hang issue(which we will start observing once we implement
these "CREATE STATEMENT" changes) as mentioned in the comment
in the PrepareTransaction().

Just thinking if it's acceptable to FIX this and make it consistent by
properly
setting XACT_FLAGS_ACCESSEDTEMPNAMESPACE, so that it should always
fail if we access the temporary object, I also agree here that it will not
actually cause
any issue because of xact lock but then from user perspective it seems
weird
when the same PREPARE TRANSACTION is working second time onwards, thoughts?

Thanks,
Himanshu

On Thu, Apr 8, 2021 at 7:43 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> 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
>