Thread: Calling PLpgSQL function with composite type fails with an error:"ERROR: could not open relation with OID ..."

Hi All,

When the following test-case is executed on master, it fails with an
error: "ERROR:  could not open relation with OID ..."

-- create a test table:
create table tab1(a int, b text);

-- create a test function:
create or replace function f1() returns void as
$$
declare
   var1 tab1;
begin
   select * into var1 from tab1;
end
$$ language plpgsql;

-- call the test function:
select f1();

-- drop the test table and re-create it:
drop table tab1;
create table tab1(a int, b text);

-- call the test function:
select f1();

-- call the test function once again:
select f1(); -- this fails with an error "ERROR:  could not open
relation with OID .."

I'm trying to investigate this issue and will try to share my findings soon...

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

The issue here is that PLpgSQL_rec structure being updated by
revalidate_rectypeid() is actually a local/duplicate copy of the
PLpgSQL_rec structure available in plpgsql_HashTable (refer to
copy_plpgsql_datums() where you would notice that if datum type is
PLPGSQL_DTYPE_REC we actually mempcy() the PLpgSQL_rec structure
available in func >datums[] array). This basically means that the
rectypeid field updated post typcache entry validation in
revalidation_rectypeid() is actually a field in duplicate copy of
PLpgSQL_rec structure, not the original copy of it available in
func->datums[]. Hence, when the same function is executed for the
second time, the rectypeid field of PLpgSQL_rec structure being
reloaded from the func->datums[] actually contains the stale value
however the typcache entry in it is up-to-date which means
revalidation_rectypeid() returns immediately leaving a stale value in
rectypeid. This causes the function make_expanded_record_from_typeid()
to use the outdated value in rec->rectypeid resulting into the given
error.

To fix this, I think instead of using rec->rectypeid field we should
try using rec->datatype->typoid when calling
make_expanded_record_from_typeid(). Here is the change that I'm
suggesting:

