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

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 <vignesh21(at)gmail(dot)com> wrote:

> On Tue, Apr 6, 2021 at 8:18 PM Himanshu Upadhyaya
> <upadhyaya(dot)himanshu(at)gmail(dot)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
>