Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

Jim Nasby  writes:
> On 1/9/17 5:38 PM, Tom Lane wrote:
>> Yeah.  I looked at that but couldn't get terribly excited about it,
>> because AFAICS, Tcl in general is apt to fall over under sufficient
>> memory pressure.

> Though, since a memory error could just as likely come out of tcl, which 
> is going to panic us anyway, I guess it doesn't matter.

Exactly.  I can't get excited about making our code slower and less
readable if there's only a fifty-fifty chance that doing so avoids a
crash.  Tcl users just need to stay far away from OOM conditions.

(If it were a more popular language, maybe there would be reason to
try to push to improve this, but ...)

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 1/9/17 5:38 PM, Tom Lane wrote:

Jim Nasby  writes:

Hmm... I suspect there's more places where this could be a problem. For
example, pltcl_quote calls palloc, which could ereport as well.


Yeah.  I looked at that but couldn't get terribly excited about it,
because AFAICS, Tcl in general is apt to fall over under sufficient
memory pressure.  There are too many API functions with no failure
return provision, even though they clearly must do memory allocation
under the hood.  (The fact that they've even got ckalloc() tells you
what their philosophy is here: they're content to PANIC on OOM.)


Uhm... wow. According to [1] that's going to result in TCL calling 
abort(). I'm guessing there's no reasonable way for us to recognize a 
TCL abort as something that we didn't need to panic on...


In any case, AFAICT there's still a bug here: if PG hits a memory error, 
we'll ERROR, which is going to leave the interpreter right back in a bad 
state for the rest of that session. That doesn't seem so good. It also 
means a pltcl proc wouldn't get the chance to trap the error.


Though, since a memory error could just as likely come out of tcl, which 
is going to panic us anyway, I guess it doesn't matter.



I think pltcl should attempt to cover any error conditions that aren't
purely out-of-memory ones, but in a quick scan I didn't see any other
hazards.


Yeah, I think we're OK on that front.

I was hoping to establish a precedent for all the new TCL functions that 
pltcl provides so it would be extremely unlikely that the returnnext bug 
could be repeated. Putting them in a separate file with a nice comment 
block would be another alternative.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

Jim Nasby  writes:
> Hmm... I suspect there's more places where this could be a problem. For 
> example, pltcl_quote calls palloc, which could ereport as well.

Yeah.  I looked at that but couldn't get terribly excited about it,
because AFAICS, Tcl in general is apt to fall over under sufficient
memory pressure.  There are too many API functions with no failure
return provision, even though they clearly must do memory allocation
under the hood.  (The fact that they've even got ckalloc() tells you
what their philosophy is here: they're content to PANIC on OOM.)

I think pltcl should attempt to cover any error conditions that aren't
purely out-of-memory ones, but in a quick scan I didn't see any other
hazards.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 1/9/17 4:23 PM, Tom Lane wrote:

Jim Nasby  writes:

Got a stack trace. The abort happens in TclObjLookupVar:


Yeah, I found the problem: pltcl_returnnext calls pltcl_build_tuple_result
which may throw elog(ERROR), leaving the Tcl interpreter's state all
screwed up, so that later functions go south.  It seems quite accidental
that this is only failing with 8.4.  We need pltcl_returnnext to use a
subtransction, like the other pltcl statements that can call into generic
PG code.  Working on a fix now.


Hmm... I suspect there's more places where this could be a problem. For 
example, pltcl_quote calls palloc, which could ereport as well.


Perhaps all of the internal commands should be wrapped in the 
pltcl_subtrans_begin() construct. The subtrans overhead would be 
unfortunate though. But AFAIK the only reason we need the subtrans is if 
we're underneath a TCL catch command, and there's probably a way to 
determine if that's the case.


BTW, now that I've seen this pattern in pltcl and plpythonu, I'm 
wondering if there might be some easier way to handle it through our 
error callbacks.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

Jim Nasby  writes:
> Got a stack trace. The abort happens in TclObjLookupVar:

Yeah, I found the problem: pltcl_returnnext calls pltcl_build_tuple_result
which may throw elog(ERROR), leaving the Tcl interpreter's state all
screwed up, so that later functions go south.  It seems quite accidental
that this is only failing with 8.4.  We need pltcl_returnnext to use a
subtransction, like the other pltcl statements that can call into generic
PG code.  Working on a fix now.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 1/9/17 3:12 PM, Jim Nasby wrote:

I'm compiling 8.4 now, will see if I can duplicate.


Got a stack trace. The abort happens in TclObjLookupVar:

if (nsPtr->varResProc != NULL || iPtr->resolverPtr != NULL) {

nsPtr itself is NULL.


* thread #1: tid = 0x, 0x00010949bca8 
libtcl8.4g.dylib`TclObjLookupVar(interp=0x7fdc7b508ba0, part1Ptr=0x7fdc7d009090, 
part2=0x, flags=5, msg="set", createPart1=1, createPart2=1, 
arrayPtrPtr=0x7fff5712e3c0) + 440 at tclVar.c:400, stop reason = signal SIGSTOP
  * frame #0: 0x00010949bca8 
libtcl8.4g.dylib`TclObjLookupVar(interp=0x7fdc7b508ba0, part1Ptr=0x7fdc7d009090, 
part2=0x, flags=5, msg="set", createPart1=1, createPart2=1, 
arrayPtrPtr=0x7fff5712e3c0) + 440 at tclVar.c:400
frame #1: 0x00010949d22e 
libtcl8.4g.dylib`Tcl_ObjSetVar2(interp=0x7fdc7b508ba0, 
part1Ptr=0x7fdc7d009090, part2Ptr=0x, 
newValuePtr=0x7fdc7c00e800, flags=5) + 170 at tclVar.c:1517
frame #2: 0x00010941407a libtcl8.4g.dylib`Tcl_AddObjErrorInfo(interp=0x7fdc7b508ba0, 
message="\ninvoked from within\n\"__PLTcl_proc_16466 {quote foo bar}\"", 
length=-1) + 460 at tclBasic.c:6530
frame #3: 0x000109411514 libtcl8.4g.dylib`Tcl_LogCommandInfo(interp=0x7fdc7b508ba0, 
script="__PLTcl_proc_16466 {quote foo bar}", command="__PLTcl_proc_16466 {quote foo 
bar}", length=34) + 488 at tclBasic.c:3521
frame #4: 0x0001094112fa 
libtcl8.4g.dylib`Tcl_EvalObjv(interp=0x7fdc7b508ba0, objc=2, 
objv=0x7fff5712e6d0, flags=393216) + 740 at tclBasic.c:3428
frame #5: 0x00010941247d 
libtcl8.4g.dylib`Tcl_EvalObjEx(interp=0x7fdc7b508ba0, 
objPtr=0x7fdc7c001260, flags=393216) + 376 at tclBasic.c:5109
frame #6: 0x0001093efe24 pltcl.so`pltcl_func_handler + 660
frame #7: 0x0001093ef186 pltcl.so`pltcl_handler + 246
frame #8: 0x000108c3ff91 postgres`ExecMakeFunctionResultNoSets + 209
frame #9: 0x000108c3f42e postgres`ExecProject + 590
frame #10: 0x000108c56153 postgres`ExecResult + 179
frame #11: 0x000108c38bae postgres`ExecProcNode + 94
frame #12: 0x000108c35010 postgres`standard_ExecutorRun + 288
frame #13: 0x000108d5ab3f postgres`PortalRunSelect + 239
frame #14: 0x000108d5a744 postgres`PortalRun + 500
frame #15: 0x000108d58919 postgres`PostgresMain + 8745
frame #16: 0x000108cec483 postgres`PostmasterMain + 7443
frame #17: 0x000108c717ba postgres`main + 1562
frame #18: 0x7fff8dd765ad libdyld.dylib`start + 1



--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 1/9/17 1:22 PM, Tom Lane wrote:

I wrote:

Pushed with that and some other, mostly-cosmetic changes.


Hmm, looks like the new test cases have turned up a pre-existing bug.
Some of the buildfarm is showing crashes :-(.  It looks like all the
unhappy critters are running Tcl 8.4.something, which might be a
coincidence but I bet not.

I get the impression it's a stack clobber or memory clobber associated
with reporting Tcl errors back to PG, but haven't been able to isolate
it yet.


I notice the failures are also on "unusual" platforms, with only 1 being 
x86. Though, that's very possibly the only animals running 8.4...


I'm compiling 8.4 now, will see if I can duplicate.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

I wrote:
> Pushed with that and some other, mostly-cosmetic changes.

Hmm, looks like the new test cases have turned up a pre-existing bug.
Some of the buildfarm is showing crashes :-(.  It looks like all the
unhappy critters are running Tcl 8.4.something, which might be a
coincidence but I bet not.

I get the impression it's a stack clobber or memory clobber associated
with reporting Tcl errors back to PG, but haven't been able to isolate
it yet.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

Jim Nasby  writes:
> On 1/8/17 11:25 AM, Tom Lane wrote:
>> But I don't understand
>> how you got the sample output shown in the patch.  Is this based
>> on some unsubmitted changes in pltcl's error handling?

> Maybe it's a version difference?
> echo 'puts [info patchlevel];exit 0' | tclsh
> 8.6.6

Mmm, yeah, I'm on 8.5.13.  Evidently what we're looking at here is a
change in what Tcl puts into $::errorCode for this error.  That being
the case, we can't use $::errorCode for the regression test output, or
it'll fail depending on Tcl version.  I changed it to just return "$err",
ie the basic error message.  It might turn out that that's
version-dependent too, but the buildfarm should tell us.

Pushed with that and some other, mostly-cosmetic changes.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 1/8/17 11:25 AM, Tom Lane wrote:

But I don't understand
how you got the sample output shown in the patch.  Is this based
on some unsubmitted changes in pltcl's error handling?


AFAICT you've got everything. What I had on my end is:

create function public.tcl_error_handling_test(text)
 returns text
 language pltcl
as $function$
if {[catch $1 err]} {
# Set keys that will change over time to fixed values
array set myArray $::errorCode
set myArray(funcname) "'funcname'"
set myArray(lineno) 'lineno'
set myArray(POSTGRES) 'POSTGRES'

# Format into something nicer
set vals []
foreach {key} [lsort [array names myArray]] {
set value [string map {"\n" "\n\t"} $myArray($key)]
lappend vals "$key: $value"
}
return [join $vals "\n"]
} else {
return "no error"
}
$function$
;

Maybe it's a version difference?

echo 'puts [info patchlevel];exit 0' | tclsh
8.6.6

Anyway, attached is a complete new patch that fixes that issue (and a 
couple test diffs I missed :/), as well as the utf_e2u issue you 
discovered. I've applied this patch to master via git apply and run it 
through make check-world, so hopefully this puts the horse out to pasture.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --git a/src/pl/tcl/expected/pltcl_queries.out 
b/src/pl/tcl/expected/pltcl_queries.out
index 3a9fef3447..564ec8a294 100644
--- a/src/pl/tcl/expected/pltcl_queries.out
+++ b/src/pl/tcl/expected/pltcl_queries.out
@@ -1,3 +1,7 @@
+BEGIN;
+SET LOCAL client_min_messages = WARNING;
+CREATE EXTENSION IF NOT EXISTS plpgsql;
+COMMIT;
 -- suppress CONTEXT so that function OIDs aren't in output
 \set VERBOSITY terse
 insert into T_pkey1 values (1, 'key1-1', 'test key');
@@ -185,12 +189,23 @@ select * from T_pkey2 order by key1 using @<, key2 
collate "C";
 
 -- show dump of trigger data
 insert into trigger_test values(1,'insert');
-NOTICE:  NEW: {i: 1, v: insert}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: INSERT
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
insert}
 NOTICE:  OLD: {}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: INSERT
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
@@ -232,13 +247,37 @@ NOTICE:  TG_table_name: trigger_test_view
 NOTICE:  TG_table_schema: public
 NOTICE:  TG_when: {INSTEAD OF}
 NOTICE:  args: {24 {skidoo view}}
+update trigger_test set v = 'update', test_skip=true where i = 1;
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: UPDATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  SKIPPING OPERATION UPDATE
 update trigger_test set v = 'update' where i = 1;
-NOTICE:  NEW: {i: 1, v: update}
-NOTICE:  OLD: {i: 1, v: insert}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: UPDATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
update}
+NOTICE:  OLD: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
insert}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: UPDATE
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
@@ -246,16 +285,39 @@ NOTICE:  TG_when: BEFORE
 NOTICE:  args: {23 skidoo}
 delete from trigger_test;
 NOTICE:  NEW: {}
-NOTICE:  OLD: {i: 1, v: update}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: DELETE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: 

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

Jim Nasby  writes:
> On 1/6/17 2:17 PM, Tom Lane wrote:
>> This is in a format that neither patch(1) nor "git apply" recognize.
>> Please resubmit in a more usual format, diff -c or diff -u perhaps.

> Odd, dunno what happened there. New patch attached.

This applies, but I fail to get the expected regression test output:

*** /home/postgres/pgsql/src/pl/tcl/expected/pltcl_queries.out  Sun Jan  8 
11:54:19 2017
--- /home/postgres/pgsql/src/pl/tcl/results/pltcl_queries.out   Sun Jan  8 
12:18:27 2017
***
*** 515,529 
  select tcl_eval('spi_prepare a "b {"');
  ERROR:  unmatched open brace in list
  select tcl_error_handling_test($tcl${ spi_prepare "moo" }$tcl$);
!tcl_error_handling_test
! --
!  COMMAND:  spi_prepare "moo" +
!  POSTGRES: 'POSTGRES'+
!  TCL: LOOKUP +
!  funcname: 'funcname'+
!  lineno: 'lineno'
! (1 row)
! 
  -- test full error text
  select tcl_error_handling_test($tcl$
  spi_exec "DO $$
--- 515,521 
  select tcl_eval('spi_prepare a "b {"');
  ERROR:  unmatched open brace in list
  select tcl_error_handling_test($tcl${ spi_prepare "moo" }$tcl$);
! ERROR:  list must have an even number of elements
  -- test full error text
  select tcl_error_handling_test($tcl$
  spi_exec "DO $$


Investigation shows that $::errorCode contains just "NONE", which
is why the "array set myArray $::errorCode" is blowing up.  And
that's sort of what I'd expect, because $err is
invalid command name " spi_prepare "moo" "
which is not a Postgres-detected error.  So probably the example
tcl_error_handling_test function should not dispense with the
if {[lindex $::errorCode 0] == "POSTGRES"}
guard that we recommend in the manual.  But I don't understand
how you got the sample output shown in the patch.  Is this based
on some unsubmitted changes in pltcl's error handling?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 1/6/17 2:17 PM, Tom Lane wrote:

Jim Nasby  writes:

On 10/31/16 3:24 PM, Jim Nasby wrote:

This patch increases test coverage for pltcl, from 70% to 83%. Aside
from that, the work on this uncovered 2 new bugs (the trigger return one
I just submitted, as well as a bug in the SRF/composite patch).



Rebased patch attached. Test coverage is now at 85% (by line count).


This is in a format that neither patch(1) nor "git apply" recognize.
Please resubmit in a more usual format, diff -c or diff -u perhaps.


Odd, dunno what happened there. New patch attached.

BTW, this also includes some case changes back to lowercase. My muscle 
memory would prefer to go the other direction but I figured consistency 
was worth something. Feel free not to include that bit if you want.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --git a/src/pl/tcl/expected/pltcl_queries.out 
b/src/pl/tcl/expected/pltcl_queries.out
index 3a9fef3447..4794e88a5b 100644
--- a/src/pl/tcl/expected/pltcl_queries.out
+++ b/src/pl/tcl/expected/pltcl_queries.out
@@ -1,3 +1,7 @@
+BEGIN;
+SET LOCAL client_min_messages = WARNING;
+CREATE EXTENSION IF NOT EXISTS plpgsql;
+COMMIT;
 -- suppress CONTEXT so that function OIDs aren't in output
 \set VERBOSITY terse
 insert into T_pkey1 values (1, 'key1-1', 'test key');
@@ -185,12 +189,23 @@ select * from T_pkey2 order by key1 using @<, key2 
collate "C";
 
 -- show dump of trigger data
 insert into trigger_test values(1,'insert');
-NOTICE:  NEW: {i: 1, v: insert}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: INSERT
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
insert}
 NOTICE:  OLD: {}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: INSERT
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
@@ -232,13 +247,37 @@ NOTICE:  TG_table_name: trigger_test_view
 NOTICE:  TG_table_schema: public
 NOTICE:  TG_when: {INSTEAD OF}
 NOTICE:  args: {24 {skidoo view}}
+update trigger_test set v = 'update', test_skip=true where i = 1;
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: UPDATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  SKIPPING OPERATION UPDATE
 update trigger_test set v = 'update' where i = 1;
-NOTICE:  NEW: {i: 1, v: update}
-NOTICE:  OLD: {i: 1, v: insert}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: UPDATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
update}
+NOTICE:  OLD: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
insert}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: UPDATE
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
@@ -246,16 +285,39 @@ NOTICE:  TG_when: BEFORE
 NOTICE:  args: {23 skidoo}
 delete from trigger_test;
 NOTICE:  NEW: {}
