Horton

Thread

Commits

  1. Expand the regression tests for PL/Tcl.

  1. Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-10-31T20:24:13Z

    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

  2. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-12-29T21:46:56Z

    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)

  3. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-06T20:17:10Z

    Jim Nasby <Jim.Nasby@BlueTreble.com> 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

  4. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-08T00:13:31Z

    On 1/6/17 2:17 PM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> 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)

  5. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-08T17:25:07Z

    Jim Nasby <Jim.Nasby@bluetreble.com> 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

  6. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-09T00:21:36Z

    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)

  7. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-09T15:17:20Z

    Jim Nasby <Jim.Nasby@bluetreble.com> 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

  8. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-09T19:22:14Z

    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

  9. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-09T21:12:07Z

    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)

  10. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-09T21:45:57Z

    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 = 0x0000, 0x000000010949bca8 libtcl8.4g.dylib`TclObjLookupVar(interp=0x00007fdc7b508ba0, part1Ptr=0x00007fdc7d009090, part2=0x0000000000000000, flags=5, msg="set", createPart1=1, createPart2=1, arrayPtrPtr=0x00007fff5712e3c0) + 440 at tclVar.c:400, stop reason = signal SIGSTOP
>   * frame #0: 0x000000010949bca8 libtcl8.4g.dylib`TclObjLookupVar(interp=0x00007fdc7b508ba0, part1Ptr=0x00007fdc7d009090, part2=0x0000000000000000, flags=5, msg="set", createPart1=1, createPart2=1, arrayPtrPtr=0x00007fff5712e3c0) + 440 at tclVar.c:400
>     frame #1: 0x000000010949d22e libtcl8.4g.dylib`Tcl_ObjSetVar2(interp=0x00007fdc7b508ba0, part1Ptr=0x00007fdc7d009090, part2Ptr=0x0000000000000000, newValuePtr=0x00007fdc7c00e800, flags=5) + 170 at tclVar.c:1517
>     frame #2: 0x000000010941407a libtcl8.4g.dylib`Tcl_AddObjErrorInfo(interp=0x00007fdc7b508ba0, message="\n    invoked from within\n\"__PLTcl_proc_16466 {quote foo bar}\"", length=-1) + 460 at tclBasic.c:6530
>     frame #3: 0x0000000109411514 libtcl8.4g.dylib`Tcl_LogCommandInfo(interp=0x00007fdc7b508ba0, script="__PLTcl_proc_16466 {quote foo bar}", command="__PLTcl_proc_16466 {quote foo bar}", length=34) + 488 at tclBasic.c:3521
>     frame #4: 0x00000001094112fa libtcl8.4g.dylib`Tcl_EvalObjv(interp=0x00007fdc7b508ba0, objc=2, objv=0x00007fff5712e6d0, flags=393216) + 740 at tclBasic.c:3428
>     frame #5: 0x000000010941247d libtcl8.4g.dylib`Tcl_EvalObjEx(interp=0x00007fdc7b508ba0, objPtr=0x00007fdc7c001260, flags=393216) + 376 at tclBasic.c:5109
>     frame #6: 0x00000001093efe24 pltcl.so`pltcl_func_handler + 660
>     frame #7: 0x00000001093ef186 pltcl.so`pltcl_handler + 246
>     frame #8: 0x0000000108c3ff91 postgres`ExecMakeFunctionResultNoSets + 209
>     frame #9: 0x0000000108c3f42e postgres`ExecProject + 590
>     frame #10: 0x0000000108c56153 postgres`ExecResult + 179
>     frame #11: 0x0000000108c38bae postgres`ExecProcNode + 94
>     frame #12: 0x0000000108c35010 postgres`standard_ExecutorRun + 288
>     frame #13: 0x0000000108d5ab3f postgres`PortalRunSelect + 239
>     frame #14: 0x0000000108d5a744 postgres`PortalRun + 500
>     frame #15: 0x0000000108d58919 postgres`PostgresMain + 8745
>     frame #16: 0x0000000108cec483 postgres`PostmasterMain + 7443
>     frame #17: 0x0000000108c717ba postgres`main + 1562
>     frame #18: 0x00007fff8dd765ad 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)

  11. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-09T22:23:06Z

    Jim Nasby <Jim.Nasby@bluetreble.com> 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

  12. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-09T23:05:48Z

    On 1/9/17 4:23 PM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@bluetreble.com> 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)

  13. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-09T23:38:27Z

    Jim Nasby <Jim.Nasby@bluetreble.com> 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

  14. Re: Increase pltcl test coverage

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-10T00:38:02Z

    On 1/9/17 5:38 PM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@bluetreble.com> 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)

  15. Re: Increase pltcl test coverage

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-10T01:27:54Z

    Jim Nasby <Jim.Nasby@bluetreble.com> 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