Horton

Thread

Commits

  1. Ensure that PLPGSQL_DTYPE_ROW variables have valid refname fields.

  1. snprintf assert is broken by plpgsql #option dump

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-10-04T20:50:23Z

    Hi

Today I had broken plpgsql_check tests aganst PostgreSQL 12. After command

create or replace function ml_trg()
returns trigger as $$
#option dump
declare
begin
  if TG_OP = 'INSERT' then
    if NEW.status_from IS NULL then
      begin
        -- performance issue only
        select status into NEW.status_from
           from pa
          where pa_id = NEW.pa_id;
        -- nonexist target value
        select status into NEW.status_from_xxx
           from pa
          where pa_id = NEW.pa_id;
      exception
        when DATA_EXCEPTION then
          new.status_from := 'DE';
      end;
    end if;
  end if;
  if TG_OP = 'DELETE' then return OLD; else return NEW; end if;
exception
  when OTHERS then
    NULL;
    if TG_OP = 'DELETE' then return OLD; else return NEW; end if;
end;
$$ language plpgsql;

I got backtrace:
Program received signal SIGABRT, Aborted.
0x00007f2c140e653f in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x00007f2c140e653f in raise () from /lib64/libc.so.6
#1  0x00007f2c140d0895 in abort () from /lib64/libc.so.6
#2  0x00000000008b7903 in ExceptionalCondition
(conditionName=conditionName@entry=0xb00d3b "!(strvalue != ((void *)0))",
    errorType=errorType@entry=0x906034 "FailedAssertion",
fileName=fileName@entry=0xb00d30 "snprintf.c",
    lineNumber=lineNumber@entry=674) at assert.c:54
#3  0x00000000008ff368 in dopr (target=target@entry=0x7fff4ff6aad0,
format=0x7f2bfe4b0de3 " fields",
    format@entry=0x7f2bfe4b0dda "ROW %-16s fields",
args=args@entry=0x7fff4ff6ab18)
at snprintf.c:674
#4  0x00000000008ff6b7 in pg_vfprintf (stream=<optimized out>,
fmt=fmt@entry=0x7f2bfe4b0dda
"ROW %-16s fields",
    args=args@entry=0x7fff4ff6ab18) at snprintf.c:261
#5  0x00000000008ff7ff in pg_printf (fmt=fmt@entry=0x7f2bfe4b0dda "ROW
%-16s fields") at snprintf.c:286
#6  0x00007f2bfe4a7c67 in plpgsql_dumptree (func=func@entry=0x26811e8) at
pl_funcs.c:1676
#7  0x00007f2bfe49a136 in do_compile (fcinfo=fcinfo@entry=0x7fff4ff6afa0,
procTup=procTup@entry=0x7f2bfe4ba100, function=0x26811e8,
    function@entry=0x0, hashkey=hashkey@entry=0x7fff4ff6ad20,
forValidator=forValidator@entry=true) at pl_comp.c:794
#8  0x00007f2bfe49a27a in plpgsql_compile (fcinfo=fcinfo@entry=0x7fff4ff6afa0,
forValidator=forValidator@entry=true) at pl_comp.c:224
#9  0x00007f2bfe497126 in plpgsql_validator (fcinfo=<optimized out>) at
pl_handler.c:504
#10 0x00000000008c03df in OidFunctionCall1Coll
(functionId=functionId@entry=13392,
collation=collation@entry=0, arg1=arg1@entry=40960)
    at fmgr.c:1418
#11 0x0000000000563444 in ProcedureCreate (procedureName=<optimized out>,
procNamespace=procNamespace@entry=2200,
    replace=<optimized out>, returnsSet=<optimized out>,
returnType=<optimized out>, proowner=16384, languageObjectId=13393,
    languageValidator=13392,
    prosrc=0x264feb8 "\n#option

There are new assert

Assert(strvalue != NULL);

probably all refname usage inside plpgsql dump functions has problem with
it.

I found two parts

diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index b93f866223..d97997c001 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -1519,7 +1519,7 @@ dump_execsql(PLpgSQL_stmt_execsql *stmt)
        dump_ind();
        printf("    INTO%s target = %d %s\n",
               stmt->strict ? " STRICT" : "",
-              stmt->target->dno, stmt->target->refname);
+              stmt->target->dno, stmt->target->refname ?
stmt->target->refname : "null");
    }
    dump_indent -= 2;
 }
@@ -1673,7 +1673,7 @@ plpgsql_dumptree(PLpgSQL_function *func)
                    PLpgSQL_row *row = (PLpgSQL_row *) d;
                    int         i;

-                   printf("ROW %-16s fields", row->refname);
+                   printf("ROW %-16s fields", row->refname ? row->refname
: "null");
                    for (i = 0; i < row->nfields; i++)
                    {
                        printf(" %s=var %d", row->fieldnames[i],

Regards

Pavel

  2. Re: snprintf assert is broken by plpgsql #option dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-04T21:17:14Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> There are new assert
> Assert(strvalue != NULL);
> probably all refname usage inside plpgsql dump functions has problem with
> it.

This isn't so much a "new assert" as a modeling of the fact that some
printf implementations dump core on a null string pointer, and have done
so since the dawn of time.

Now that we only use snprintf.c in HEAD, it'd be possible to consider
modeling glibc's behavior instead, ie instead of the Assert do

	if (strvalue == NULL)
	    strvalue = "(null)";

I do not think this would be a good idea though, at least not till all
release branches that *don't* always use snprintf.c are out of support.
Every assert failure that we find here is a live bug in the back
branches, even if it happens not to occur on $your-favorite-platform.

Even once that window elapses, I'd not be especially on board with
snprintf.c papering over such cases.  They're bugs really.

> I found two parts

Thanks for the report, will push something.

			regards, tom lane

  3. Re: snprintf assert is broken by plpgsql #option dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-04T21:57:53Z

    I wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> I found two parts

> Thanks for the report, will push something.

On closer look, I'm not sure that these are the only places that are
assuming that any PLpgSQL_variable struct has a refname.  What seems
like a safer answer is to make sure they all do, more or less as
attached.

			regards, tom lane

  4. Re: snprintf assert is broken by plpgsql #option dump

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-10-05T00:54:58Z

    čt 4. 10. 2018 v 23:57 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> I wrote:
> > Pavel Stehule <pavel.stehule@gmail.com> writes:
> >> I found two parts
>
> > Thanks for the report, will push something.
>
> On closer look, I'm not sure that these are the only places that are
> assuming that any PLpgSQL_variable struct has a refname.  What seems
> like a safer answer is to make sure they all do, more or less as
> attached.
>


+1

Pavel


>                         regards, tom lane
>
>