-NOTICE:  OLD: {i: 1, v: update}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: DELETE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
update}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: DELETE
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
 NOTICE:  

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

Jim Nasby  writes:
> On 10/31/16 3:24 PM, Jim Nasby wrote:
>> This patch increases test coverage for pltcl, from 70% to 83%. Aside
>> from that, the work on this uncovered 2 new bugs (the trigger return one
>> I just submitted, as well as a bug in the SRF/composite patch).

> Rebased patch attached. Test coverage is now at 85% (by line count).

This is in a format that neither patch(1) nor "git apply" recognize.
Please resubmit in a more usual format, diff -c or diff -u perhaps.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

On 10/31/16 3:24 PM, Jim Nasby wrote:

This patch increases test coverage for pltcl, from 70% to 83%. Aside
from that, the work on this uncovered 2 new bugs (the trigger return one
I just submitted, as well as a bug in the SRF/composite patch).


Rebased patch attached. Test coverage is now at 85% (by line count).

Original patch by Karl Lehenbauer. Work sponsored by FlightAware.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --cc src/pl/tcl/expected/pltcl_queries.out
index 3a9fef3447,9e3a0dcd0b..00
--- a/src/pl/tcl/expected/pltcl_queries.out
+++ b/src/pl/tcl/expected/pltcl_queries.out
@@@ -288,6 -350,20 +350,22 @@@ select tcl_argisnull(null)
   t
  (1 row)
  
