Re: snprintf assert is broken by plpgsql #option dump
čt 4. 10. 2018 v 23:57 odesílatel Tom Lane napsal: > I wrote: > > Pavel Stehule 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 > >
Re: snprintf assert is broken by plpgsql #option dump
I wrote: > Pavel Stehule 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 diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index 721234d..59460d2 100644 *** a/src/pl/plpgsql/src/pl_comp.c --- b/src/pl/plpgsql/src/pl_comp.c *** build_row_from_vars(PLpgSQL_variable **v *** 1896,1901 --- 1896,1903 row = palloc0(sizeof(PLpgSQL_row)); row->dtype = PLPGSQL_DTYPE_ROW; + row->refname = "(unnamed row)"; + row->lineno = -1; row->rowtupdesc = CreateTemplateTupleDesc(numvars, false); row->nfields = numvars; row->fieldnames = palloc(numvars * sizeof(char *)); diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 574234d..4552638 100644 *** a/src/pl/plpgsql/src/pl_exec.c --- b/src/pl/plpgsql/src/pl_exec.c *** exec_stmt_call(PLpgSQL_execstate *estate *** 2205,2210 --- 2205,2211 row = palloc0(sizeof(*row)); row->dtype = PLPGSQL_DTYPE_ROW; + row->refname = "(unnamed row)"; row->lineno = -1; row->varnos = palloc(sizeof(int) * FUNC_MAX_ARGS); diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index b59869a..68e399f 100644 *** a/src/pl/plpgsql/src/pl_gram.y --- b/src/pl/plpgsql/src/pl_gram.y *** decl_cursor_args : *** 613,618 --- 613,619 new = palloc0(sizeof(PLpgSQL_row)); new->dtype = PLPGSQL_DTYPE_ROW; + new->refname = "(unnamed row)"; new->lineno = plpgsql_location_to_lineno(@1); new->rowtupdesc = NULL; new->nfields = list_length($2); *** read_into_scalar_list(char *initial_name *** 3526,3531 --- 3527,3533 row = palloc0(sizeof(PLpgSQL_row)); row->dtype = PLPGSQL_DTYPE_ROW; + row->refname = "(unnamed row)"; row->lineno = plpgsql_location_to_lineno(initial_location); row->rowtupdesc = NULL; row->nfields = nfields; *** make_scalar_list1(char *initial_name, *** 3560,3565 --- 3562,3568 row = palloc0(sizeof(PLpgSQL_row)); row->dtype = PLPGSQL_DTYPE_ROW; + row->refname = "(unnamed row)"; row->lineno = lineno; row->rowtupdesc = NULL; row->nfields = 1; diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 4a4c7cb..f6c35a5 100644 *** a/src/pl/plpgsql/src/plpgsql.h --- b/src/pl/plpgsql/src/plpgsql.h *** typedef struct PLpgSQL_var *** 326,332 * Note that there's no way to name the row as such from PL/pgSQL code, * so many functions don't need to support these. * ! * refname, isconst, notnull, and default_val are unsupported (and hence * always zero/null) for a row. The member variables of a row should have * been checked to be writable at compile time, so isconst is correctly set * to false. notnull and default_val aren't applicable. --- 326,337 * Note that there's no way to name the row as such from PL/pgSQL code, * so many functions don't need to support these. * ! * That also means that there's no real name for the row variable, so we ! * conventionally set refname to "(unnamed row)". We could leave it NULL, ! * but it's too convenient to be able to assume that refname is valid in ! * all variants of PLpgSQL_variable. ! * ! * isconst, notnull, and default_val are unsupported (and hence * always zero/null) for a row. The member variables of a row should have * been checked to be writable at compile time, so isconst is correctly set * to false. notnull and default_val aren't applicable.
Re: snprintf assert is broken by plpgsql #option dump
Pavel Stehule 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
snprintf assert is broken by plpgsql #option dump
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.
0x7f2c140e653f in raise () from /lib64/libc.so.6
(gdb) bt
#0 0x7f2c140e653f in raise () from /lib64/libc.so.6
#1 0x7f2c140d0895 in abort () from /lib64/libc.so.6
#2 0x008b7903 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 0x008ff368 in dopr (target=target@entry=0x7fff4ff6aad0,
format=0x7f2bfe4b0de3 " fields",
format@entry=0x7f2bfe4b0dda "ROW %-16s fields",
args=args@entry=0x7fff4ff6ab18)
at snprintf.c:674
#4 0x008ff6b7 in pg_vfprintf (stream=,
fmt=fmt@entry=0x7f2bfe4b0dda
"ROW %-16s fields",
args=args@entry=0x7fff4ff6ab18) at snprintf.c:261
#5 0x008ff7ff in pg_printf (fmt=fmt@entry=0x7f2bfe4b0dda "ROW
%-16s fields") at snprintf.c:286
#6 0x7f2bfe4a7c67 in plpgsql_dumptree (func=func@entry=0x26811e8) at
pl_funcs.c:1676
#7 0x7f2bfe49a136 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 0x7f2bfe49a27a in plpgsql_compile (fcinfo=fcinfo@entry=0x7fff4ff6afa0,
forValidator=forValidator@entry=true) at pl_comp.c:224
#9 0x7f2bfe497126 in plpgsql_validator (fcinfo=) at
pl_handler.c:504
#10 0x008c03df in OidFunctionCall1Coll
(functionId=functionId@entry=13392,
collation=collation@entry=0, arg1=arg1@entry=40960)
at fmgr.c:1418
#11 0x00563444 in ProcedureCreate (procedureName=,
procNamespace=procNamespace@entry=2200,
replace=, returnsSet=,
returnType=, 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
