Hi Vignesh,
Thanks for sharing the review comments. Please find my response below.
> 1) We can drop the table after this test.
>
Done.
> 2) +-- Test for accessing Temporary table
> +-- in prepare transaction.
> can be changed to
> -- Test for accessing cached temporary table in a prepared transaction.
>
Comment is now modified as above.
> 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.
>
The error is not about accessing the temporary object rather it's about
disallowing create transaction as it is referring to the temporary objects.
I have changed it with the exact error we get in those cases.
Please find attached the V2 patch.
Thanks,
Himanshu
On Wed, Apr 7, 2021 at 4:11 PM vignesh C <vignes...@gmail.com> wrote:
> On Tue, Apr 6, 2021 at 8:18 PM Himanshu Upadhyaya
> <upadhyaya.himan...@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 consider the 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 temporary objects), 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 from load_typcache_tupdesc,
> but in the second run of “create function…” in the above #2 block will not
> call load_typcache_tupdesc because 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 first call 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 the current transaction but
> because of the above-described issue of not setting
> XACT_FLAGS_ACCESSEDTEMPNAMESPACE in MyXactFlags second 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
>
From 28252200d3e49af785e154dd99e01578beaaa3a2 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya <himanshu.upadhy...@enterprisedb.com>
Date: Wed, 7 Apr 2021 21:42:37 +0530
Subject: [PATCH v2] PREPARE TRANSACTION must fail when mixed with temporary
object.
---
src/backend/utils/cache/typcache.c | 30 ++++++++++++++++--------------
src/test/regress/expected/temp.out | 15 +++++++++++++++
src/test/regress/sql/temp.sql | 15 +++++++++++++++
3 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 4915ef5934..36fac6150d 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -813,7 +813,6 @@ lookup_type_cache(Oid type_id, int flags)
* 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);
@@ -881,21 +880,25 @@ load_typcache_tupdesc(TypeCacheEntry *typentry)
/*
* Link to the tupdesc and increment its refcount (we assert it's a
- * refcounted descriptor). We don't use IncrTupleDescRefCount() for this,
- * because the reference mustn't be entered in the current resource owner;
+ * refcounted descriptor), do this only if it was not populated previously.
+ * We don't use IncrTupleDescRefCount() for this, because the reference
+ * mustn't be entered in the current resource owner;
* it can outlive the current query.
*/
- typentry->tupDesc = RelationGetDescr(rel);
+ if (typentry->tupDesc == NULL)
+ {
+ typentry->tupDesc = RelationGetDescr(rel);
- Assert(typentry->tupDesc->tdrefcount > 0);
- typentry->tupDesc->tdrefcount++;
+ Assert(typentry->tupDesc->tdrefcount > 0);
+ typentry->tupDesc->tdrefcount++;
- /*
- * In future, we could take some pains to not change tupDesc_identifier if
- * the tupdesc didn't really change; but for now it's not worth it.
- */
- typentry->tupDesc_identifier = ++tupledesc_id_counter;
+ /*
+ * In future, we could take some pains to not change tupDesc_identifier if
+ * the tupdesc didn't really change; but for now it's not worth it.
+ */
+ typentry->tupDesc_identifier = ++tupledesc_id_counter;
+ }
relation_close(rel, AccessShareLock);
}
@@ -1529,9 +1532,8 @@ cache_record_field_properties(TypeCacheEntry *typentry)
int newflags;
int i;
- /* Fetch composite type's tupdesc if we don't have it already */
- if (typentry->tupDesc == NULL)
- load_typcache_tupdesc(typentry);
+ /* Fetch composite type's tupdesc */
+ load_typcache_tupdesc(typentry);
tupdesc = typentry->tupDesc;
/* Must bump the refcount while we do additional catalog lookups */
diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out
index a5b3ed34a3..aaff917df0 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -319,6 +319,21 @@ select relname from pg_class where relname ~ '^temp_inh_oncommit_test';
(1 row)
drop table temp_inh_oncommit_test;
+-- Test for accessing cached temporary table in a prepared transaction.
+-- These cases should fail with "cannot PREPARE a transaction that has
+-- operated on temporary objects" ERROR.
+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';
+ERROR: cannot PREPARE a transaction that has operated on temporary objects
+BEGIN;
+CREATE FUNCTION longname(temp_tbl) RETURNS TEXT LANGUAGE SQL
+AS $$SELECT $1.first || ' ' || $1.last$$;
+PREPARE TRANSACTION 'temp_tbl_access';
+ERROR: cannot PREPARE a transaction that has operated on temporary objects
+DROP TABLE temp_tbl;
-- Tests with two-phase commit
-- Transactions creating objects in a temporary namespace cannot be used
-- with two-phase commit.
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index 424d12b283..4c4dc6344a 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -240,6 +240,21 @@ select * from temp_inh_oncommit_test;
select relname from pg_class where relname ~ '^temp_inh_oncommit_test';
drop table temp_inh_oncommit_test;
+-- Test for accessing cached temporary table in a prepared transaction.
+-- These cases should fail with "cannot PREPARE a transaction that has
+-- operated on temporary objects" ERROR.
+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';
+
+DROP TABLE temp_tbl;
-- Tests with two-phase commit
-- Transactions creating objects in a temporary namespace cannot be used
-- with two-phase commit.
--
2.25.1