+ -- should error
+ insert into trigger_test(test_argisnull) values(true);
+ NOTICE:  NEW: {}
+ NOTICE:  OLD: {}
+ NOTICE:  TG_level: STATEMENT
+ NOTICE:  TG_name: statement_trigger
+ NOTICE:  TG_op: INSERT
+ NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+ NOTICE:  TG_relid: bogus:12345
+ NOTICE:  TG_table_name: trigger_test
+ NOTICE:  TG_table_schema: public
+ NOTICE:  TG_when: BEFORE
+ NOTICE:  args: {42 {statement trigger}}
+ ERROR:  argisnull cannot be used in triggers
++select trigger_data();
++ERROR:  trigger functions can only be called as triggers
  -- Test spi_lastoid primitive
  create temp table t1 (f1 int);
  select tcl_lastoid('t1');
@@@ -303,64 -379,216 +381,277 @@@ select tcl_lastoid('t2') > 0
   t
  (1 row)
  
 --- Test quote
 +-- test some error cases
- CREATE FUNCTION tcl_error(OUT a int, OUT b int) AS $$return {$$ LANGUAGE 
pltcl;
- SELECT tcl_error();
++create function tcl_error(out a int, out b int) as $$return {$$ language 
pltcl;
++select tcl_error();
 +ERROR:  missing close-brace
- CREATE FUNCTION bad_record(OUT a text, OUT b text) AS $$return [list a]$$ 
LANGUAGE pltcl;
- SELECT bad_record();
++create function bad_record(out a text, out b text) as $$return [list a]$$ 
language pltcl;
++select bad_record();
 +ERROR:  column name/value list must have even number of elements
- CREATE FUNCTION bad_field(OUT a text, OUT b text) AS $$return [list a 1 b 2 
cow 3]$$ LANGUAGE pltcl;
- SELECT bad_field();
++create function bad_field(out a text, out b text) as $$return [list a 1 b 2 
cow 3]$$ language pltcl;
++select bad_field();
 +ERROR:  column name/value list contains nonexistent column name "cow"
 +-- test compound return
 +select * from tcl_test_cube_squared(5);
 + squared | cubed 
 +-+---
 +  25 |   125
 +(1 row)
 +
- -- test SRF
++-- test srf
 +select * from tcl_test_squared_rows(0,5);
 + x | y  
 +---+
 + 0 |  0
 + 1 |  1
 + 2 |  4
 + 3 |  9
 + 4 | 16
 +(5 rows)
 +
 +select * from tcl_test_sequence(0,5) as a;
 + a 
 +---
 + 0
 + 1
 + 2
 + 3
 + 4
 +(5 rows)
 +
 +select 1, tcl_test_sequence(0,5);
 + ?column? | tcl_test_sequence 
 +--+---
 +1 | 0
 +1 | 1
 +1 | 2
 +1 | 3
 +1 | 4
 +(5 rows)
 +
- CREATE FUNCTION non_srf() RETURNS int AS $$return_next 1$$ LANGUAGE pltcl;
++create function non_srf() returns int as $$return_next 1$$ language pltcl;
 +select non_srf();
 +ERROR:  return_next cannot be used in non-set-returning functions
- CREATE FUNCTION bad_record_srf(OUT a text, OUT b text) RETURNS SETOF record 
AS $$
++create function bad_record_srf(out a text, out b text) returns setof record 
as $$
 +return_next [list a]
- $$ LANGUAGE pltcl;
- SELECT bad_record_srf();
++$$ language pltcl;
++select bad_record_srf();
 +ERROR:  column name/value list must have even number of elements
- CREATE FUNCTION bad_field_srf(OUT a text, OUT b text) RETURNS SETOF record AS 
$$
++create function bad_field_srf(out a text, out b text) returns setof record as 
$$
 +return_next [list a 1 b 2 cow 3]
- $$ LANGUAGE pltcl;
- SELECT bad_field_srf();
++$$ language pltcl;
++select bad_field_srf();
 +ERROR:  column name/value list contains nonexistent column name "cow"
++-- test quote
+ select tcl_eval('quote foo bar');
+ ERROR:  wrong # args: should be "quote string"
+ select tcl_eval('quote [format %c 39]');
+  tcl_eval 
+ --
+  ''
+ (1 row)
+ 
+ select tcl_eval('quote [format %c 92]');
+  tcl_eval 
+ --
+  \\
+ (1 row)
+ 
+ -- Test argisnull
+ select tcl_eval('argisnull');
+ ERROR:  wrong # args: should be "argisnull argno"
+ select tcl_eval('argisnull 14');
+ ERROR:  argno out of range
+ select tcl_eval('argisnull abc');
+ ERROR:  expected integer but got "abc"
+ -- Test return_null
+ select tcl_eval('return_null 14');
+ ERROR:  wrong # args: should be "return_null "
+ -- should error
+ insert into trigger_test(test_return_null) values(true);
+ NOTICE:  NEW: {}
+ NOTICE:  OLD: {}
+ NOTICE:  TG_level: STATEMENT
+ NOTICE:  TG_name: statement_trigger
+ NOTICE:  TG_op: INSERT
+ NOTICE:  

[HACKERS] Increase pltcl test coverage

[Jim Nasby]

This patch increases test coverage for pltcl, from 70% to 83%. Aside 
from that, the work on this uncovered 2 new bugs (the trigger return one 
I just submitted, as well as a bug in the SRF/composite patch).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
diff --git a/src/pl/tcl/expected/pltcl_queries.out 
b/src/pl/tcl/expected/pltcl_queries.out
index 6cb1fdb..9e3a0dc 100644
--- a/src/pl/tcl/expected/pltcl_queries.out
+++ b/src/pl/tcl/expected/pltcl_queries.out
@@ -1,3 +1,7 @@
+BEGIN;
+SET LOCAL client_min_messages = WARNING;
+CREATE EXTENSION IF NOT EXISTS plpgsql;
+COMMIT;
 -- suppress CONTEXT so that function OIDs aren't in output
 \set VERBOSITY terse
 insert into T_pkey1 values (1, 'key1-1', 'test key');
@@ -185,12 +189,23 @@ select * from T_pkey2 order by key1 using @<, key2 
collate "C";
 
 -- show dump of trigger data
 insert into trigger_test values(1,'insert');
-NOTICE:  NEW: {i: 1, v: insert}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: INSERT
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
insert}
 NOTICE:  OLD: {}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: INSERT
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
@@ -232,13 +247,37 @@ NOTICE:  TG_table_name: trigger_test_view
 NOTICE:  TG_table_schema: public
 NOTICE:  TG_when: {INSTEAD OF}
 NOTICE:  args: {24 {skidoo view}}
+update trigger_test set v = 'update', test_skip=true where i = 1;
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: UPDATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  SKIPPING OPERATION UPDATE
 update trigger_test set v = 'update' where i = 1;
-NOTICE:  NEW: {i: 1, v: update}
-NOTICE:  OLD: {i: 1, v: insert}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: UPDATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
update}
+NOTICE:  OLD: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
insert}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: UPDATE
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
@@ -246,16 +285,39 @@ NOTICE:  TG_when: BEFORE
 NOTICE:  args: {23 skidoo}
 delete from trigger_test;
 NOTICE:  NEW: {}