--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -6942,7 +6942,7 @@ make_expanded_record_for_rec(PLpgSQL_execstate *estate,
            newerh = make_expanded_record_from_exprecord(srcerh,
                                                         mcontext);
        else
-           newerh = make_expanded_record_from_typeid(rec->rectypeid, -1,
+          newerh = make_expanded_record_from_typeid(rec->datatype->typoid, -1,

Thoughts ?

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Thu, Dec 26, 2019 at 3:57 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi All,
>
> When the following test-case is executed on master, it fails with an
> error: "ERROR:  could not open relation with OID ..."
>
> -- create a test table:
> create table tab1(a int, b text);
>
> -- create a test function:
> create or replace function f1() returns void as
> $$
> declare
>    var1 tab1;
> begin
>    select * into var1 from tab1;
> end
> $$ language plpgsql;
>
> -- call the test function:
> select f1();
>
> -- drop the test table and re-create it:
> drop table tab1;
> create table tab1(a int, b text);
>
> -- call the test function:
> select f1();
>
> -- call the test function once again:
> select f1(); -- this fails with an error "ERROR:  could not open
> relation with OID .."
>
> I'm trying to investigate this issue and will try to share my findings soon...
>
> --
> With Regards,
> Ashutosh Sharma
> EnterpriseDB:http://www.enterprisedb.com

Ashutosh Sharma <ashu.coek88@gmail.com> writes:
> The issue here is that PLpgSQL_rec structure being updated by
> revalidate_rectypeid() is actually a local/duplicate copy of the
> PLpgSQL_rec structure available in plpgsql_HashTable (refer to
> copy_plpgsql_datums() where you would notice that if datum type is
> PLPGSQL_DTYPE_REC we actually mempcy() the PLpgSQL_rec structure
> available in func >datums[] array). This basically means that the
> rectypeid field updated post typcache entry validation in
> revalidation_rectypeid() is actually a field in duplicate copy of
> PLpgSQL_rec structure, not the original copy of it available in
> func->datums[]. Hence, when the same function is executed for the
> second time, the rectypeid field of PLpgSQL_rec structure being
> reloaded from the func->datums[] actually contains the stale value
> however the typcache entry in it is up-to-date which means
> revalidation_rectypeid() returns immediately leaving a stale value in
> rectypeid. This causes the function make_expanded_record_from_typeid()
> to use the outdated value in rec->rectypeid resulting into the given
> error.

Good catch!

> To fix this, I think instead of using rec->rectypeid field we should
> try using rec->datatype->typoid when calling
> make_expanded_record_from_typeid().

This is a crummy fix, though.  In the first place, if we did it like this
we'd have to fix every other caller of revalidate_rectypeid() likewise.
Basically the issue here is that revalidate_rectypeid() is failing to do
what it says on the tin, and you're proposing to make the callers work
around that instead of fixing revalidate_rectypeid().  That seems like
an odd choice from here.

More generally, the reason for the separation between PLpgSQL_rec and
PLpgSQL_type in this part of the code is that PLpgSQL_rec.rectypeid is
supposed to record the actual type ID currently instantiated in that
variable (in the current function execution), whereas PLpgSQL_type is a
cache for the last type lookup we did; that's why it's okay to share the
latter but not the former across function executions.  So failing to
update rec->rectypeid is almost certainly going to lead to problems
later on.

I pushed a fix that makes revalidate_rectypeid() deal with this case.
Thanks for the report and debugging!

            regards, tom lane

On Fri, Dec 27, 2019 at 1:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Ashutosh Sharma <ashu.coek88@gmail.com> writes:
> > The issue here is that PLpgSQL_rec structure being updated by
> > revalidate_rectypeid() is actually a local/duplicate copy of the
> > PLpgSQL_rec structure available in plpgsql_HashTable (refer to
> > copy_plpgsql_datums() where you would notice that if datum type is
> > PLPGSQL_DTYPE_REC we actually mempcy() the PLpgSQL_rec structure
> > available in func >datums[] array). This basically means that the
> > rectypeid field updated post typcache entry validation in
> > revalidation_rectypeid() is actually a field in duplicate copy of
> > PLpgSQL_rec structure, not the original copy of it available in
> > func->datums[]. Hence, when the same function is executed for the
> > second time, the rectypeid field of PLpgSQL_rec structure being
> > reloaded from the func->datums[] actually contains the stale value
> > however the typcache entry in it is up-to-date which means
> > revalidation_rectypeid() returns immediately leaving a stale value in
> > rectypeid. This causes the function make_expanded_record_from_typeid()
> > to use the outdated value in rec->rectypeid resulting into the given
> > error.
>
> Good catch!
>
> > To fix this, I think instead of using rec->rectypeid field we should
> > try using rec->datatype->typoid when calling
> > make_expanded_record_from_typeid().
>
> This is a crummy fix, though.  In the first place, if we did it like this
> we'd have to fix every other caller of revalidate_rectypeid() likewise.
> Basically the issue here is that revalidate_rectypeid() is failing to do
> what it says on the tin, and you're proposing to make the callers work
> around that instead of fixing revalidate_rectypeid().  That seems like
> an odd choice from here.
>
> More generally, the reason for the separation between PLpgSQL_rec and
> PLpgSQL_type in this part of the code is that PLpgSQL_rec.rectypeid is
> supposed to record the actual type ID currently instantiated in that
> variable (in the current function execution), whereas PLpgSQL_type is a
> cache for the last type lookup we did; that's why it's okay to share the
> latter but not the former across function executions.  So failing to
> update rec->rectypeid is almost certainly going to lead to problems
> later on.
>
> I pushed a fix that makes revalidate_rectypeid() deal with this case.
> Thanks for the report and debugging!
>

Okay. Thanks for that fix. You've basically forced
revalidate_rectypeid() to update the PLpgSQL_rec's rectypeid
irrespective of typcache entry requires re-validation or not.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Ashutosh Sharma <ashu.coek88@gmail.com> writes:
> Okay. Thanks for that fix. You've basically forced
> revalidate_rectypeid() to update the PLpgSQL_rec's rectypeid
> irrespective of typcache entry requires re-validation or not.

Right.  The assignment is cheap enough that it hardly seems
worth avoiding.

            regards, tom lane

On Fri, Dec 27, 2019 at 9:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Ashutosh Sharma <ashu.coek88@gmail.com> writes:
> > Okay. Thanks for that fix. You've basically forced
> > revalidate_rectypeid() to update the PLpgSQL_rec's rectypeid
> > irrespective of typcache entry requires re-validation or not.
>
> Right.  The assignment is cheap enough that it hardly seems
> worth avoiding.
>

Agreed.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com