-NOTICE:  OLD: {i: 1, v: update}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: DELETE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
+NOTICE:  NEW: {}
+NOTICE:  OLD: {i: 1, test_argisnull: f, test_return_null: f, test_skip: f, v: 
update}
 NOTICE:  TG_level: ROW
 NOTICE:  TG_name: show_trigger_data_trig
 NOTICE:  TG_op: DELETE
-NOTICE:  TG_relatts: {{} i v}
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
 NOTICE:  TG_relid: bogus:12345
 NOTICE:  TG_table_name: trigger_test
 NOTICE:  TG_table_schema: public
 NOTICE:  TG_when: BEFORE
 NOTICE:  args: {23 skidoo}
+truncate trigger_test;
+NOTICE:  NEW: {}
+NOTICE:  OLD: {}
+NOTICE:  TG_level: STATEMENT
+NOTICE:  TG_name: statement_trigger
+NOTICE:  TG_op: TRUNCATE
+NOTICE:  TG_relatts: {{} i v {} test_skip test_return_null test_argisnull}
+NOTICE:  TG_relid: bogus:12345
+NOTICE:  TG_table_name: trigger_test
+NOTICE:  TG_table_schema: public
+NOTICE:  TG_when: BEFORE
+NOTICE:  args: {42 {statement trigger}}
 -- Test composite-type arguments
 select tcl_composite_arg_ref1(row('tkey', 42, 'ref2'));
  tcl_composite_arg_ref1 
@@ -288,6 +350,20 @@