Re: [HACKERS] Back-patch use of unnamed POSIX semaphores for Linux?

[Alex Hunsaker]

On Wed, Dec 7, 2016 at 3:42 PM, Tom Lane  wrote:

>
>
> Hmm ... after further experimentation, I still can't get this version of
> systemd (231) to do anything evil.  It turns out that Fedora ships it with
> KillUserProcesses turned off by default, and maybe having that on is a
> prerequisite for the other behavior?  But that doesn't make a lot of sense
> because we'd never be seeing the reports of databases moaning about lost
> semaphores if the processes got killed first.  Anyway, I see nothing bad
> happening if KillUserProcesses is off, while if it's on then the database
> gets shut down reasonably politely via SIGTERM.
>
> Color me confused ... maybe systemd's behavior has changed?
>

Hrm, the following incantation seems to break for me on a fresh Fedora 25
system:
1) As root su to $USER and start postgres.
2) ssh in as $USER and then logout
3) # psql localhost

FATAL: semctl(4980742, 3, SETVAL, 0) failed: Invalid argument
LOG: server process (PID 14569) exited with exit code 1
...

Re: [HACKERS] Back-patch use of unnamed POSIX semaphores for Linux?

[Alex Hunsaker]

On Wed, Dec 7, 2016 at 1:12 PM, Tom Lane  wrote:


> But this is all kind of moot if Peter is right that systemd will zap
> POSIX shmem along with SysV semaphores.  I've been trying to reproduce
> the issue on a Fedora 25 installation, and so far I can't get it to
> zap anything, so I'm a bit at a loss how to prove things one way or
> the other.
>


Don't know precisely about Fedora 25, but I've had success in the past with:
ssh in as the user
start postgres under tmux/screen
logout
do another ssh login/logout cycle

After logon, you should see "/usr/lib/systemd/systemd --user" running for
that
user. After logout out, said proc should exit. If either of those is not
true,
either systemd is not setup to track sessions (probably via pam) or it
thinks
you still have an active logon. Another way to check if systemd thinks the
user
is logged in is if /run/user/$USER exists.

Re: [HACKERS] empty array case in plperl_ref_from_pg_array not handled correctly

[Alex Hunsaker]

On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund  wrote:

> Hi,
>
> Per the new valgrind animal we get:
>
>
> http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=2016-03-08%2004%3A22%3A00
> 2016-03-08 05:56:05.566 UTC [56de6971.723:5] LOG:  statement: select
> plperl_sum_array('{}');
> ==1827== Invalid write of size 4
> ==1827==at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459)
>
>
[ I think you may have meant to CC me not Alexey K. I'm probably the person
responsible :D. ]

Indeed, I think the simplest fix is to make plperl_ref_from_pg_array()
return an "empty" array in that case. The attached fixes the valgrind
warning for me. (cassert enabled build on master).


>
> ISTM the assumption that an array always has a dimension is a bit more
> widespread... E.g. split_array() looks like it'd not work nicely with a
> zero dimensional array...
>

Hrm, plperl_ref_from_pg_array() is the only caller of split_array(), so I
added an Assert() to make its contract more clear.

Recursive calls to split_array() should be fine as nested 0 dim arrays get
collapsed down to single 0 dim array (I hand tested it nonetheless):
 # select ARRAY[ARRAY[ARRAY[]]]::int[];
 array
───
 {}
(1 row)

Looking around, everything that makes an array seems to pass through
plperl_ref_from_pg_array(), so I think once that is fixed all is good. I
also looked for dimensions and ARR_NDIM() just to make sure (didn't find
anything fishy).

Thanks,


plperl_array_valgrind.patch
Description: Binary data

-- 
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] error message diff with Perl 5.22.0

[Alex Hunsaker]

On Sat, Jun 6, 2015 at 7:03 PM, Peter Eisentraut pete...@gmx.net wrote:

 With the recently released Perl 5.22.0, the tests fail thus:

 -ERROR:  Global symbol $global requires explicit package name at line 3.
 -Global symbol $other_global requires explicit package name at line 4.
 +ERROR:  Global symbol $global requires explicit package name (did you
 forget to declare my $global?) at line 3.
 +Global symbol $other_global requires explicit package name (did you
 forget to declare my $other_global?) at line 4.
  CONTEXT:  compilation of PL/Perl function uses_global


FWIW the committed expected file seems fine to me. There is not a perl
option to toggle this behavior (and even if there was, I think the
resulting changes to pl/perl functions we be quite ugly).

(What about the back branches? :D)

Re: [HACKERS] Precedence of standard comparison operators

[Alex Hunsaker]

On Tue, Mar 10, 2015 at 10:11 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 I wrote:

 This thread seems to have died off without any clear resolution.  I'd
 hoped somebody would try the patch on some nontrivial application to
 see if it broke anything or caused any warnings, but it doesn't seem
 like that is happening.


I tried it on a fairly typical web application. Around 5000 or so distinct
statements according to pg_stat_statements. With
operator_precedence_warning = on, no warnings yet.

Re: [HACKERS] [BUGS] BUG #9223: plperlu result memory leak

[Alex Hunsaker]

On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:

 Can I bug you into verifying what supported releases need this patch,
 and to which does it backpatch cleanly?  And if there's any to which it
 doesn't, can I further bug you into providing one that does?

Sure! Not bugging at all. I'll dig into this in a few hours.


-- 
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] [BUGS] BUG #9223: plperlu result memory leak

[Alex Hunsaker]

On Wed, Mar 5, 2014 at 12:55 PM, Alex Hunsaker bada...@gmail.com wrote:
 On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:

 Can I bug you into verifying what supported releases need this patch,
 and to which does it backpatch cleanly?  And if there's any to which it
 doesn't, can I further bug you into providing one that does?

 Sure! Not bugging at all. I'll dig into this in a few hours.

This will apply cleanly all the way to REL9_2_STABLE. It applies (with
fuzz, but cleanly to REL9_1). REL9_0 does this completely differently
and so does not have this leak.

Thanks!


-- 
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] [BUGS] BUG #9223: plperlu result memory leak

[Alex Hunsaker]

On Tue, Feb 25, 2014 at 6:56 AM, Sergey Burladyan eshkin...@gmail.com wrote:

 It looks like I found the problem, Perl use reference count and something that
 is called Mortal for memory management.  As I understand it, mortal is free
 after FREETMPS. Plperl call FREETMPS in plperl_call_perl_func() but after it,
 plperl ask perl interpreter again for new mortal SV variables, for example, in
 hek2cstr from plperl_sv_to_datum, and this new SV is newer freed.

So I think hek2cstr is the only place we leak (its the only place I
can see that allocates a mortal sv without being wrapped in
ENTER/SAVETMPS/FREETMPS/LEAVE).

Does the attached fix it for you?
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 930b9f0..3bc4034 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -304,6 +304,16 @@ static char *setlocale_perl(int category, char *locale);
 static char *
 hek2cstr(HE *he)
 {
+	char *ret;
+	SV	 *sv;
+
+	/*
+	 * HeSVKEY_force will return a temporary mortal SV*, so we need to make
+	 * sure to free it with ENTER/SAVE/FREE/LEAVE
+	 */
+	ENTER;
+	SAVETMPS;
+
 	/*-
 	 * Unfortunately,  while HeUTF8 is true for most things  256, for values
 	 * 128..255 it's not, but perl will treat them as unicode code points if
@@ -328,11 +338,17 @@ hek2cstr(HE *he)
 	 * right thing
 	 *-
 	 */
-	SV		   *sv = HeSVKEY_force(he);
 
+	sv = HeSVKEY_force(he);
 	if (HeUTF8(he))
 		SvUTF8_on(sv);
-	return sv2cstr(sv);
+	ret = sv2cstr(sv);
+
+	/* free sv */
+	FREETMPS;
+	LEAVE;
+
+	return ret;
 }
 
 /*

-- 
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] Memory leakage associated with plperl spi_prepare/spi_freeplan

[Alex Hunsaker]

On Fri, Mar 1, 2013 at 7:38 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Alex Hunsaker bada...@gmail.com writes:

 Applied with some fixes.

Thanks! Your version looks much better than mine.

  One annonce is it still leaks :-(.

 I fixed that, at least for the function-lifespan leakage from
 spi_prepare() --- is that what you meant?


Yep, I don't see the leak with your version.


-- 
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] Memory leakage associated with plperl spi_prepare/spi_freeplan

[Alex Hunsaker]

On Tue, Feb 26, 2013 at 3:56 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 I'm inclined to think the right fix is to make a small memory context
 for each prepared plan made by plperl_spi_prepare().  The qdesc for it
 could be made right in the context (getting rid of the unchecked
 malloc's near the top of the function), the FmgrInfos and their
 subsidiary data could live there too, and plperl_spi_freeplan could
 replace its retail free's with a single MemoryContextDelete.


Seemed fairly trivial, find the above approach in the attached. I added a
CHECK_FOR_INTERRUPTS() at the top of plperl_spi_prepare(), it was fairly
annoying that I couldn't ctrl+c my way out of test function.

One annonce is it still leaks :-(. I tracked it down and it seemed to stem
from parseTypeString().  I chased down the rabbit hole for a bit, but
nothing jumped out... raw_parser() is a bit of a black box to me. Adding
the seemingly obvious list_free(raw_parsetree_list); or setting the memory
context before parseTypeString() didn't seem to do much.

It would be nice to squish the other leaks due to perm_fmgr_info()... but
this is a start.


plperl_spi_leak.patch
Description: Binary data

-- 
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] plperl sigfpe reset can crash the server

[Alex Hunsaker]

On Fri, Aug 24, 2012 at 4:10 PM, Andres Freund and...@2ndquadrant.com wrote:

 We probably should workaround that bug anyway given that its a pretty trivial
 DOS using only a trusted language and it will take quite some time to push out
 newer perl versions even if that bug gets fixed.

 Doing a pqsignal(SIGFPE, FloatExceptionHandler) after PERL_SYS_INIT3 seems to
 work. Is that acceptable?

Makes sense to me. (I have not looked to see if there is some perl
knob we can flip for this)


-- 
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] Unexpected plperl difference between 8.4 and 9.1

[Alex Hunsaker]

On Mon, Aug 20, 2012 at 10:14 AM, Alvaro Herrera
alvhe...@2ndquadrant.comwrote:

 Excerpts from Alex Hunsaker's message of lun ago 20 12:03:11 -0400 2012:
  On Sun, Aug 19, 2012 at 2:26 PM, Joel Jacobson j...@trustly.com wrote:
 
   After upgrading from 8.4 to 9.1, one of my plperl functions stopped
   working properly.

 I can reproduce the failure with 5.14.2


Me too, however it works for me with 5.14.1, looking more like a strange
perl bug.

I've tried reproducing this in straight perl but I don't think I can
without using some C code, specifically the call to SvPVutf8 in sv2cstr()
seems to be the culprit. If I change that to SvPV() it seems to work. Im
wondering if there is some strange caching of utf8 strings going on that =~
m// is not clearing.

Ill keep digging and hopefully be able to narrow it down to a commit
between 5.14.1 and 5.14.2 so we can understand more whats going here.

Re: [HACKERS] Unexpected plperl difference between 8.4 and 9.1

[Alex Hunsaker]

On Sun, Aug 19, 2012 at 2:26 PM, Joel Jacobson j...@trustly.com wrote:

 After upgrading from 8.4 to 9.1, one of my plperl functions stopped
 working properly.

 For some reason, when matching a string using a regex, the $1 variable
 cannot be returned directly using return_next() but must be
 set to a variable first.
 If returned directly, it appears to be cached in some strange way,
 returning the same value for all 10 rows in the example below.


Hrm seems to work for me. What version of perl is this?
$ perl -V
Summary of my perl5 (revision 5 version 16 subversion 0) configuration:
[snip]
Characteristics of this binary (from libperl):
  Compile-time options: HAS_TIMES MYMALLOC PERLIO_LAYERS
PERL_DONT_CREATE_GVSV PERL_MALLOC_WRAP
PERL_PRESERVE_IVUV USE_64_BIT_ALL USE_64_BIT_INT
USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE
USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_PERLIO
USE_PERL_ATOF
$!psql
baroque= SELECT version();
 version

-
 PostgreSQL 9.1.5 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.7.1
20120721 (prerelease), 64-bit
(1 row)

baroque= CREATE OR REPLACE FUNCTION test1() RETURNS SETOF NUMERIC AS $BODY$
baroque$ use strict;
baroque$ use warnings;
baroque$ for(my $i=0 ; $i10; $i++) {
baroque$ my $rand = rand();
baroque$ $rand =~ m/(.*)/;
baroque$ return_next($1);
baroque$ }
baroque$ return;
baroque$ $BODY$ LANGUAGE plperl;
CREATE FUNCTION
baroque=
baroque= select * from test1();
   test1
---
 0.284491935120062
 0.213769321886019
 0.758221121077565
 0.810816779589864
 0.649781285447791
 0.630792307420037
  0.17897035660857
 0.876314955338863
 0.899575315174307
 0.225134707347706
(10 rows)

[HACKERS] Re: [HACKERS] PL/Perl build problem: error: ‘OP_SETSTATE’ undeclared

[Alex Hunsaker]

On Sun, Aug 12, 2012 at 9:57 PM, Peter Eisentraut pete...@gmx.net wrote:

 It appears that a recent Perl version (I have 5.14.2) has eliminated
 OP_SETSTATE, which causes the current PostgreSQL build to fail:

 plperl.c: In function ‘_PG_init’:
 plperl.c:442:5645: error: ‘OP_SETSTATE’ undeclared (first use in this
 function)
 plperl.c:442:5645: note: each undeclared identifier is reported only once
 for each function it appears in


Hrm, Thats strange, PLPERL_SET_OPMASK() is generated by plperl_opmask.pl--
that should use whatever OP's your current perl has defined (They come from
the use Opcode at the top and unless you somehow installed a different
Opcode module than what your perl has...).

Im running a non threaded 5.16.0 and I don't see OP_SETSTATE anywhere:
$ pwd
/home/alex/src/postgresql/src/pl/plperl
$ grep -RI 'OP_SETSTATE' *
$

I know i've used 5.14 in the past successfully.  Wat happens if you
regenerate plperl_opmask.h? (rm plperl_opmask.h  make)

Re: [HACKERS] Support for array_remove and array_replace functions

[Alex Hunsaker]

On Wed, Jul 11, 2012 at 9:54 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Marco Nenciarini marco.nenciar...@2ndquadrant.it writes:
 Patch v3 attached.

 I'm looking at this patch now.  The restriction of array_remove to
 one-dimensional arrays seems a bit annoying.  I see the difficulty:
 if the input is multi-dimensional then removing some elements could
 lead to a non-rectangular array, which isn't supported.  However,
 that could be dealt with by decreeing that the *result* is
 one-dimensional and of the necessary length, regardless of the
 dimensionality of the input.

Makes sense to me. +1

The other option ISTM is to replace removed entries with NULL-- which
I don't really like.

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

Re: [SPAM] [MessageLimit][lowlimit] Re: [HACKERS] pl/perl and utf-8 in sql_ascii databases

[Alex Hunsaker]

On Wed, Jul 11, 2012 at 1:42 PM, Alvaro Herrera
alvhe...@commandprompt.com wrote:

 I have pushed these changes to HEAD, 9.2 and 9.1.  Instead of the games
 with plperl_lc_*.out being copied around, I just used the ASCII version
 as plperl_lc_1.out and the UTF8 one as plperl_lc.out.

 ... and this story hasn't ended yet, because one of the new tests is
 failing.  See here:

 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=magpiedt=2012-07-11%2010%3A00%3A04

 [...]
   SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
 ! ERROR:  character with byte sequence 0xe5 0xb7 0x9d in encoding UTF8 has 
 no equivalent in encoding LATIN1
 ! CONTEXT:  PL/Perl function perl_utf_inout


 I am not sure what can we do here other than remove this function and
 query from the test.

Hrm, me neither. I say drop em.

-- 
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] pl/perl and utf-8 in sql_ascii databases

[Alex Hunsaker]

On Tue, Jul 3, 2012 at 2:59 AM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
 Hello, Here is regression test runs on pg's also built with
 cygwin-gcc and VC++.

Thank you!

 The patches attached following,

 - plperl_sql_ascii-4.patch : fix for pl/perl utf8 vs sql_ascii
 - plperl_sql_ascii_regress-1.patch : regression test for this patch.
  I added some tests on encoding to this.

 I will mark this patch as 'ready for committer' after this.

Both patches look good to me..

 I've been stuck in mud trying to plperl work on windows
 environment. I saw many messages complaining that plperl wouldn't
 be built to work. For the convenience of those and myself, I
 describe the process of building postgresql with plperl on
 Windows with cygwin and VC++ I've done below.

Hrm, I don't develop on windows here, but out of curiosity, what were
the messages like?

 - The remainder of the patch whic fixes the easy fixable leakes
   of palloc'ed memory won't be ported into 9.1. This is only for
   9.3dev.

 What should I do for this?

Just let the commiter decide? :-)

-- 
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] Support for array_remove and array_replace functions

[Alex Hunsaker]

On Sat, Jun 30, 2012 at 3:28 PM, Marco Nenciarini
marco.nenciar...@2ndquadrant.it wrote:

 On 30/06/2012 04:16, Alex Hunsaker wrote:
 
  Hi, I've been reviewing this patch.
 
  Good documentation, and regression tests. The code looked fine but I
  didn't care for the code duplication between array_replace and
  array_remove so I merged those into a helper function,
  array_replace_internal(). Thoughts?

 It looks reasonable.

 There was a typo in array_replace which was caught by regression tests.
 I've fixed the typo and changed a comment in array_replace_internal.

 Patch v3 attached.

Looks good to me, marked ready for commiter.

-- 
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] Support for array_remove and array_replace functions

[Alex Hunsaker]

On Thu, Jun 14, 2012 at 4:41 AM, Marco Nenciarini 
marco.nenciar...@2ndquadrant.it wrote:

 Hi,

  following Gabriele's email regarding our previous patch on Foreign
 Key Arrays[1], I am sending a subset of that patch which includes only
 two array functions which will be needed in that patch: array_remove
 (limited to single-dimensional arrays) and array_replace.



  The patch includes changes to the documentation.


Hi, I've been reviewing this patch.

Good documentation, and regression tests. The code looked fine but I didn't
care for the code duplication between array_replace and array_remove so I
merged those into a helper function, array_replace_internal(). Thoughts?

Other than that it all looks good to me.


array-functions_v2.patch.bz2
Description: BZip2 compressed data

-- 
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] pl/perl and utf-8 in sql_ascii databases

[Alex Hunsaker]

On Wed, Jun 20, 2012 at 1:15 PM, Alvaro Herrera
alvhe...@commandprompt.com wrote:
 Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:

 Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
 and SvPVUTF8() when turning a perl string into a cstring.

 Right.

 So I played a bit with this patch, and touched it a bit mainly just to
 add some more comments; and while at it I noticed that some of the
 functions in Util.xs might leak some memory, so I made an attempt to
 plug them, as in the attached patch (which supersedes yours).

I think most of these leaks go back to 9.0. Dunno if its worth
backpatching them...

 to test the problem in the original report, I notice that we now have a
 regression failure:

 I'm not really sure what to do here -- maybe have a second expected file
 for that test is a good enough answer?  Or should I just take the test
 out?  Opinions please.

I think we have broken that check twice so it seems like it would be
nice to keep. But I don't feel *to* strongly about it.

The comment and cleanups all look good to me.

Thanks!

-- 
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] pl/perl and utf-8 in sql_ascii databases

[Alex Hunsaker]

On Thu, Jun 21, 2012 at 5:22 AM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:

 So I played a bit with this patch, and touched it a bit mainly
 [...] functions in Util.xs might leak some memory, so I made an attempt to

 Ok, Is it ok to look into the newer patch including fix of leaks
 at first?

Yeah :-).

 Variable naming in util_quote_*() seems a bit confusing,

 Renaming ret to quoted and str to ret as the patch attached might
 make it easily readable.

Ok.

 [ regression failure on a  SQL_ASCII database ]
 I'm not really sure what to do here -- maybe have a second expected file
 for that test is a good enough answer?  Or should I just take the test
 out?  Opinions please.

 The attached ugly patch does it. We seem should put NO_LOCALE=1
 on the 'make check' command line for the encodings not compatible
 with the environmental locale, although it looks work.

+REGRESS_LC0 = $(subst .sql,,$(shell cd sql; ls plperl_lc_$(shell echo
$(ENCODING) | tr A-Z- a-z_).sql 2/dev/null))
+REGRESS_LC = $(if $(REGRESS_LC0),$(REGRESS_LC0),plperl_lc)
+REGRESS = plperl $(REGRESS_LC) plperl_trigger plperl_shared
plperl_elog plperl_util plperl_init plperlu plperl_array

Hrm, that's quite cute. I dunno if there is a more cannon way of doing
the above-- but it seems to work. I'm not sure this regression test is
worth it. I'm thinking maybe we should just remove
theegressionegression test instead.

There is a minor issue with the patch where
sql/plperl_lc_sql_ascii.sql contains the text plperl_lc.sql. After
copying sql/plperl_lc.sql to sql/plperl_lc_sql_ascii.sql everything
worked as described.

Thanks for the review!

-- 
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] pl/perl and utf-8 in sql_ascii databases

[Alex Hunsaker]

On Mon, Jun 18, 2012 at 1:30 PM, Alvaro Herrera
alvhe...@commandprompt.com wrote:

 Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:

 Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
 and SvPVUTF8() when turning a perl string into a cstring.

 Hmm, this patch belongs into back branches too, right?  Not just the
 current development tree?

(Have been out of town, sorry for the late reply)

Yeah, back to 9.1.

-- 
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] plperl_helpers.h fix for clang

[Alex Hunsaker]

On Thu, May 24, 2012 at 11:19 AM, Peter Eisentraut pete...@gmx.net wrote:
 clang warns about that newish SvREFCNT_inc(sv) call in plperl_helpers.h
 about an unused return value, because the macro expansion of
 SvREFCNT_inc(sv) returns sv.  The merit of that warning might be
 debatable, but it seems easy to fix by using SvREFCNT_inc_void(sv)
 instead.

 And we could use SvREFCNT_inc_simple_void(sv), since sv doesn't have any
 side effects, but that's optional.

 Any concerns?

Hrm I can't seem to find either of those functions in my copy of perl
5.8.1 or 5.16.0. I also looked for _inc_ in the event I got casing
wrong :-(.

Perhaps I misunderstood, are you proposing to introduce those
functions? Im fine with that. Or doing (void)SvREFCNT_inc().

-- 
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] plperl_helpers.h fix for clang

[Alex Hunsaker]

On Thu, May 24, 2012 at 11:31 AM, Alex Hunsaker bada...@gmail.com wrote:
 On Thu, May 24, 2012 at 11:19 AM, Peter Eisentraut pete...@gmx.net wrote:

 And we could use SvREFCNT_inc_simple_void(sv), since sv doesn't have any
 side effects, but that's optional.

 Hrm I can't seem to find either of those functions in my copy of perl
 5.8.1 or 5.16.0. I also looked for _inc_ in the event I got casing
 wrong :-(.

Doh, it is indeed there in 5.16.0, looks like it got added in 5.10
:-(. (I was on the wrong branch...).

-- 
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] plperl_helpers.h fix for clang

[Alex Hunsaker]

On Thu, May 24, 2012 at 12:03 PM, Peter Eisentraut pete...@gmx.net wrote:
 On tor, 2012-05-24 at 11:36 -0600, Alex Hunsaker wrote:

 Doh, it is indeed there in 5.16.0, looks like it got added in 5.10
 :-(. (I was on the wrong branch...).

 It's in ppport.h.

Don't see any reason not to then. +1 for the simple_void variant.

[ I completely forgot about ppport.h and was just checking against the
perl source. :-) ]

-- 
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] [COMMITTERS] pgsql: Add new keywords SNAPSHOT and TYPES to the keyword list in gram.

[Alex Hunsaker]

On Thu, Feb 9, 2012 at 11:30, Tom Lane t...@sss.pgh.pa.us wrote:
 Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Tom Lane's message of jue feb 09 12:17:59 -0300 2012:

 FWIW that script is throwing a warning here:
 Use of assignment to $[ is deprecated at 
 /pgsql/source/HEAD/src/tools/check_keywords.pl line 19.

 Any Perl hackers want to improve that script?

The attached 2 liner does it for me.
*** a/src/tools/check_keywords.pl
--- b/src/tools/check_keywords.pl
***
*** 16,22  if (@ARGV) {
  	$path = .;
  }
  
- $[ = 1;			# set array base to 1
  $, = ' ';		# set output field separator
  $\ = \n;		# set output record separator
  
--- 16,21 
***
*** 60,66  line: while (GRAM) {
  $n = (@arr = split(' ', $S));
  
  # Ok, we're in a keyword list. Go through each field in turn
! for (my $fieldIndexer = 1; $fieldIndexer = $n; $fieldIndexer++) {
  	if ($arr[$fieldIndexer] eq '*/'  $comment) {
  	$comment = 0;
  	next;
--- 59,65 
  $n = (@arr = split(' ', $S));
  
  # Ok, we're in a keyword list. Go through each field in turn
! for (my $fieldIndexer = 0; $fieldIndexer  $n; $fieldIndexer++) {
  	if ($arr[$fieldIndexer] eq '*/'  $comment) {
  	$comment = 0;
  	next;

-- 
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] pl/perl and utf-8 in sql_ascii databases

[Alex Hunsaker]

On Thu, Feb 9, 2012 at 03:21, Christoph Berg c...@df7cb.de wrote:
 Hi,

 we have a database that is storing strings in various encodings (and
 non-encodings, namely the arbitrary byte soup [ ... ]
 For this reason, the database uses
 sql_ascii encoding

 ...snip...

 In sql_ascii databases, utf_e2u does not do any recoding, but then
 SvUTF8_on still marks the string as utf-8, while it isn't.

 (Returned values might also need fixing.)

 In my view, this is clearly a bug in pl/perl on sql_ascii databases.

Yeah, there was some musing about this over in:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg01142.php

Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
and SvPVUTF8() when turning a perl string into a cstring.

With the attached I get:
= create or replace function perl_white(a text) returns text as $$
return shift; $$ language plperlu;
= select perl_white(E'\200'), perl_white(E'\200')::bytea,
coalesce(perl_white(E'\200'), 'null');
 perl_white | perl_white | coalesce
++--
| \x80   |

= select perl_white(E'\401');
 perl_white

 \x01
(1 row)

Does the attached fix the issue for you?

Ill note that all the pls seem to behave a bit differently:

= create or replace function py_white(a text) returns text as $$
return a; $$ language plpython3u;
= select py_white(E'\200'), py_white(E'\200')::bytea,
coalesce(py_white(E'\200'), 'null');
py_white | py_white | coalesce
--+--+--
  |  | null
(1 row)

=select py_white(E'\401');
 py_white
--
 \x01
(1 row)

= create or replace function tcl_white(text) returns text as $$
return $1; $$ language pltcl;
= select tcl_white(E'\200'), tcl_white(E'\200')::bytea,
coalesce(tcl_white(E'\200'), 'null');
 tcl_white | tcl_white | coalesce
---+---+--
   | \x80  |

 = select tcl_white(E'\402');
 tcl_white
---
 \x02
(1 row)
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 5,23 
   * convert from utf8 to database encoding
   */
  static inline char *
! utf_u2e(const char *utf8_str, size_t len)
  {
! 	int 	enc = GetDatabaseEncoding();
! 
! 	char	   *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, enc);
  
  	/*
! 	* when we are a PG_UTF8 or SQL_ASCII database
! 	* pg_do_encoding_conversion() will not do any conversion or
! 	* verification. we need to do it manually instead.
  	*/
  	if (enc == PG_UTF8 || enc == PG_SQL_ASCII)
! 		pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
  
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
--- 5,24 
   * convert from utf8 to database encoding
   */
  static inline char *
! utf_u2e(char *utf8_str, size_t len)
  {
! 	int		   enc = GetDatabaseEncoding();
! 	char	   *ret = utf8_str;
  
  	/*
! 	* when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
! 	* will not do any conversion or verification. we need to do it manually
! 	* instead.
  	*/
  	if (enc == PG_UTF8 || enc == PG_SQL_ASCII)
! 		pg_verify_mbstr_len(enc, utf8_str, len, false);
! 	else
! 		ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, enc);
  
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
***
*** 66,72  sv2cstr(SV *sv)
  		 * we are done */
  		SvREFCNT_inc(sv);
  
! 	val = SvPVutf8(sv, len);
  
  	/*
  	 * we use perl's length in the event we had an embedded null byte to ensure
--- 67,80 
  		 * we are done */
  		SvREFCNT_inc(sv);
  
! 	/*
! 	 * when SQL_ASCII just treat it as byte soup, that is fetch the string out
! 	 * however it is currently stored by perl
! 	 */
! 	if (GetDatabaseEncoding() == PG_SQL_ASCII)
! 		val = SvPV(sv, len);
! 	else
! 		val = SvPVutf8(sv, len);
  
  	/*
  	 * we use perl's length in the event we had an embedded null byte to ensure
***
*** 89,99  static inline SV *
  cstr2sv(const char *str)
  {
  	SV		   *sv;
! 	char	   *utf8_str = utf_e2u(str);
  
  	sv = newSVpv(utf8_str, 0);
  	SvUTF8_on(sv);
- 
  	pfree(utf8_str);
  
  	return sv;
--- 97,112 
  cstr2sv(const char *str)
  {
  	SV		   *sv;
! 	char	   *utf8_str;
! 
! 	/* no conversion when SQL_ASCII */
! 	if (GetDatabaseEncoding() == PG_SQL_ASCII)
! 		return newSVpv(str, 0);
! 
! 	utf8_str = utf_e2u(str);
  
  	sv = newSVpv(utf8_str, 0);
  	SvUTF8_on(sv);
  	pfree(utf8_str);
  
  	return sv;

-- 
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] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

[Alex Hunsaker]

On Fri, Jan 13, 2012 at 16:07, Andrew Dunstan and...@dunslane.net wrote:


 On 01/12/2012 09:28 PM, Alex Hunsaker wrote:

 Util.c/o not depending on plperl_helpers.h was also throwing me for a loop
 so I fixed it and SPI.c... Thoughts?


 Basically looks good, but I'm confused by this:

   do language plperl $$ elog('NOTICE', ${^TAINT}); $$;



 Why is NOTICE quoted here? It's constant that we've been careful to set up.

No reason.

[ Err well It was just part of me flailing around while trying to
figure out why elog was still crashing even thought I had the issue
fixed. The real problem was Util.o was not being recompiled due to
Util.c not depending on plperl_helpers.h) ]

-- 
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] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

[Alex Hunsaker]

On Fri, Jan 6, 2012 at 14:05, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 Oh my... I dunno exactly what I was smoking last night, but its a good
 thing I didn't share :-). Uh so my test program was also completely
 wrong, Ill have to redo it all. I've narrowed it down to:
         if ((type == SVt_PVGV || SvREADONLY(sv)))
         {
             if (type != SVt_PV 
                 type != SVt_NV)
             {
                 sv = newSVsv(sv);
             }
        }

 Has anyone tried looking at the source code for SvPVutf8 to see exactly
 what cases it fails on?  The fact that there's an explicit croak() call
 makes me think it might not be terribly hard to tell.

Well its easy to find the message, its not so easy to trace it back up
:-). It is perl source code after all. It *looks* like its just:
sv.c:
Perl_sv_pvn_force_flags(SV *sv, STRLEN, I32 flags)
{
 [ Flags is SV_GMAGIC ]
if (SvREADONLY(sv)  !(flags  SV_MUTABLE_RETURN))
   // more or less...
   Perl_croak(aTHX_ Can't coerce readonly %s to string, ref)

if ((SvTYPE(sv)  SVt_PVLV  SvTYPE(sv) != SVt_PVFM)
|| isGV_with_GP(sv))
Perl_croak(aTHX_ Can't coerce %s to string in %s,
sv_reftype(sv,0),
}

Given that I added this hunk:
+
+   if (SvREADONLY(sv) ||
+   isGV_with_GP(sv) ||
+   (SvTYPE(sv)  SVt_PVLV  SvTYPE(sv) != SVt_PVFM))
+   sv = newSVsv(sv);
+   else
+   /* increase the reference count so we cant just
SvREFCNT_dec() it when
+* we are done */
+   SvREFCNT_inc(sv);

And viola all of these work (both in 5.14 and 5.8.9, although 5.8.9
gives different notices...)

do language plperl $$ elog(NOTICE, *foo); $$;
NOTICE:  *main::foo
CONTEXT:  PL/Perl anonymous code block

do language plperl $$ elog(NOTICE, $^V); $$;
NOTICE:  v5.14.2
CONTEXT:  PL/Perl anonymous code block

do language plperl $$ elog(NOTICE, ${^TAINT}); $$;
NOTICE:  0
CONTEXT:  PL/Perl anonymous code block

So I've done that in the attached patch. ${^TAINT} seemed to be the
only case that gave consistent notices in 5.8.9 and up so I added it
to the regression tests.

Util.c/o not depending on plperl_helpers.h was also throwing me for a
loop so I fixed it and SPI.c...

Thoughts?
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 72,82  perlchunks.h: $(PERLCHUNKS)
  
  all: all-lib
  
! SPI.c: SPI.xs
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
  	$(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
! Util.c: Util.xs
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
  	$(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
--- 72,82 
  
  all: all-lib
  
! SPI.c: SPI.xs plperl_helpers.h
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
  	$(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
! Util.c: Util.xs plperl_helpers.h
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
  	$(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
*** a/src/pl/plperl/expected/plperl_elog.out
--- b/src/pl/plperl/expected/plperl_elog.out
***
*** 58,60  select uses_global();
--- 58,62 
   uses_global worked
  (1 row)
  
+ -- make sure we don't choke on readonly values
+ do language plperl $$ elog('NOTICE', ${^TAINT}); $$;
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 47,74  sv2cstr(SV *sv)
  {
  	char	   *val, *res;
  	STRLEN		len;
- 	SV *nsv;
  
  	/*
  	 * get a utf8 encoded char * out of perl. *note* it may not be valid utf8!
  	 *
  	 * SvPVutf8() croaks nastily on certain things, like typeglobs and
  	 * readonly objects such as $^V. That's a perl bug - it's not supposed to
! 	 * happen. To avoid crashing the backend, we make a copy of the
! 	 * sv before passing it to SvPVutf8(). The copy is garbage collected 
  	 * when we're done with it.
  	 */
! 	nsv = newSVsv(sv);
! 	val = SvPVutf8(nsv, len);
  
  	/*
  	 * we use perl's length in the event we had an embedded null byte to ensure
  	 * we error out properly
  	 */
! 	res =  utf_u2e(val, len);
  
  	/* safe now to garbage collect the new SV */
! 	SvREFCNT_dec(nsv);
  
  	return res;
  }
--- 47,81 
  {
  	char	   *val, *res;
  	STRLEN		len;
  
  	/*
  	 * get a utf8 encoded char * out of perl. *note* it may not be valid utf8!
  	 *
  	 * SvPVutf8() croaks nastily on certain things, like typeglobs and
  	 * readonly objects such as $^V. That's a perl bug - it's not supposed to
! 	 * happen. To avoid crashing the backend, we make a copy of the sv before
! 	 * passing it to SvPVutf8(). The copy is garbage collected

Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

[Alex Hunsaker]

On Fri, Jan 6, 2012 at 06:34, Andrew Dunstan and...@dunslane.net wrote:

 PFA that copies if its readonly and its not a scalar.

 I didn't bother adding regression tests-- should I have?

 I have several questions.

 1. How much are we actually saving here? newSVsv() ought to be pretty cheap,
 no? I imagine it's pretty heavily used inside the interpreter.

It will incur an extra copy for every return value from plperl and
every value passed to a spi function (and possibly other areas I
forgot about). Personally I think avoiding yet another copy of the
return value is worth it.

 2. Unless I'm insufficiently caffeinated right now, there's something wrong
 with this logic:

 !       if (SvREADONLY(sv) 
 !                       (type != SVt_IV ||
 !                       type != SVt_NV ||
 !                       type != SVt_PV))

Oh my... I dunno exactly what I was smoking last night, but its a good
thing I didn't share :-). Uh so my test program was also completely
wrong, Ill have to redo it all. I've narrowed it down to:
if ((type == SVt_PVGV || SvREADONLY(sv)))
{
if (type != SVt_PV 
type != SVt_NV)
{
sv = newSVsv(sv);
}
   }

One odd thing is we have to copy SVt_IV (plain numbers) as apparently
that is shared with refs (my $str = 's'; my $ref = \$str;).

Given this, #3 and the other unknowns we might run into in the future
I think if ((SvREADONLY(sv) || type == SVt_GVPV) proposed upthread
makes the most sense.

 3. The above is in any case almost certainly insufficient, because in my
 tests a typeglob didn't trigger SvREADONLY(), but did cause a crash.

Hrm the glob I was testing was *STDIN. It failed to fail in my test
program because I was not testing *STDIN directly but instead had
@test = (*STDIN); Ugh. Playing with it a bit more it seems only
*STDIN, *STDERR and *STDOUT have problems. For example this seems to
work fine for me:
do LANGUAGE plperlu $$ open(my $fh, '', '/tmp/t.tmp'); elog(NOTICE, $fh) $$;

 And yes, we should possibly add a regression test or two. Of course, we
 can't use the cause of the original complaint ($^V) in them, though.

I poked around  the perl source looking for some candidates elog(INFO,
${^TAINT}) works fine on 5.8.9 and 5.14.2. I thought we could get away
with elog(INFO, *STDIN) but 5.8.9 says:
NOTICE:
While other version of perl (5.14) say:
NOTICE:  *main::STDIN

-- 
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] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

[Alex Hunsaker]

On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan and...@dunslane.net wrote:
 Fix breakage from earlier plperl fix.

 Apparently the perl garbage collector was a bit too eager, so here
 we control when the new SV is garbage collected.

I know im a little late to the party...

I can't help but think this seems a bit inefficient for the common
case. Would it be worth only copying the sv when its a glob or
readonly? Something like the below? I tested a few more svtypes that
were easy to make (code, regexp) and everything seems peachy.

[ BTW it seems readonly in general is fine, for example elog(ERROR,
1); worked previously as well as elog(ERROR, 1);. Both of those sv's
are readonly. ISTM, at least on my version of perl (5.14.2), globs and
readonly vstrings seem to be the problem children. I think we could
get away with testing if its a glob or vstring. But I don't have time
right now to test all the way back to perl 5.8 and everything
in-between, Ill find it if anyone is interested.  ]

--

*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 47,53  sv2cstr(SV *sv)
  {
char   *val, *res;
STRLEN  len;
-   SV *nsv;

/*
 * get a utf8 encoded char * out of perl. *note* it may not be valid 
utf8!
--- 47,52 
***
*** 58,65  sv2cstr(SV *sv)
 * sv before passing it to SvPVutf8(). The copy is garbage collected
 * when we're done with it.
 */
!   nsv = newSVsv(sv);
!   val = SvPVutf8(nsv, len);

/*
 * we use perl's length in the event we had an embedded null byte to 
ensure
--- 57,68 
 * sv before passing it to SvPVutf8(). The copy is garbage collected
 * when we're done with it.
 */
!   if(SvTYPE(sv) == SVt_PVGV || SvREADONLY(sv))
!   sv = newSVsv(sv);
!   else
!   SvREFCNT_inc(sv);
!
!   val = SvPVutf8(sv, len);

/*
 * we use perl's length in the event we had an embedded null byte to 
ensure
***
*** 68,74  sv2cstr(SV *sv)
res =  utf_u2e(val, len);

/* safe now to garbage collect the new SV */
!   SvREFCNT_dec(nsv);

return res;
  }
--- 71,77 
res =  utf_u2e(val, len);

/* safe now to garbage collect the new SV */
!   SvREFCNT_dec(sv);

return res;
  }

-- 
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] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

[Alex Hunsaker]

On Thu, Jan 5, 2012 at 16:59, Andrew Dunstan and...@dunslane.net wrote:


 On 01/05/2012 06:31 PM, Alex Hunsaker wrote:

 On Thu, Jan 5, 2012 at 16:02, Andrew Dunstanand...@dunslane.net  wrote:

 Fix breakage from earlier plperl fix.

 I can't help but think this seems a bit inefficient

 So, yes, we should probably adjust this one more time, but ideally we need a
 better test than just SvREADONLY(). If you want to follow up your
 investigation of exactly when we need a copied SV and see how much you can
 narrow it down that would be great.

After further digging I found it chokes on any non scalar (IOW any
reference). I attached a simple c program that I tested with 5.8.9,
5.10.1, 5.12.4 and 5.14.2 (for those who did not know about it,
perlbrew made testing across all those perls relatively painless).

PFA that copies if its readonly and its not a scalar. Also I fixed up
Tom's complaint about having sv2cstr() inside do_util_elog's PG_TRY
block. I didn't bother fixing the ones in plperl.c tho-- some seemed
like they would require quite a bit of rejiggering.

I didn't bother adding regression tests-- should I have?
*** a/src/pl/plperl/Util.xs
--- b/src/pl/plperl/Util.xs
***
*** 37,47  static void
  do_util_elog(int level, SV *msg)
  {
  	MemoryContext oldcontext = CurrentMemoryContext;
! 	char	   * volatile cmsg = NULL;
  
  	PG_TRY();
  	{
- 		cmsg = sv2cstr(msg);
  		elog(level, %s, cmsg);
  		pfree(cmsg);
  	}
--- 37,46 
  do_util_elog(int level, SV *msg)
  {
  	MemoryContext oldcontext = CurrentMemoryContext;
! 	char	   * volatile cmsg = sv2cstr(msg);
  
  	PG_TRY();
  	{
  		elog(level, %s, cmsg);
  		pfree(cmsg);
  	}
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 47,74  sv2cstr(SV *sv)
  {
  	char	   *val, *res;
  	STRLEN		len;
! 	SV *nsv;
  
  	/*
  	 * get a utf8 encoded char * out of perl. *note* it may not be valid utf8!
  	 *
! 	 * SvPVutf8() croaks nastily on certain things, like typeglobs and
! 	 * readonly objects such as $^V. That's a perl bug - it's not supposed to
! 	 * happen. To avoid crashing the backend, we make a copy of the
! 	 * sv before passing it to SvPVutf8(). The copy is garbage collected 
! 	 * when we're done with it.
  	 */
! 	nsv = newSVsv(sv);
! 	val = SvPVutf8(nsv, len);
  
  	/*
  	 * we use perl's length in the event we had an embedded null byte to ensure
  	 * we error out properly
  	 */
! 	res =  utf_u2e(val, len);
  
  	/* safe now to garbage collect the new SV */
! 	SvREFCNT_dec(nsv);
  
  	return res;
  }
--- 47,79 
  {
  	char	   *val, *res;
  	STRLEN		len;
! 	svtype		type = SvTYPE(sv);
  
  	/*
  	 * get a utf8 encoded char * out of perl. *note* it may not be valid utf8!
  	 *
! 	 * SvPVutf8() croaks nastily on readonly refs, That's a perl bug - it's not
! 	 * supposed to happen. To avoid crashing the backend, we make a copy of the
! 	 * sv before passing it to SvPVutf8().
  	 */
! 	if (SvREADONLY(sv) 
! 			(type != SVt_IV ||
! 			type != SVt_NV ||
! 			type != SVt_PV))
! 		sv = newSVsv(sv);
! 	else
! 		SvREFCNT_inc(sv);
! 
! 	val = SvPVutf8(sv, len);
  
  	/*
  	 * we use perl's length in the event we had an embedded null byte to ensure
  	 * we error out properly
  	 */
! 	res = utf_u2e(val, len);
  
  	/* safe now to garbage collect the new SV */
! 	SvREFCNT_dec(sv);
  
  	return res;
  }
/*
 * compile with gcc  -O2 -ggdb `perl -MExtUtils::Embed -e ccopts -e ldopts` svutf8_ro_test.c
 */
#include EXTERN.h
#include perl.h

int main(void)
{
	char *embed[] = { , -e, 0 };
	int x;
	AV	*test;
	PerlInterpreter *perl;

	perl_construct(perl);
	perl_parse(perl, NULL, 3, embed, NULL);
	perl_run(perl);

	eval_pv(my $scalar = 'string';
			@test = (
			'string', 
			$scalar, 
			\\$scalar, 
			1, 
			1.5, 
			[], 
			{}, 
			$^V, ,
			v5.0.0, 
			sub {}, 
			qr//, 
			*STDIN, 
			bless({}, ''), 
			);, 1);

	test = get_av(test, 0);
	for(x=0; x=av_len(test); x++)
	{
		char *crap;
		STRLEN len;
		SV *sv = *av_fetch(test, x, 0);
		svtype type = SvTYPE(sv);

		SvREADONLY_on(sv);

		if (SvREADONLY(sv) 
type != SVt_IV ||
type != SVt_NV ||
type != SVt_PV)
			sv = newSVsv(sv);

		crap = SvPVutf8(sv, len);
	}

	perl_destruct(perl);
	perl_free(perl);

	return 0;
}

-- 
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] PL/Perl Does not Like vstrings

[Alex Hunsaker]

On Wed, Jan 4, 2012 at 13:13, Andrew Dunstan and...@dunslane.net wrote:


 On 01/04/2012 12:56 PM, Tom Lane wrote:

 I looked at that last night but it appeared that SvOK would be perfectly
 happy.  (Didn't actually try it, though, I was just eyeballing the flags
 in gdb.)


 I tested it and you're right, it doesn't help. I don't see what else we can
 do about it. There doesn't appear to be any test for an SV in the API.

I think about the best we can do is something along the lines of:

sv2cstr()
{
...
  if (Perl_vverify(sv))
   return utf_u2e(SvPV(sv));
...
}

I dont the the utf_u2e is strictly needed (other than that it strdups)
as I don't think versions can have utf8 chars, or at least that $^V
will not have utf8 chars (and even if it did it would only cause
problems if they had codepoints in 128  255).

We would still have issues with typeglobs...

-- 
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] Review: Non-inheritable check constraints

[Alex Hunsaker]

On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera
alvhe...@commandprompt.com wrote:

 Yeah.  Nikhil, Alex, this is the merged patch.  Have a look that it
 still works for you (particularly the pg_dump bits) and I'll commit it.
 I adjusted the regression test a bit too.

Other than the version checks seem to be off by one looks fine. I
assume I/we missed that in the original patch. I also adjusted the
version check in describe.c to be consistent with the other version
checks in that file (= 90200 instead of  90100).

(Also, nice catch on false AS as r.conisonly in describe.c)

--

*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***
*** 5996,6003  getTableAttrs(TableInfo *tblinfo, int numTables)
  tbinfo-dobj.name);

resetPQExpBuffer(q);
!   if (g_fout-remoteVersion = 90100)
{
appendPQExpBuffer(q, SELECT tableoid, oid, 
conname, 
   
pg_catalog.pg_get_constraintdef(oid) AS consrc, 
  conislocal, 
convalidated, conisonly 
--- 5996,6004 
  tbinfo-dobj.name);

resetPQExpBuffer(q);
!   if (g_fout-remoteVersion = 90200)
{
+   /* conisonly is new in 9.2 */
appendPQExpBuffer(q, SELECT tableoid, oid, 
conname, 
   
pg_catalog.pg_get_constraintdef(oid) AS consrc, 
  conislocal, 
convalidated, conisonly 
***
*** 6007,6012  getTableAttrs(TableInfo *tblinfo, int numTables)
--- 6008,6026 
  ORDER BY 
conname,
  
tbinfo-dobj.catId.oid);
}
+   else if (g_fout-remoteVersion = 90100)
+   {
+   /* conisvalidated is new in 9.1 */
+   appendPQExpBuffer(q, SELECT tableoid, oid, 
conname, 
+  
pg_catalog.pg_get_constraintdef(oid) AS consrc, 
+ conislocal, 
convalidated, 
+ false as 
conisonly 
+ FROM 
pg_catalog.pg_constraint 
+ WHERE 
conrelid = '%u'::pg_catalog.oid 
+AND 
contype = 'c' 
+ ORDER BY 
conname,
+ 
tbinfo-dobj.catId.oid);
+   }
else if (g_fout-remoteVersion = 80400)
{
appendPQExpBuffer(q, SELECT tableoid, oid, 
conname, 
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
***
*** 1783,1789  describeOneTableDetails(const char *schemaname,
{
char *is_only;

!   if (pset.sversion  90100)
is_only = r.conisonly;
else
is_only = false AS conisonly;
--- 1783,1789 
{
char *is_only;

!   if (pset.sversion = 90200)
is_only = r.conisonly;
else
is_only = false AS conisonly;

-- 
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] Review: Non-inheritable check constraints

[Alex Hunsaker]

On Fri, Dec 16, 2011 at 14:01, Alvaro Herrera
alvhe...@commandprompt.com wrote:

 Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011:

 On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera
 alvhe...@commandprompt.com wrote:

  Yeah.  Nikhil, Alex, this is the merged patch.  Have a look that it
  still works for you (particularly the pg_dump bits) and I'll commit it.
  I adjusted the regression test a bit too.

 Other than the version checks seem to be off by one looks fine.

 Uhm ... you're right that convalidated is present in 9.1 [...] So I
 don't think we really need to add a separate branch for 9.1 here, but it
 certainly needs a comment improvement.

Hrm... What am I missing?

$ inh_v4/bin/psql -c 'select version();' -d test
 version
-
 PostgreSQL 9.1.0 on x86_64-unknown-linux-gnu, compiled by gcc (GCC)
4.6.1 20110819 (prerelease), 64-bit
(1 row)

$ inh_v4/bin/pg_dump test
pg_dump: SQL command failed
pg_dump: Error message from server: ERROR:  column conisonly does not exist
LINE 1: ...aintdef(oid) AS consrc, conislocal, convalidated, conisonly ...
 ^
pg_dump: The command was: SELECT tableoid, oid, conname,
pg_catalog.pg_get_constraintdef(oid) AS consrc, conislocal,
convalidated, conisonly FROM pg_catalog.pg_constraint WHERE conrelid =
'237964'::pg_catalog.oidAND contype = 'c' ORDER BY conname

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Wed, Nov 2, 2011 at 17:12, Andrew Dunstan and...@dunslane.net wrote:
 Considering that the issue appears to have been ignored from
 mid-February until early October, I don't see why it should now get to
 jump to the head of the queue.  Other people may have different
 opinions, of course.

 Added. :-)


 I'm just starting to look at this, by way of a break in staring at pg_dump
 code ;-). This just needs to be backpatched to 9.1, is that right?

Yes please.

9.0 did not have this problem (or at least if it did it was a separate issue).

-- 
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] pl/perl example in the doc no longer works in 9.1

[Alex Hunsaker]

On Thu, Oct 13, 2011 at 16:05, Tom Lane t...@sss.pgh.pa.us wrote:

 Applied with some further hacking of my own to clean up memory leaks
 and grotty coding.

Thanks!

BTW after seeing it I agree passing in fcinfo (and the other fixes) to
plperl_sv_to_datum() is better.

-- 
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] pl/perl example in the doc no longer works in 9.1

[Alex Hunsaker]

On Wed, Oct 12, 2011 at 15:00, Tom Lane t...@sss.pgh.pa.us wrote:
 David E. Wheeler da...@kineticode.com writes:
 On Oct 12, 2011, at 9:15 AM, Tom Lane wrote:
 Well, the real question is why a function declared to return VOID cares
 at all about what the last command in its body is.  If this has changed
 since previous versions then I think it's a bug and we should fix it,
 not just change the example.

 It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to 
 add a bunch of bare return statements to existing functions.

[ For those that missed it, 9.0 is OK, it is indeed a bug in 9.1. ]

 This appears to have gotten broken in commit
 87bb2ade2ce646083f39d5ab3e3307490211ad04, which changed the function
 return code to go through plperl_sv_to_datum, which is making
 unwarranted assumptions ... but since it's utterly bereft of comments,
 it's hard for a non Perl hacker to identify exactly what it should do
 instead.

Yeah, its a mess.

 The core of the problem seems to be that if SvROK(sv) then
 the code assumes that it must be intended to convert that to an array or
 composite, no matter whether the declared result type of the function is
 compatible with such a thing.

Hrm, well 9.0 and below did not get this right either:
create or replace function test_hash() returns text as $$ return
{'a'=1}; $$ language plperl;
select test_array();
  test_array
---
 ARRAY(0x7fd92384dcb8)
(1 row)

create or replace function test_hash() returns text as $$ return
{'a'=1}; $$ language plperl;
select test_hash();
  test_hash
--
 HASH(0x7fd92387f848)
(1 row)


9.1 does this:
select test_array();
 test_array

 \x01
(1 row)

select test_hash();
ERROR:  type text is not composite
CONTEXT:  PL/Perl function test_hash

  So I think this probably broke not only
 VOID-result cases, but also cases where a Perl array or hash is supposed
 to be returned as, say, text.

Given the output above (both pre 9.1 and post) it seems unless the
type is a set or composite we should throw an error. Maybe PL/Perl
function returning type %s must not return a reference ?

  It would be more appropriate to drive the
 cases off the nature of the function result type, perhaps.

Ill see if I can cook up something that's not too invasive.

[ I have a patch to fix the VOID issues. it gets rid of that horrid
has_retval variable/logic and makes it look much closer to 9.0's code.
Unfortunately its on my laptop at home as I was hacking on it before I
went to work... ]

-- 
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] Patch: Perl xsubpp

[Alex Hunsaker]

On Wed, Oct 12, 2011 at 17:53, David E. Wheeler da...@kineticode.com wrote:
 On Sep 15, 2011, at 3:04 PM, Alex Hunsaker wrote:

 Close, seems I was wrong about the typemap ExtUtils::ParseXS does not
 install a new one so we still need to point to the one in privlib.
 Also xsubpp is not executable so the test should be -r or something.

 Also don't think we should change the configure switch tests to test 
 XSUBPPDIR.

 Find those plus some minor typos fixed in the attached.
 xsubpp_v3.patch
 --

 Doesn't look like this has been applied yet. I think it ought to be 
 backported, too, frankly. DId I miss it?

Nah, probably should add it to the next commit fest so it does not get
forgotten.

-- 
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] pl/perl example in the doc no longer works in 9.1

[Alex Hunsaker]

On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker bada...@gmail.com wrote:
 On Wed, Oct 12, 2011 at 15:00, Tom Lane t...@sss.pgh.pa.us wrote:

 The core of the problem seems to be that if SvROK(sv) then
 the code assumes that it must be intended to convert that to an array or
 composite, no matter whether the declared result type of the function is
 compatible with such a thing.

 Hrm, well 9.0 and below did not get this right either:
 create or replace function test_hash() returns text as $$ return
 {'a'=1}; $$ language plperl;
 select test_array();
      test_array
 ---
  ARRAY(0x7fd92384dcb8)
 (1 row)

 create or replace function test_hash() returns text as $$ return
 {'a'=1}; $$ language plperl;
 select test_hash();
      test_hash
 --
  HASH(0x7fd92387f848)
 (1 row)


 Given the output above (both pre 9.1 and post) it seems unless the
 type is a set or composite we should throw an error. Maybe PL/Perl
 function returning type %s must not return a reference ?

  It would be more appropriate to drive the
 cases off the nature of the function result type, perhaps.

 Ill see if I can cook up something that's not too invasive.

PFA my attempt at a fix.

This gets rid of of most of the if/else chain and the has_retval crap
in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
the lifting. It also now handles VOIDOID and checks that the request
result oid can be converted from the perl structure. For example if
you passed in a hashref with a result oid that was not an rowtype it
will error out with PL/Perl cannot convert hash to non rowtype %s.
Arrays behave similarly.

One side effect is you can now return a composite literal where you
could not before. ( We already let you return array literals )

The comments might still be a bit sparse-- Im hoping the added errors
make things a bit more self explanatory.

A large portion of the diff is added regression tests, testing what
happens when you return various references.

Comments?


plperl_returns.patch.gz
Description: GNU Zip compressed data

-- 
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] Review: Non-inheritable check constraints

[Alex Hunsaker]

On Fri, Oct 7, 2011 at 21:30, Nikhil Sontakke nikkh...@gmail.com wrote:
 Hi Alex,

 I guess we both are in agreement with each other :)

 After sleeping over it, I think that check is indeed dead code with this new
 non-inheritable check constraints functionality in place. So unless you have
 some other comments, we can mark this as 'Ready for Commiter'.

 Again, thanks for the thorough review and subsequent changes!

PFA an updated patch with the check removed and a comment or two added.

I've also marked it ready.


non_inh_check_v3.patch.gz
Description: GNU Zip compressed data

-- 
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] alter table only ... drop constraint broken in HEAD

[Alex Hunsaker]

On Sun, Oct 9, 2011 at 09:17, Greg Stark st...@mit.edu wrote:
 On Fri, Oct 7, 2011 at 5:45 PM, Alex Hunsaker bada...@gmail.com wrote:
 If I find the time maybe Ill submit something along these lines for
 the next commit fest.


 So i just picked up the non-inherited constraints patch and quickly
 ran into the same problem and found this thread.

 I think it makes sense to hold off on this patch until these issues
 are resolved. Because we really do need to test the cases when adding
 or removing child tables that have constraints with the same name as
 non-inherited parent tables. And I'm not sure what will happen in
 these cases once these issues are resolved.

Doesn't someone just need to commit Roberts patch? I suppose it could
do with a better review than my eyeballing... Maybe thats where the
hang up is?

-- 
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] Review: Non-inheritable check constraints

[Alex Hunsaker]

On Fri, Oct 7, 2011 at 00:28, Nikhil Sontakke nikkh...@gmail.com wrote:
 Hi Alex,

 So with it all spelled out now I see the constraint must be added to
 child tables too check is dead code.


 Thanks the above step-wise explanation helps.

 But AFAICS, the default inhOpt value can be governed by the SQL_inheritance
 guc too. So in that case, it's possible that recurse is false and child
 tables are present, no?

Well... Do we really want to differentiate between those two case? I
would argue no.

Given that:
  set sql_inhertance to off;
  alter table xxx alter column;
behaves the same as
  set sql_inhertance to on;
  alter table only xxx alter column;

Why should we treat constraints differently? Or put another way if set
sql_inhertance off makes alter table behave with an implicit only,
shouldn't add/drop constraint respect that?

 Infact as I now remember, the reason my patch was looping through was to
 handle this very case. It was based on the assumptions that some constraints
 might be ONLY type and some can be inheritable.
 Although admittedly the current ALTER TABLE functionality does not allow this.

Hrm... Ill I see is a user who turned off sql_inhertance wondering why
they have to specify ONLY on some alter table commands and not others.
I think if we want to support ONLY constraint types in the way you
are thinking about them, we need to put ONLY some place else (alter
table xxx add only constraint ?). Personally I don't see a reason to
have that kind of constraint. Mostly because I don't see how its
functionally different. Is it somehow?

Anyone else have any thoughts on this?

-- 
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] alter table only ... drop constraint broken in HEAD

[Alex Hunsaker]

On Fri, Oct 7, 2011 at 07:53, Robert Haas robertmh...@gmail.com wrote:

 The only way we could
 trip up in that case is if there were two identically named
 constraints.  We'd have to visit the first tuple, update it, then
 visit the second tuple, recurse (thus incrementing the command
 counter), and then visit the updated version of the first tuple.  And
 that should be impossible, because we've got code to disallow multiple
 constraints on the same relation with the same name (though no unique
 index, for some reason).

Surely an oversight...

  Still, that's a long chain of reasoning, so
 I'm wondering if we can't come up with something that is more
 obviously correct.

 If we're confident that the inner loop here should never iterate more
 than once (i.e. the lack of a unique index is not an ominous sign)
 then maybe we should just rewrite this so that the inner loop scans
 until it finds a match and then terminates.  Then, outside the loop,
 we check whether a tuple was found and if so process it - but without
 ever going back to look for another one.  See attached.

I eyeballed it and it does indeed seem simpler. My only thought is
perhaps we should add that missing unique index on (conrelid,
conname). If we are not going to support duplicate names in the code,
we might as well enforce it. No?

-- 
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] alter table only ... drop constraint broken in HEAD

[Alex Hunsaker]

On Fri, Oct 7, 2011 at 09:50, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Oct 7, 2011 at 11:19 AM, Alex Hunsaker bada...@gmail.com wrote:
 My only thought is
 perhaps we should add that missing unique index on (conrelid,
 conname). If we are not going to support duplicate names in the code,
 we might as well enforce it. No?

 Not sure.  There could be performance or other ramifications to that.
 For now I'm more interested in fixing this particular bug than I am in
 getting into a wider world of re-engineering...

Yeah, looking at the code a bit closer we would also want to fix
various places to take advantage of the index. Seems like it could be
a big win when you have thousands of constraints (albeit only in the
add/drop case).

If I find the time maybe Ill submit something along these lines for
the next commit fest.

Thanks!

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Wed, Oct 5, 2011 at 20:36, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Oct 5, 2011 at 5:03 PM, Alex Hunsaker bada...@gmail.com wrote:
 On Wed, Oct 5, 2011 at 08:18, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
 amit.khande...@enterprisedb.com wrote:
 I have no more issues with the patch.
 Thanks!

 I think this patch needs to be added to the open CommitFest, with
 links to the reviews, and marked Ready for Committer.

 The open commitfest? Even if its an important bug fix that should be
 backpatched?

 Considering that the issue appears to have been ignored from
 mid-February until early October, I don't see why it should now get to
 jump to the head of the queue.  Other people may have different
 opinions, of course.

Added. :-)

-- 
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] Review: Non-inheritable check constraints

[Alex Hunsaker]

On Thu, Oct 6, 2011 at 02:42, Nikhil Sontakke nikkh...@gmail.com wrote:
 Hi Alex,

 I didn't care for the changes to gram.y so I reworked it a bit so we
 now pass is_only to AddRelationNewConstraint() (like we do with
 is_local). Seemed simpler but maybe I missed something. Comments?


 Hmmm, your patch checks for a constraint being only via:

   !recurse  !recursing

 I hope that is good enough to conclusively conclude that the constraint is
 'only'. This check was not too readable in the existing code for me anyways
 ;). If we check at the grammar level, we can be sure. But I remember not
 being too comfortable about the right position to ascertain this
 characteristic.

Well I traced through it here was my thinking (maybe should be a comment?):

1: AlterTable() calls ATController() with recurse =
interpretInhOption(stmt-relation-inhOpt
2: ATController() calls ATPrepCmd() with recurse and recursing = false
3: ATPrepCmd() saves the recurse flag with the subtup
AT_AddConstraintRecurse, otherwise the subtype is AT_AddConstraint
4: ATExecCmd() calls ATExecAddConstraint() with recurse == true when
subtype == AT_AddConstraintRecurse, recurse = false otherwise
5: ATExecAddConstraint() calls ATAddCheckConstraint() with recuse and
recursing = false
6: now we are in ATAddCheckConstraint() where recurse ==
interpretInhOption(rv-inhOpt) and recursing == false. Recursing is
only true when ATAddCheckConstaint() loops through children and
recursively calls ATAddCheckConstraint()

So with it all spelled out now I see the constraint must be added to
child tables too check is dead code.

Ill take out that check and then mark it as ready for commiter (and
Ill add any comments about the logic of the recurse flag above if I
can think of a concise way to put it). Sound good?

-- 
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] alter table only ... drop constraint broken in HEAD

[Alex Hunsaker]

On Thu, Oct 6, 2011 at 07:24, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Oct 5, 2011 at 10:29 PM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Oct 5, 2011 at 5:53 PM, Alex Hunsaker bada...@gmail.com wrote:
 tldr:

 Seems to be broken by
 http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665
 :
 commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665
 Author: Robert Haas rh...@postgresql.org
 Date:   Mon Jun 27 10:27:17 2011 -0400

    Avoid having two copies of the HOT-chain search logic.


 Oh, I see the problem, and I now agree that it's the DROP CONSTRAINT
 code that is buggy.

Want me to roll this fix in as part of the alter table only constraint
patch? Or keep it split out? We might want to backpatch to at least
8.3 where HOT was introduced (yes looks like the bug existed back
then). I suppose its a fairly narrow chance to hit this bug so I could
see the argument for not back patching...

 I took a look around for other places that might have this same
 problem and didn't find any, but I'm not too confident that that means
 there are none there, since there are a fair number of things that can
 call CommandCounterIncrement() indirectly.

I skimmed for the direct easy to find ones as well. Didn't see any
other than the one you note below.

  shdepReassignOwned() does
 a direct CCI from within a scan, but ISTM that any update we do there
 would produce a new tuple that doesn't match the scan, so that should
 be OK.

Well its on purpose so I hope its ok :-)

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
amit.khande...@enterprisedb.com wrote:
 On 4 October 2011 22:57, Alex Hunsaker bada...@gmail.com wrote:
 On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
 amit.khande...@enterprisedb.com wrote:
 On 4 October 2011 14:04, Alex Hunsaker bada...@gmail.com wrote:
 On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
 amit.khande...@enterprisedb.com wrote:

 WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
 utf8_str, so pg_verify_mbstr_len() will not get called. [...]

 Consider a latin1 database where utf8_str was a string of ascii
 characters. [...]

 [Patch] Look ok to you?


 +       if(GetDatabaseEncoding() == PG_UTF8)
 +               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

 In your patch, the above will again skip mb-validation if the database
 encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
 the un-converted string even if *one* of the src and dest encodings is
 SQL_ASCII.

 *scratches head* I thought the point of SQL_ASCII was no encoding
 conversion was done and so there would be nothing to verify.

 Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
 NULL bytes in the string when we are a single byte encoding.

 I think :
        if (ret == utf8_str)
 +       {
 +               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
                ret = pstrdup(ret);
 +       }

 This (ret == utf8_str) condition would be a reliable way for knowing
 whether pg_do_encoding_conversion() has done the conversion at all.

 Yes. However (and maybe im nitpicking here), I dont see any reason to
 verify certain strings twice if we can avoid it.

 What do you think about:
 +    /*
 +    * when we are a PG_UTF8 or SQL_ASCII database 
 pg_do_encoding_conversion()
 +    * will not do any conversion or verification. we need to do it
 manually instead.
 +    */
 +       if( GetDatabaseEncoding() == PG_UTF8 ||
              GetDatabaseEncoding() == SQL_ASCII)
 +               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);


 You mean the final changes in plperl_helpers.h would look like
 something like this right? :

  static inline char *
  utf_u2e(const char *utf8_str, size_t len)
  {
        char       *ret = (char *) pg_do_encoding_conversion((unsigned
 char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());

        if (ret == utf8_str)
 +       {
 +               if (GetDatabaseEncoding() == PG_UTF8 ||
 +                       GetDatabaseEncoding() == PG_SQL_ASCII)
 +               {
 +                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
 +               }
 +
                ret = pstrdup(ret);
 +       }
        return ret;
  }

Yes.

 Yeah I am ok with that. It's just an additional check besides (ret ==
 utf8_str) to know if we really require validation.


-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Wed, Oct 5, 2011 at 00:30, Alex Hunsaker bada...@gmail.com wrote:
 On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
 amit.khande...@enterprisedb.com wrote:

 You mean the final changes in plperl_helpers.h would look like
 something like this right? :

  static inline char *
  utf_u2e(const char *utf8_str, size_t len)
  {
        char       *ret = (char *) pg_do_encoding_conversion((unsigned
 char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());

        if (ret == utf8_str)
 +       {
 +               if (GetDatabaseEncoding() == PG_UTF8 ||
 +                       GetDatabaseEncoding() == PG_SQL_ASCII)
 +               {
 +                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
 +               }
 +
                ret = pstrdup(ret);
 +       }
        return ret;
  }


 Yeah I am ok with that. It's just an additional check besides (ret ==
 utf8_str) to know if we really require validation.

Find it attached. [ Note I didn't put the check inside the if (ret ==
utf8_str) as it seemed a bit cleaner (indentation wise) to have it
outside ]
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 57,63  PSQLDIR = $(bindir)
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h
  
  plperl_opmask.h: plperl_opmask.pl
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
--- 57,63 
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h plperl_helpers.h
  
  plperl_opmask.h: plperl_opmask.pl
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
***
*** 639,641  CONTEXT:  PL/Perl anonymous code block
--- 639,651 
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
  ERROR:  Useless use of sort in scalar context at line 1.
  CONTEXT:  PL/Perl anonymous code block
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+   return abcd\0efg;
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();
+ ERROR:  invalid byte sequence for encoding UTF8: 0x00
+ CONTEXT:  PL/Perl function perl_zerob
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 7,16 
  static inline char *
  utf_u2e(const char *utf8_str, size_t len)
  {
! 	char	   *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
  
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
  	return ret;
  }
  
--- 7,27 
  static inline char *
  utf_u2e(const char *utf8_str, size_t len)
  {
! 	int 	enc = GetDatabaseEncoding();
! 
! 	char	   *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, enc);
! 
! 	/*
! 	* when we are a PG_UTF8 or SQL_ASCII database
! 	* pg_do_encoding_conversion() will not do any conversion or
! 	* verification. we need to do it manually instead.
! 	*/
! 	if (enc == PG_UTF8 || enc == PG_SQL_ASCII)
! 		pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
  
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
+ 
  	return ret;
  }
  
*** a/src/pl/plperl/sql/plperl.sql
--- b/src/pl/plperl/sql/plperl.sql
***
*** 415,417  DO $do$ use strict; my $name = foo; my $ref = $$name; $do$ LANGUAGE plperl;
--- 415,426 
  -- check that we can use warnings (in this case to turn a warn into an error)
  -- yields ERROR:  Useless use of sort in scalar context.
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
+ 
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+   return abcd\0efg;
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();

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

[HACKERS] Review: Non-inheritable check constraints

[Alex Hunsaker]

Hi! *Waves*

First off, it all seems to work as described:
- regressions pass
- domains work
- tried various inherit options (merging constraints, alter table no
inherit etc)
- pg_dump/restore

I didn't care for the changes to gram.y so I reworked it a bit so we
now pass is_only to AddRelationNewConstraint() (like we do with
is_local). Seemed simpler but maybe I missed something. Comments?

I also moved the is_only check in AtAddCheckConstraint() to before we
grab and loop through any children. Seemed a bit wasteful to loop
through the new constraints just to set a flag so that we could bail
out while looping through the children.

You also forgot to bump Natts_pg_constraint.

PFA the above changes as well as being rebased against master.


non_inh_check_v2.patch.gz
Description: GNU Zip compressed data

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Wed, Oct 5, 2011 at 08:18, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
 amit.khande...@enterprisedb.com wrote:
 I have no more issues with the patch.
 Thanks!

 I think this patch needs to be added to the open CommitFest, with
 links to the reviews, and marked Ready for Committer.

The open commitfest? Even if its an important bug fix that should be
backpatched?

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

[HACKERS] alter table only ... drop constraint broken in HEAD

[Alex Hunsaker]

tldr:

Seems to be broken by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665
:
commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665
Author: Robert Haas rh...@postgresql.org
Date:   Mon Jun 27 10:27:17 2011 -0400

Avoid having two copies of the HOT-chain search logic.

Full story:

While playing with the non inheritable constraints patch
(https://commitfest.postgresql.org/action/patch_view?id=611) I noticed
the following sequence was broken:
create table colors (c text check (not null));
create table colors_i () inherits (colors);
alter table only colors drop constraint colors_check;
ERROR:  relation 16412 has non-inherited constraint colors_check

Naturally I assumed it was the patches fault, but after further
digging turns out it was not. I bisected it down to the above commit
(for those that have not tried git bisect and git bisect run its great
for this kind of thing).

The basic problem seems to be after we update pg_constraint to
decrement inhcount for a child table we call
CommandCounterIncrement(); then we fetch the next row from
pg_constraint... which happens to be the row we just updated. So we
try to decrement it again, only now its at 0 which shouldn't happen so
we error out.

The simple fix seemed to be to move the CommandCounterIncrement()
outside of the while(... systable_getnext()) loop. Im not sure if
that's the right thing to-do or if there is some other bug here...
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 6734,6739  ATExecDropConstraint(Relation rel, const char *constrName,
--- 6734,6740 
  	{
  		Oid			childrelid = lfirst_oid(child);
  		Relation	childrel;
+ 		bool		updated = false;
  
  		/* find_inheritance_children already got lock */
  		childrel = heap_open(childrelid, NoLock);
***
*** 6790,6798  ATExecDropConstraint(Relation rel, const char *constrName,
  	con-coninhcount--;
  	simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
  	CatalogUpdateIndexes(conrel, copy_tuple);
! 
! 	/* Make update visible */
! 	CommandCounterIncrement();
  }
  			}
  			else
--- 6791,6798 
  	con-coninhcount--;
  	simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
  	CatalogUpdateIndexes(conrel, copy_tuple);
! 	/* need to call CommandCounterIncrement() */
! 	updated = true;
  }
  			}
  			else
***
*** 6807,6815  ATExecDropConstraint(Relation rel, const char *constrName,
  
  simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
  CatalogUpdateIndexes(conrel, copy_tuple);
! 
! /* Make update visible */
! CommandCounterIncrement();
  			}
  
  			heap_freetuple(copy_tuple);
--- 6807,6814 
  
  simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
  CatalogUpdateIndexes(conrel, copy_tuple);
! /* need to call CommandCounterIncrement() */
! updated = true;
  			}
  
  			heap_freetuple(copy_tuple);
***
*** 6824,6829  ATExecDropConstraint(Relation rel, const char *constrName,
--- 6823,6832 
  	   constrName,
  	   RelationGetRelationName(childrel;
  
+ 		/* Make update visible */
+ 		if (updated)
+ 			CommandCounterIncrement();
+ 
  		heap_close(childrel, NoLock);
  	}
  
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***
*** 2103,2105  ALTER TABLE tt7 NOT OF;
--- 2103,2112 
   x  | integer  | 
   y  | numeric(8,2) | 
  
+ -- make sure we can drop a constraint on the parent but it remains on the child
+ create table test_drop_constr_parent (c text check (c is not null));
+ create table test_drop_constr_child () inherits (test_drop_constr_parent);
+ alter table only test_drop_constr_parent drop constraint test_drop_constr_parent_c_check;
+ -- should fail
+ insert into test_drop_constr_child (c) values (NULL);
+ ERROR:  new row for relation test_drop_constr_child violates check constraint test_drop_constr_parent_c_check
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***
*** 1456,1458  CREATE TYPE tt_t1 AS (x int, y numeric(8,2));
--- 1456,1465 
  ALTER TABLE tt7 OF tt_t1;			-- reassign an already-typed table
  ALTER TABLE tt7 NOT OF;
  \d tt7
+ 
+ -- make sure we can drop a constraint on the parent but it remains on the child
+ create table test_drop_constr_parent (c text check (c is not null));
+ create table test_drop_constr_child () inherits (test_drop_constr_parent);
+ alter table only test_drop_constr_parent drop constraint test_drop_constr_parent_c_check;
+ -- should fail
+ insert into test_drop_constr_child (c) values (NULL);

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
amit.khande...@enterprisedb.com wrote:

 WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
 utf8_str, so pg_verify_mbstr_len() will not get called. That's the
 reason, pg_verify_mbstr_len() is under the ( ret == utf8_str )
 condition.

Consider a latin1 database where utf8_str was a string of ascii
characters. Then no conversion would take place and ret == utf8_str
but the string would be verified by pg_do_encdoing_conversion() and
verified again by your added check :-).

 It might be worth adding a regression test also...

 I could not find any basic pl/perl tests in the regression
 serial_schedule. I am not sure if we want to add just this scenario
 without any basic tests for pl/perl ?

I went ahead and added one in the attached based upon your example.

Look ok to you?

BTW thanks for the patch!

[ side note ]
I still think we should not be doing any conversion in the SQL_ASCII
case but this slimmed down patch should be less controversial.
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 57,63  PSQLDIR = $(bindir)
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h
  
  plperl_opmask.h: plperl_opmask.pl
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
--- 57,63 
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h plperl_helpers.h
  
  plperl_opmask.h: plperl_opmask.pl
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
***
*** 639,641  CONTEXT:  PL/Perl anonymous code block
--- 639,651 
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
  ERROR:  Useless use of sort in scalar context at line 1.
  CONTEXT:  PL/Perl anonymous code block
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+   return abcd\0efg;
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();
+ ERROR:  invalid byte sequence for encoding UTF8: 0x00
+ CONTEXT:  PL/Perl function perl_zerob
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 9,14  utf_u2e(const char *utf8_str, size_t len)
--- 9,22 
  {
  	char	   *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
  
+ 	/*
+ 	 * when src encoding == dest encoding (PG_UTF8 ==
+ 	 * GetDatabaseEncoding(), pg_do_encoding_conversion() is a noop and
+ 	 * does not verify the string so we need to do it manually
+ 	 */
+ 	if(GetDatabaseEncoding() == PG_UTF8)
+ 		pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+ 
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
  	return ret;
*** a/src/pl/plperl/sql/plperl.sql
--- b/src/pl/plperl/sql/plperl.sql
***
*** 415,417  DO $do$ use strict; my $name = foo; my $ref = $$name; $do$ LANGUAGE plperl;
--- 415,426 
  -- check that we can use warnings (in this case to turn a warn into an error)
  -- yields ERROR:  Useless use of sort in scalar context.
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
+ 
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+   return abcd\0efg;
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
amit.khande...@enterprisedb.com wrote:
 On 4 October 2011 14:04, Alex Hunsaker bada...@gmail.com wrote:
 On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
 amit.khande...@enterprisedb.com wrote:

 WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
 utf8_str, so pg_verify_mbstr_len() will not get called. [...]

 Consider a latin1 database where utf8_str was a string of ascii
 characters. [...]

 [Patch] Look ok to you?


 +       if(GetDatabaseEncoding() == PG_UTF8)
 +               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

 In your patch, the above will again skip mb-validation if the database
 encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
 the un-converted string even if *one* of the src and dest encodings is
 SQL_ASCII.

*scratches head* I thought the point of SQL_ASCII was no encoding
conversion was done and so there would be nothing to verify.

Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
NULL bytes in the string when we are a single byte encoding.

 I think :
        if (ret == utf8_str)
 +       {
 +               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
                ret = pstrdup(ret);
 +       }

 This (ret == utf8_str) condition would be a reliable way for knowing
 whether pg_do_encoding_conversion() has done the conversion at all.

Yes. However (and maybe im nitpicking here), I dont see any reason to
verify certain strings twice if we can avoid it.

What do you think about:
+/*
+* when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
+* will not do any conversion or verification. we need to do it
manually instead.
+*/
+   if( GetDatabaseEncoding() == PG_UTF8 ||
  GetDatabaseEncoding() == SQL_ASCII)
+   pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Mon, Oct 3, 2011 at 04:20, Amit Khandekar
amit.khande...@enterprisedb.com wrote:

 Is there a plan to commit this issue? I am still seeing this issue on
 PG 9.1 STABLE branch. Attached is a small patch that targets only the
 specific issue in the described testcase :

 create or replace function zerob() returns text as $$ return
 abcd\0efg; $$ language plperl;
 SELECT zerob();

 The patch does the perl data validation in the function utf_u2e() itself.

I think thats fine, but as coded it will verify the string twice in
the GetDatabaseEncoding() != PG_UTF8 case (once for
pg_do_encoding_conversion() and again with the added
pg_verify_mbstr_len), which seems a bit wasteful.

It might be worth adding a regression test also...

-- 
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] /proc/self/oom_adj is deprecated in newer Linux kernels

[Alex Hunsaker]

On Fri, Sep 16, 2011 at 10:37, Tom Lane t...@sss.pgh.pa.us wrote:
 Greg Stark st...@mit.edu writes:
 On Fri, Sep 16, 2011 at 3:57 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Does anyone want
 to argue for doing something more complicated, and if so what exactly?

 Well there's no harm trying to write to oom_score_adj and if that
 fails with EEXISTS trying to write to oom_adj.

Yeah, I don't really like the idea of a compile time option that is
kernel version dependent... But i don't feel too strongly about it
either (all my kernels are new enough that they support
oom_score_adj).

I'll also note that on my system we are in the good company of ssd and chromium:
sshd (978): /proc/978/oom_adj is deprecated, please use
/proc/978/oom_score_adj instead.
chromium-sandbo (1377): /proc/1375/oom_adj is deprecated, please use
/proc/1375/oom_score_adj instead.

[ It quite annoying that soon after we decided to stick
-DLINUX_OOM_ADJ in they changed it.  Whatever happened to a stable
userspace API :-( ]

-- 
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] Patch: Perl xsubpp

[Alex Hunsaker]

On Thu, Sep 15, 2011 at 10:44, David E. Wheeler da...@kineticode.com wrote:
 Hackers,

 Since installing Perl 5.14.1, I installed newer version of ExtUtils::ParseXS 
 from CPAN. I installed it with `make install UNINST=1`, which removes the 
 copy of xsubpp that ships with core Perl. This results in an error during 
 PostgreSQL `make`:

 make -C plperl install
 gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith 
 -Wdeclaration-after-statement -Wendif-labels -Wformat-security 
 -fno-strict-aliasing -fwrapv  -I. -I. -I../../../src/include 
 -I/usr/local/include/libxml2  -I/usr/local/include 
 -I/usr/local/lib/perl5/5.14.1/darwin-thread-multi-2level/CORE  -c -o plperl.o 
 plperl.c
 '/usr/local/bin/perl' /usr/local/lib/perl5/5.14.1/ExtUtils/xsubpp -typemap 
 /usr/local/lib/perl5/5.14.1/ExtUtils/typemap SPI.xs SPI.c
 Can't open perl script /usr/local/lib/perl5/5.14.1/ExtUtils/xsubpp: No such 
 file or directory

 I [asked][] Perl 5 Porters for the proper way to find xsubpp, and was 
 [told][] that it was probably best to look in @Config{qw(installsitebin 
 installvendorbin installbin)}.

Doesn't work for me :-( I have:
  'installbin' = '/usr/bin',
  'installsitebin' = '/usr/bin',
  'installvendorbin' = '/usr/bin',
  'installscript' = '/usr/bin/core_perl',
  'installprivlib' = '/usr/share/perl5/core_perl',
  'installsitescript' = '/usr/bin/site_perl',

$ ls /usr/bin/xsubpp
ls: cannot access /usr/bin/xsubpp: No such file or directory

$ ls /usr/bin/core_perl/xsubpp
/usr/bin/core_perl/xsubpp

The worst part is it tells me I need to configure with --with-perl.
Seems it complaining that it couldn't find xsubpp, I did configure
with perl!

Normally it uses the one in /usr/share/perl5/core_perl/ExtUtils/xsubpp.

Also it looks like it uses the wrong typemap file, still uses the one
from privlib.

So then I tried to install the newer ExtUtils::ParseXS to see where it
installed xsubpp for me. It reports:
...
Installing /usr/share/perl5/site_perl/ExtUtils/xsubpp

Installing /usr/bin/site_perl/xsubpp

Looking at its makefile looks like installs xsubpp into
installsitescript. Seems  install(site|vendor)bin is quite right :-(.

ExtUtils searches @INC, privlibexp maybe we should do that?

ExtUtils/MM_Unix.pm:

# line 3456
sub tool_xsubpp {

my @xsubpp_dirs = @INC;

# Make sure we pick up the new xsubpp if we're building perl.
unshift @xsubpp_dirs, $self-{PERL_LIB} if $self-{PERL_CORE};

foreach my $dir (@xsubpp_dirs) {
$xsdir = $self-catdir($dir, 'ExtUtils');
if( -r $self-catfile($xsdir, xsubpp) ) {
last;
}
}

-- 
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] Patch: Perl xsubpp

[Alex Hunsaker]

On Thu, Sep 15, 2011 at 15:53, David E. Wheeler da...@kineticode.com wrote:
 On Sep 15, 2011, at 4:41 PM, Alex Hunsaker wrote:

 ExtUtils searches @INC, privlibexp maybe we should do that?

 Yes, I just got an email from David Golden to that effect. So perhaps the 
 attached patch is better?

Close, seems I was wrong about the typemap ExtUtils::ParseXS does not
install a new one so we still need to point to the one in privlib.
Also xsubpp is not executable so the test should be -r or something.

Also don't think we should change the configure switch tests to test XSUBPPDIR.

Find those plus some minor typos fixed in the attached.
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 55,60  endif
--- 55,63 
  # where to find psql for running the tests
  PSQLDIR = $(bindir)
  
+ # where to find xsubpp for building XS.
+ XSUBPPDIR = $(shell $(PERL) -e 'use List::Util qw(first); print first { -r $$_/ExtUtils/xsubpp } @INC')
+ 
  include $(top_srcdir)/src/Makefile.shlib
  
  plperl.o: perlchunks.h plperl_opmask.h
***
*** 71,81  all: all-lib
  
  SPI.c: SPI.xs
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
! 	$(PERL) $(perl_privlibexp)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
  Util.c: Util.xs
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
! 	$(PERL) $(perl_privlibexp)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
  
  install: all install-lib install-data
--- 74,84 
  
  SPI.c: SPI.xs
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
! 	$(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
  Util.c: Util.xs
  	@if [ x$(perl_privlibexp) = x ]; then echo configure switch --with-perl was not specified.; exit 1; fi
! 	$(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $ $@
  
  
  install: all install-lib install-data
*** a/src/tools/msvc/Mkvcbuild.pm
--- b/src/tools/msvc/Mkvcbuild.pm
***
*** 13,18  use Project;
--- 13,20 
  use Solution;
  use Cwd;
  use File::Copy;
+ use Config;
+ use List::Util qw(first);
  
  use Exporter;
  our (@ISA, @EXPORT_OK);
***
*** 106,116  sub mkvcbuild
  (my $xsc = $xs) =~ s/\.xs/.c/;
  if (Solution::IsNewer($plperlsrc$xsc,$plperlsrc$xs))
  {
  print Building $plperlsrc$xsc...\n;
  system( $solution-{options}-{perl}
. '/bin/perl '
. $solution-{options}-{perl}
!   . '/lib/ExtUtils/xsubpp -typemap '
. $solution-{options}-{perl}
. '/lib/ExtUtils/typemap '
. $plperlsrc$xs 
--- 108,119 
  (my $xsc = $xs) =~ s/\.xs/.c/;
  if (Solution::IsNewer($plperlsrc$xsc,$plperlsrc$xs))
  {
+ my $xsubppdir = first { -e $_\\ExtUtils\\xsubpp.BAT } @INC;
  print Building $plperlsrc$xsc...\n;
  system( $solution-{options}-{perl}
. '/bin/perl '
. $solution-{options}-{perl}
!   . $xsubppdir/ExtUtils/xsubpp -typemap 
. $solution-{options}-{perl}
. '/lib/ExtUtils/typemap '
. $plperlsrc$xs 

-- 
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] PL/Perl Returned Array

[Alex Hunsaker]

On Wed, Aug 17, 2011 at 10:06, Andrew Dunstan and...@dunslane.net wrote:


 On 08/12/2011 09:17 PM, Alex Hunsaker wrote:

 [empty arrays returned are not handled correctly]


 Anyway, the attached patch fixes it for me. That is when we don't have
 an array state, just return an empty array.  (Also adds some
 additional comments)

 Applied, thanks.

Thanks for picking this up.

-- 
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] PL/Perl Returned Array

[Alex Hunsaker]

On Fri, Aug 12, 2011 at 18:00, David E. Wheeler da...@kineticode.com wrote:
 Hackers,

 Given this script on 9.1beta3:

    BEGIN;

    CREATE EXTENSION plperl;

    CREATE OR REPLACE FUNCTION wtf(
    ) RETURNS TEXT[] LANGUAGE plperl AS $$ return []; $$;

    SELECT wtf() = '{}'::TEXT[];

 Why is that? If I save the values to TEXT[] columns, they are still not the 
 same. But if I pg_dump them and then load them up again, they *are* the same. 
 The dump looks like this:

Eek.

 Is PL/Perl doing something funky under the hood to array values it returns on 
 9.1?

Yeah, its not handling empty arrays very well (its calling
accumArrayResult which increments the number of elements even when we
are a zero length array).

This was rewritten in 9.1 as part of the array overhaul. Previously
returned arrays were converted into a string with the perl sub
encode_array_literal(), potgres would then convert that string into
the appropriate data type. Not only was that a tad inefficient, but it
failed to handle composite types nicely. In 9.1 you can pass in arrays
with composite types and the like, we figured it would be awfully nice
if you could return them as well :-)

Anyway, the attached patch fixes it for me. That is when we don't have
an array state, just return an empty array.  (Also adds some
additional comments)

I thought about doing this right after we set dims[0] in
plperl_array_to_datum(), but that would fail to handle nested empty
multidim arrays like [[]]. As long as you can do that via
ARRAY[ARRAY[]]... I figure plperl should support it to.

Thanks for the report!
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 1078,1091  _array_to_datum(AV *av, int *ndims, int *dims, int cur_depth,
  	int			i = 0;
  	int			len = av_len(av) + 1;
  
- 	if (len == 0)
- 		astate = accumArrayResult(astate, (Datum) 0, true, atypid, NULL);
- 
  	for (i = 0; i  len; i++)
  	{
  		SV		  **svp = av_fetch(av, i, FALSE);
  		SV		   *sav = svp ? get_perl_array_ref(*svp) : NULL;
  
  		if (sav)
  		{
  			AV		   *nav = (AV *) SvRV(sav);
--- 1078,1092 
  	int			i = 0;
  	int			len = av_len(av) + 1;
  
  	for (i = 0; i  len; i++)
  	{
+ 		/* fetch the array element */
  		SV		  **svp = av_fetch(av, i, FALSE);
+ 
+ 		/* see if this element is an array, if so get that */
  		SV		   *sav = svp ? get_perl_array_ref(*svp) : NULL;
  
+ 		/* multi-dimensional array? */
  		if (sav)
  		{
  			AV		   *nav = (AV *) SvRV(sav);
***
*** 1149,1154  plperl_array_to_datum(SV *src, Oid typid)
--- 1150,1158 
  	astate = _array_to_datum((AV *) SvRV(src), ndims, dims, 1, astate, typid,
  			 atypid);
  
+ 	if (!astate)
+ 		return PointerGetDatum(construct_empty_array(atypid));
+ 
  	for (i = 0; i  ndims; i++)
  		lbs[i] = 1;
  

-- 
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] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[Alex Hunsaker]

On Sun, Aug 7, 2011 at 17:06, Tim Bunce tim.bu...@pobox.com wrote:

 On Sat, Aug 06, 2011 at 12:37:28PM -0600, Alex Hunsaker wrote:
 ...
 Find attached a version that does the equivalent of local %SIG for
 each pl/perl(u) call.

 +     gv = gv_fetchpv(SIG, 0, SVt_PVHV);
 +     save_hash(gv);                  /* local %SIG */

 ... [ local %SIG dosn't work ] The %SIG does become empty but the OS
 level handlers, even those installed by perl, *aren't changed*:

Looks like I trusted in $SIG{'ALRM'} being undef after it had been set
in a different scope too much :-( Thanks for pointing this out.

 That sure seems like a bug (I'll check with the perl5-porters list).

Well even if it was deemed a bug, it dont do us any good.

 Localizing an individual element of %SIG works fine.
 In C that's something like this (untested):

    hv = gv_fetchpv(SIG, 0, SVt_PVHV);
    keysv = ...SV containing ALRM...
    he = hv_fetch_ent(hv, keysv, 0, 0);
    if (he) {  /* arrange to restore existing elem */
        save_helem_flags(hv, keysv, HeVAL(he), SAVEf_SETMAGIC);
    }
    else {     /* arrange to delete a new elem */
        SAVEHDELETE(hv, keysv);
    }

I played with this a bit... and found yes, it locals them but no it
does not fix the reported problem. After playing with things a bit
more I found even local $SIG{'ALRM'} = .,..; alarm(1); still results
in postgres crashing. To wit, local does squat. AFAICT it just resets
the signal handler back to the default with SIG_DFL. (Which in
hindsight I don't know what else I expected it to-do...)

So I think for this to be robust we would have to detect what signals
they set and then reset those back to what postgres wants. Doable, but
is it worth it? Anyone else have any bright ideas?

Find below my test case and attached a patch that locals individual
%SIG elements the way mentioned above.

= set statement_timeout to '5s';
SET

= create or replace function test_alarm() returns void as $$ local
$SIG{'ALRM'} = sub { warn alarm; }; alarm(1); sleep 2; $$ language
plperlu;
CREATE FUNCTION

= select test_alarm();
WARNING:  alarm at line 1.
CONTEXT:  PL/Perl function test_alarm
 test_alarm


(1 row)

= select pg_sleep(6);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Server Log:
WARNING:  alarm at line 1.
CONTEXT:  PL/Perl function test_alarm
LOG:  server process (PID 32659) was terminated by signal 14: Alarm clock
LOG:  terminating any other active server processes
WARNING:  terminating connection because of crash of another server process
DETAIL:  The postmaster has commanded this server process to roll back
the current transaction and exit, because another server process
exited abnormally and possibly corrupted shared memory.
HINT:  In a moment you should be able to reconnect to the database and
repeat your command.
FATAL:  the database system is in recovery mode
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
***
*** 639,641  CONTEXT:  PL/Perl anonymous code block
--- 639,643 
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
  ERROR:  Useless use of sort in scalar context at line 1.
  CONTEXT:  PL/Perl anonymous code block
+ DO $do$ die SIG ALRM is set: $SIG{'ALRM'} if($SIG{'ALRM'}); $SIG{'ALRM'} = sub { print alarm!\n}; $do$ LANGUAGE plperl;
+ DO $do$ die SIG ALRM is set: $SIG{'ALRM'} if($SIG{'ALRM'}); $do$ LANGUAGE plperl;
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 268,273  static void plperl_inline_callback(void *arg);
--- 268,275 
  static char *strip_trailing_ws(const char *msg);
  static OP  *pp_require_safe(pTHX);
  static void activate_interpreter(plperl_interp_desc *interp_desc);
+ static void local_sigs(void);
+ static void local_sig(HV *hv, SV *tmpsv, const char *signame);
  
  #ifdef WIN32
  static char *setlocale_perl(int category, char *locale);
***
*** 1901,1906  plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
--- 1903,1910 
  	ENTER;
  	SAVETMPS;
  
+ 	local_sigs();
+ 
  	PUSHMARK(SP);
  	EXTEND(sp, desc-nargs);
  
***
*** 1968,1973  plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
--- 1972,2028 
  	return retval;
  }
  
+ /*
+  * local all of our sig handlers some modules like LWP like to set an alarm sig
+  * handler for things like network timeouts, this can cause bad stuff to happen
+  * (not to mention what happens if someone sets USR1)
+  *
+  * for now we just local() them all so they should get reset back to what
+  * postgres expects when their pl function is done
+  */
+ static void
+ local_sigs(void)
+ {
+ 	HV	*hv;
+ 	SV	*sv = newSV(9);
+ 	int i;
+ 
+ 	hv = get_hv(SIG, 0);
+ 	if (!hv)
+ 		elog(ERROR, couldn't fetch %%SIG);
+ 
+ 	/*
+ 	 * char

Re: [HACKERS] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[Alex Hunsaker]

On Fri, Aug 5, 2011 at 08:53, Andrew Dunstan and...@dunslane.net wrote:


 On 08/04/2011 11:23 PM, Alex Hunsaker wrote:

 [ ... don't let people set signal handlers postgres sets ]

 This whole thing is a massive over-reaction to a problem we almost certainly
 know how to fix fairly simply and relatively painlessly, and attempts
 (unsuccessfully, at least insofar as comprehensiveness is concerned) to fix
 a problem nobody's actually reported having AFAIK.

*shrug* OK.

Find attached a version that does the equivalent of local %SIG for
each pl/perl(u) call. I was only able to test on 5.14.1, but I looked
at 5.8.9 to make sure it looks like it works.

Its a tad slower (something like 0.00037ms per call), but uhh thats
quite acceptable IMHO (best of 5 runs):

= create or replace function simple() returns void as $$ $$ language plperl;
CREATE FUNCTION

-- pre patch
= select count(simple()) from generate_series(1, 1000);
Time: 10219.149 ms

-- patched
= select count(simple()) from generate_series(1, 1000);
Time: 13924.025 ms

Thoughts?
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
***
*** 639,641  CONTEXT:  PL/Perl anonymous code block
--- 639,643 
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
  ERROR:  Useless use of sort in scalar context at line 1.
  CONTEXT:  PL/Perl anonymous code block
+ DO $do$ die $SIG{'ALARM'} set! if($SIG{'ALARM'}); $SIG{'ALARM'} = sub { print alarm!\n}; $do$ LANGUAGE plperl;
+ DO $do$ die $SIG{'ALARM'} set! if($SIG{'ALARM'}); $do$ LANGUAGE plperl;
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 1895,1906  plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
--- 1895,1912 
  {
  	dSP;
  	SV		   *retval;
+ 	GV		   *gv;
  	int			i;
  	int			count;
  
  	ENTER;
  	SAVETMPS;
  
+ 	gv = gv_fetchpv(SIG, 0, SVt_PVHV);
+ 	if (!gv)
+ 		elog(ERROR, couldn't fetch %%SIG);
+ 	save_hash(gv);			/* local %SIG */
+ 
  	PUSHMARK(SP);
  	EXTEND(sp, desc-nargs);
  
***
*** 1976,1981  plperl_call_perl_trigger_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo,
--- 1982,1988 
  	dSP;
  	SV		   *retval,
  			   *TDsv;
+ 	GV		   *gv;
  	int			i,
  count;
  	Trigger*tg_trigger = ((TriggerData *) fcinfo-context)-tg_trigger;
***
*** 1986,1995  plperl_call_perl_trigger_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo,
  	TDsv = get_sv(_TD, 0);
  	if (!TDsv)
  		elog(ERROR, couldn't fetch $_TD);
- 
  	save_item(TDsv);			/* local $_TD */
  	sv_setsv(TDsv, td);
  
  	PUSHMARK(sp);
  	EXTEND(sp, tg_trigger-tgnargs);
  
--- 1993,2006 
  	TDsv = get_sv(_TD, 0);
  	if (!TDsv)
  		elog(ERROR, couldn't fetch $_TD);
  	save_item(TDsv);			/* local $_TD */
  	sv_setsv(TDsv, td);
  
+ 	gv = gv_fetchpv(SIG, 0, SVt_PVHV);
+ 	if (!gv)
+ 		elog(ERROR, couldn't fetch %%SIG);
+ 	save_hash(gv);			/* local %SIG */
+ 
  	PUSHMARK(sp);
  	EXTEND(sp, tg_trigger-tgnargs);
  
*** a/src/pl/plperl/sql/plperl.sql
--- b/src/pl/plperl/sql/plperl.sql
***
*** 415,417  DO $do$ use strict; my $name = foo; my $ref = $$name; $do$ LANGUAGE plperl;
--- 415,420 
  -- check that we can use warnings (in this case to turn a warn into an error)
  -- yields ERROR:  Useless use of sort in scalar context.
  DO $do$ use warnings FATAL = qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
+ 
+ DO $do$ die $SIG{'ALARM'} set! if($SIG{'ALARM'}); $SIG{'ALARM'} = sub { print alarm!\n}; $do$ LANGUAGE plperl;
+ DO $do$ die $SIG{'ALARM'} set! if($SIG{'ALARM'}); $do$ LANGUAGE plperl;

-- 
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] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[Alex Hunsaker]

On Thu, Aug 4, 2011 at 09:11, Andrew Dunstan and...@dunslane.net wrote:

 What *I'd* like is a way to prevent libperl from touching the host
 application's signal handlers at all.  Sadly, Perl does not actually
 think of itself as an embedded library, and therefore thinks it owns all
 resources of the process and can diddle them without anybody's
 permission.



 I'm not sure how perl (or any loadable library) could restrict that in
 loaded C code, which many perl modules call directly or indirectly. It's as
 open as, say, a loadable C function is in Postgres ;-) You have a gun. It's
 loaded. If you point it at your foot and pull the trigger don't blame us. I
 think you just need to be very careful about what you do with plperlu. Don't
 be surprised if things break.

Well we can't prevent perl XS (aka C) from messing with signals (and
other modules like POSIX that expose things like sigprocmask,
siglongjump etc.) , but we could prevent plperl(u) from playing with
signals on the perl level ala %SIG.

[ IIRC I proposed doing something about this when we were talking
about the whole Safe mess, but I think there was too much other
discussion going on at the time :-) ]

Mainly the options im thinking about are:
1) if anyone touches %SIG die
2) turn %SIG into a regular hash so people can set/play with %SIG, but
it has no real effect.
3) local %SIG before we call their trigger function. This lets signals
still work while in trigger scope (like we do for %_TD)
4) if we can't get any of the above to work we can save each %SIG
handler before and restore them after each trigger call. (mod_perl
does something similar so Im fairly certain we should be able to get
that to work)

Thoughts?

-- 
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] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[Alex Hunsaker]

On Thu, Aug 4, 2011 at 16:34, David E. Wheeler da...@kineticode.com wrote:
 On Aug 4, 2011, at 3:09 PM, Alex Hunsaker wrote:

 Mainly the options im thinking about are:
 1) if anyone touches %SIG die
 2) turn %SIG into a regular hash so people can set/play with %SIG, but
 it has no real effect.

 These would disable stuff like $SIG{__WARN__} and $SIG{__DIE__}, which would 
 be an unfortunate side-effect.

Yeah,  good point.

 3) local %SIG before we call their trigger function. This lets signals
 still work while in trigger scope (like we do for %_TD)

 +1

That seems to be what most people up-thread thought as well. I dont
see it being too expensive. Ill see if I can whip something up today.

-- 
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] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[Alex Hunsaker]

On Thu, Aug 4, 2011 at 17:52, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 On Thu, Aug 4, 2011 at 16:34, David E. Wheeler da...@kineticode.com wrote:
 On Aug 4, 2011, at 3:09 PM, Alex Hunsaker wrote:
 3) local %SIG before we call their trigger function. This lets signals
 still work while in trigger scope (like we do for %_TD)

 +1

 That seems to be what most people up-thread thought as well. I dont
 see it being too expensive. Ill see if I can whip something up today.

 The scenario I was imagining was:
 [ $SIG{ALRM} + statement timeout-- what happens?]
 
 Even if you don't think statement_timeout is a particularly critical
 piece of functionality, similar interference with the delivery of, say,
 SIGUSR1 would be catastrophic.

Yipes, I see your point.

 How do you propose to prevent this sort of problem?

Well, I think that makes it unworkable.

So back to #1 or #2.

For plperlu sounds like we are going to need to disallow setting _any_
signals (minus __DIE__ and __WARN__). I should be able to make it so
when you try it gives you a warning something along the lines of
plperl can't set signal handlers, ignoring

For plperl I think we should probably do the same. It seems like
Andrew might disagree though? Anyone else want to chime in on if
plperl lets you muck with signal handlers?

Im not entirely sure how much of this is workable, I still need to go
through perl's guts and see. At the very worst I think we can mark
each signal handler that is exposed in %SIG readonly (which would mean
we would  die instead of warning), but I think I can make the warning
variant workable as well.

I also have not dug deep enough to know how to handle __WARN__ and
__DIE__ (and exactly what limitations allowing those will impose). I
still have some work at $day_job before I can really dig into this.

-- 
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] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[Alex Hunsaker]

On Thu, Aug 4, 2011 at 19:40, Andrew Dunstan and...@dunslane.net wrote:

 Let's slow down a bit. Nobody that we know of has encountered the problem
 Tom's referring to, over all the years plperlu has been available. The
 changes you're proposing have the potential to downgrade the usefulness of
 plperlu considerably without fixing anything that's known to be an actual
 problem. Instead of fixing a problem caused by using LWP you could well make
 LWP totally unusable from plperlu.

Well, im not sure about it making LWP totally unusable... You could
always use statement_timeout if you were worried about it blocking
^_^.

 And it still won't do a thing about signal handlers installed by C code.

 And plperlu would be the tip of the iceberg. What about all the other PLs,
 not to mention non-PL loadable modules?

Maybe the answer is to re-issue the appropriate pqsignals() instead of
doing the perl variant?

For PL/Perl(u) we could still disallow any signals the postmaster
uses, from my quick look that would be: HUP, INT, TERM, QUIT, ALRM,
PIPE, USR1, USR2, FPE. All we would need to do is restore ALRM.

Or am I too paranoid about someone shooting themselves in the foot via
USR1? (BTW you can set signals in plperl, but you can't call alarm()
or kill())

-- 
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] Check constraints on partition parents only?

[Alex Hunsaker]

On Wed, Jul 27, 2011 at 14:08, Tom Lane t...@sss.pgh.pa.us wrote:

 Yeah.  If we're going to allow this then we should just have a concept
 of a non-inherited constraint, full stop.  This might just be a matter
 of removing the error thrown in ATAddCheckConstraint, but I'd be worried
 about whether pg_dump will handle the case correctly, what happens when
 a new child is added later, etc etc.

[ For those who missed it ]
pg_dump getting things wrong was a big reason to disallow
ONLYconstraints. That is pg_dump did not treat ONLY constraints
correctly, it always tried to stick them on the parent table:
http://archives.postgresql.org/pgsql-bugs/2007-04/msg00026.php

I for example had some backups that had to be manually fixed (by
removing constraints) to get them to import. I would wager the
mentioned clients that have been doing this have broken backups as
well :-(

Now that we have coninhcnt, conislocal etc... we can probably support
ONLY. But I agree with Robert it's probably a bit more than an
afternoon to crank out :-)

-- 
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] Arrays of Records in PL/Perl

[Alex Hunsaker]

On Tue, Jul 12, 2011 at 12:45, David E. Wheeler da...@kineticode.com wrote:
 Hackers,

 That is, if a record is passed to a PL/Perl function, it's correctly 
 converted into a hash. If, however, an array of records are passed, the 
 record are stringified, rather than turned into hashes. This seems 
 inconsistent. Bug?

All Arrays in 9.0 and lower are strings, regardless of if they are
comprised of composite types. Its not so much a bug as a limitation.
Alexey Klyukin fixed this for 9.1 :-)

[ In 9.1 we could not make them straight perl arrayrefs as we needed
to keep the string representation for backwards compatibility. What we
did in 9.1 is overload the arrayref and string operations on blessed
object so you can treat it as either. ]

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

[HACKERS] csvlog_fields review

[Alex Hunsaker]

It bit rotted a bit find a new version attached that includes the
following fixes:

- show_session_authorization() no longer exists, instead access the
session_authorization_guc directly (like we do for show_role in
commands/variable.c). I find it quite ugly tho...
- it changed %u to %U and %U to be %u... flipped it back so %u remains unchanged
- num_fields in write_csvlog  was unused, removed it
- new_csv_fields would leak in an error path of assign_csvlog_fields
(which probably matters as we are in TopMemoryContext)

All of Itagaki-san's comments still stand:

 * csvlog_fields and csvlog_header won't work with non-default log_filename
 when it doesn't contain seconds in the format. They expect they can always
 open empty log files.

I think at the very least we should document this? Or maybe only write
out the header when its a zero length file?

 * The long default value for csvlog_fields leads long text line in
 postgresql.conf, SHOW ALL, pg_settings view, but there were no better
 alternative solutions in the past discussion.

I think it might be worth revisiting using the %X syntax
log_line_prefix uses instead of inventing our own long form names.

 * csvlog_fields is marked as PGC_POSTMASTER. It can protect mixed formats
 in a csv file on default log_filename, but other similar GUC variables
 are usually marked AS PGC_SIGHUP.

I don't really see this as a problem...

As it stands I think we still need to address the first two questions
before its ready for a -commiter.


csvlog_fields_ah.patch.gz
Description: GNU Zip compressed data

-- 
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] add support for logging current role (what to review?)

[Alex Hunsaker]

On Thu, Jun 30, 2011 at 16:35, Stephen Frost sfr...@snowman.net wrote:

 I do want to rework the logging infrastructure (as discussed in the dev
 meeting), but I see that whole thing as rather orthogonal to this
 change.

*Shrug* Fine by me, im not going to argue that you should or shouldn't
rework the logging infrastructure. There seem to be people on both
sides of the fence... Including me. Im of the opinion that for *this*
field we should just add it, I think it stands on its own legs. For
some of the other columns (duration, pl line # etc)... well It might
make more sense to have a knob.

So uhh.. Im still confused at exactly what patch I should be looking
at because of my opinion that we should just add this field.

Stephen (others?), whats your preference? Maybe this should be two
patches (one that adds the field, another that adds a header)?

-- 
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] add support for logging current role (what to review?)

[Alex Hunsaker]

On Tue, Jun 28, 2011 at 09:15, Robert Haas robertmh...@gmail.com wrote:
 Anyway, if we are going to insist on rewriting substantial chunks of
 the logging infrastructure before doing this, we at least need to
 reach some agreement on what would be an acceptable outcome - and then
 let Stephen code that up as a separate patch, submit it, get it
 committed, and then do this on top of that.  Or we can just decide
 that adding one relatively minor field to the text format output is
 not worth knocking over the applecart for.

Well, one of the reasons I picked up this patch is its a feature *I*
want. I remember being quite surprised at the role csv logging reports
when i switched to it. I was logging everything so I have on occasion
had to find a SET ROLE and follow the session... its quite annoying
:-)

I think if Stephen was proposing 10 fields, or if there was a list of
fields we were planning on adding in the next release or 3, it might
be worth re-factoring. I know of no such list, and I think this field
useful/important enough that people who are using csv logging would
want it anyway. +1 on just tacking on the field and causing a flag day
for csv users.

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

[HACKERS] add support for logging current role (what to review?)

[Alex Hunsaker]

Ive been holding off because its marked as Waiting on Author, am now
thinking thats a mistake. =)

It links to this patch:
http://archives.postgresql.org/message-id/20110215135131.gx4...@tamriel.snowman.net

Which is older than the latest patch in that thread posted by Robert:
http://archives.postgresql.org/message-id/AANLkTikMadttguOWTkKLtgfe90kxR=u9njk9zebrw...@mail.gmail.com

(There are also various other patches and versions in that thread...)

The main difference between the first and the last patch is the first
one has support for changing what csv columns we output, while the
latter just tacks on an additional column.

The thread was very long and left me a bit confused as to what I
should actually be looking at. Or perhaps thats the point-- we need to
decide if a csvlog_fields GUC is worth it.

-- 
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] Creating new remote branch in git?

[Alex Hunsaker]

On Fri, Jun 10, 2011 at 00:53, Greg Smith g...@2ndquadrant.com wrote:
 On 06/10/2011 12:19 AM, Alex Hunsaker wrote:

 It looks like if you push the remote branch first everything should work
 nicely:
 git checkout master
 git push origin origin:refs/heads/REL9_1_STABLE
 git fetch # fetch the new branch
 git checkout REL9_1_STABLE

 This is basically the state of the art right now for the most frequently
 deployed versions of git.  I don't think checking out master first is
 necessary though.

I assume it will use the current HEAD as the branch point which is why
I checked out master :)

 Potentially useful automation/trivia for alternate approaches includes:

 1) Write a little script to do this messy chore, so you don't have to
 remember this weird create a new branch using a full refspec syntax.
  There is an example named git-create-branch along with a short tutorial on
 this subject at
 http://www.zorched.net/2008/04/14/start-a-new-branch-on-your-remote-git-repository/

 2) Use git_remote_branch https://github.com/webmat/git_remote_branch which
 is the swiss army knife of remote branch hackery automation.

 3) Rather than manually hack the config files, use git config to do it.
  Not sure if this is completely workable, but something like this might
 connect the newly created branch to your local one after pushing it out,
 without actually opening the config with an editor:

 git config branch.REL9_1_STABLE.remote origin
 git config branch.REL9_1_STABLE.merge refs/heads/REL9_1_STABLE

 4) Use a system with git=1.7.0, which adds:

 git branch --set-upstream REL9_1_STABLE origin/REL9_1_STABLE

But wait! there's more!

5) delete your local branch and recreate it after you push the branch out
git branch REL9_1_STABLE
git push origin REL9_1_STABLE
# -f is short hand, you could git branch -d REL9_1_STABLE and re-make it
git branch -f REL9_1_STABLE origin/REL9_1_STABLE

6) use push -u


Its git so there are probably another half dozen ways to do this...
What Im curious about is what is the 'proper' way? Or is that a
nonsensical question when talking about git :-P

-- 
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] gcc 4.6 and hot standby

[Alex Hunsaker]

On Fri, Jun 10, 2011 at 12:38, Tom Lane t...@sss.pgh.pa.us wrote:

 I've been able to reproduce this on released Fedora 15, and sure enough
 it is a compiler bug.  The problem comes from these fragments of
 ReadRecord():
 [ ... ]

Whoa, awesome. I spent a few more hours comparing the assembly-- then
I tried compiling a subset of xlog.c with some educated guesses as to
what function might be getting mis-compiled. Obviously my guesses were
not educated enough! :-)

 I've filed a bug report for this:
 https://bugzilla.redhat.com/show_bug.cgi?id=712480
 but I wouldn't care to hold my breath while waiting for a fix to appear
 upstream, let alone propagate to all the distros already using 4.6.0.

I wouldn't hold my breath either.

 I think we need a workaround.

 The obvious question to ask here is why are we updating
 tmpRecPtr.xrecoff and not RecPtr-xrecoff?  The latter choice would
 be a lot more readable, since the immediately surrounding code doesn't
 refer to tmpRecPtr.  I think the idea is to guarantee that the caller's
 referenced record pointer (if any) isn't modified, but if RecPtr is not
 pointing at tmpRecPtr here, we have got big problems anyway.

Hrm, Couldn't we change all the references to tmpRecPtr to use RecPtr
instead? (Except of course where we assign RecPtr = tmpRecPtr); I
think that would make the code look a lot less confused. Something
like the attached?

FYI Im happy to test whatever you fix you propose for a few days on
this machine. It gets a fair amount of traffic... hopefully enough to
exercise some possible corner cases.
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 3681,3694  ReadRecord(XLogRecPtr *RecPtr, int emode, bool fetching_ckpt)
  		 */
  		if (XLOG_BLCKSZ - (RecPtr-xrecoff % XLOG_BLCKSZ)  SizeOfXLogRecord)
  		{
! 			NextLogPage(tmpRecPtr);
  			/* We will account for page header size below */
  		}
  
! 		if (tmpRecPtr.xrecoff = XLogFileSize)
  		{
! 			(tmpRecPtr.xlogid)++;
! 			tmpRecPtr.xrecoff = 0;
  		}
  	}
  	else
--- 3681,3694 
  		 */
  		if (XLOG_BLCKSZ - (RecPtr-xrecoff % XLOG_BLCKSZ)  SizeOfXLogRecord)
  		{
! 			NextLogPage(RecPtr);
  			/* We will account for page header size below */
  		}
  
! 		if (RecPtr-xrecoff = XLogFileSize)
  		{
! 			(RecPtr-xlogid)++;
! 			RecPtr-xrecoff = 0;
  		}
  	}
  	else
***
*** 3725,3731  retry:
  		 * XRecOffIsValid eliminated the zero-page-offset case otherwise. Need
  		 * to skip over the new page's header.
  		 */
! 		tmpRecPtr.xrecoff += pageHeaderSize;
  		targetRecOff = pageHeaderSize;
  	}
  	else if (targetRecOff  pageHeaderSize)
--- 3725,3731 
  		 * XRecOffIsValid eliminated the zero-page-offset case otherwise. Need
  		 * to skip over the new page's header.
  		 */
! 		RecPtr-xrecoff += pageHeaderSize;
  		targetRecOff = pageHeaderSize;
  	}
  	else if (targetRecOff  pageHeaderSize)
*** a/src/include/access/xlog_internal.h
--- b/src/include/access/xlog_internal.h
***
*** 154,166  typedef XLogLongPageHeaderData *XLogLongPageHeader;
  /* Align a record pointer to next page */
  #define NextLogPage(recptr) \
  	do {	\
! 		if (recptr.xrecoff % XLOG_BLCKSZ != 0)	\
! 			recptr.xrecoff +=	\
! (XLOG_BLCKSZ - recptr.xrecoff % XLOG_BLCKSZ);	\
! 		if (recptr.xrecoff = XLogFileSize) \
  		{	\
! 			(recptr.xlogid)++;	\
! 			recptr.xrecoff = 0; \
  		}	\
  	} while (0)
  
--- 154,166 
  /* Align a record pointer to next page */
  #define NextLogPage(recptr) \
  	do {	\
! 		if (recptr-xrecoff % XLOG_BLCKSZ != 0)	\
! 			recptr-xrecoff +=	\
! (XLOG_BLCKSZ - recptr-xrecoff % XLOG_BLCKSZ);	\
! 		if (recptr-xrecoff = XLogFileSize) \
  		{	\
! 			(recptr-xlogid)++;	\
! 			recptr-xrecoff = 0; \
  		}	\
  	} while (0)
  

-- 
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] gcc 4.6 and hot standby

[Alex Hunsaker]

On Fri, Jun 10, 2011 at 14:24, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:

 Hrm, Couldn't we change all the references to tmpRecPtr to use RecPtr
 instead? (Except of course where we assign RecPtr = tmpRecPtr); I
 think that would make the code look a lot less confused. Something
 like the attached?

 Yeah, we could do that too; slightly modified version of your change
 included in the attached.

A cassert enabled build seems to be working, ill leave it running over
the weekend...

-- 
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] Creating new remote branch in git?

[Alex Hunsaker]

On Thu, Jun 9, 2011 at 21:05, Tom Lane t...@sss.pgh.pa.us wrote:
 In the next couple of days it's going to be time to branch off
 REL9_1_STABLE from master, and I realized that I am pretty foggy on
 how to do that in git.  I suppose it's some variant of

 git checkout master             # if not there already
 git branch REL9_1_STABLE
 git push origin REL9_1_STABLE

 but it's not clear to me whether any options are needed to ensure that
 the right branch tracking behavior gets set up.

That looks right, and yeah that won't setup that branch to track
upstream for you. However, it should work for anyone that gets that
branch as part of a fetch/pull. ( that is it will work like any other
remote branch )

Ive always found it easy enought to edit .git/config.  If you add an
entry that looks like any of the other RELX_X_STABLE branches it
should work fine. Something along the lines of:
[branch REL9_1_STABLE]
remote = origin
merge = refs/heads/REL9_1_STABLE

 Should this process get documented at
 http://wiki.postgresql.org/wiki/Committing_with_Git

+1 [ Im curious if any git experts chime in with a cleaner way than
mucking with the config file. ]

-- 
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] Creating new remote branch in git?

[Alex Hunsaker]

On Thu, Jun 9, 2011 at 22:02, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 On Thu, Jun 9, 2011 at 21:05, Tom Lane t...@sss.pgh.pa.us wrote:
 In the next couple of days it's going to be time to branch off
 REL9_1_STABLE from master, and I realized that I am pretty foggy on
 how to do that in git.  I suppose it's some variant of

 git checkout master             # if not there already
 git branch REL9_1_STABLE
 git push origin REL9_1_STABLE

 but it's not clear to me whether any options are needed to ensure that
 the right branch tracking behavior gets set up.

 That looks right, and yeah that won't setup that branch to track
 upstream for you. However, it should work for anyone that gets that
 branch as part of a fetch/pull. ( that is it will work like any other
 remote branch )

 So creating the branch trashes my own repo?  Surely there's a better
 way.

I dunno where you got trashes from. I must have worded that poorly. It
won't break anything, it just won't track origin/upstream.

It looks like if you push the remote branch first everything should work nicely:
git checkout master
git push origin origin:refs/heads/REL9_1_STABLE
git fetch # fetch the new branch
git checkout REL9_1_STABLE

-- 
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] [Pgbuildfarm-members] CREATE FUNCTION hang on test machine polecat on HEAD

[Alex Hunsaker]

On Tue, Jun 7, 2011 at 12:42, Alex Hunsaker bada...@gmail.com wrote:
 On Tue, Jun 7, 2011 at 12:22, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 Im looking at the raw perl 5.10.0 source... I wonder if apple is
 shipping a modified version?

 You could find out by digging around at
 http://www.opensource.apple.com/
 polecat appears to be running OSX 10.6.7, so this is what you want:
 http://www.opensource.apple.com/tarballs/perl/perl-63.tar.gz

 Thanks!

Hrm they don't seem to touch util.c where PL_get_hash_seed lives at
all :-(. I also looked at 5.8.9 and Perl_get_hash_seed looks the same
as in 5.10.0.

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

[HACKERS] gcc 4.6 and hot standby

[Alex Hunsaker]

So I've been delaying moving some production boxes over to 9.0.4 from
9.0.2 because hot standby fails with:
(this is on the hot standby machine that connects to the master)

2011-06-08 11:40:48 MDT [6072]: [2-1] user= LOG:  entering standby mode
2011-06-08 11:40:48 MDT [6072]: [3-1] user= DEBUG:  checkpoint record
is at 86/E5D725F0
2011-06-08 11:40:48 MDT [6072]: [4-1] user= DEBUG:  redo record is at
86/E39E8248; shutdown FALSE
2011-06-08 11:40:48 MDT [6072]: [5-1] user= DEBUG:  next transaction
ID: 0/35456371; next OID: 34090526
2011-06-08 11:40:48 MDT [6072]: [6-1] user= DEBUG:  next MultiXactId:
523; next MultiXactOffset: 1046
2011-06-08 11:40:48 MDT [6072]: [7-1] user= DEBUG:  oldest unfrozen
transaction ID: 654, in database 1
2011-06-08 11:40:48 MDT [6072]: [8-1] user= DEBUG:  transaction ID
wrap limit is 2147484301, limited by database with OID 1
2011-06-08 11:40:48 MDT [6072]: [9-1] user= DEBUG:  initializing for hot standby
2011-06-08 11:40:48 MDT [6072]: [10-1] user= LOG:  redo starts at 86/E39E8248
2011-06-08 11:40:48 MDT [6072]: [11-1] user= LOG:  invalid record
length at 86/E39F2010
2011-06-08 11:40:48 MDT [6074]: [1-1] user= LOG:  streaming
replication successfully connected to primary
2011-06-08 11:40:49 MDT [6072]: [12-1] user= LOG:  invalid record
length at 86/E3A16010
2011-06-08 11:40:49 MDT [6074]: [2-1] user= FATAL:  terminating
walreceiver process due to administrator command
2011-06-08 11:40:49 MDT [6072]: [13-1] user= LOG:  invalid record
length at 86/E3A3C010
2011-06-08 11:40:53 MDT [6072]: [14-1] user= LOG:  invalid record
length at 86/E3A54010
2011-06-08 11:40:53 MDT [6075]: [1-1] user= FATAL:  terminating
walreceiver process due to administrator command
2011-06-08 11:40:53 MDT [6072]: [15-1] user= LOG:  invalid record
length at 86/E3A74010
2011-06-08 11:40:58 MDT [6076]: [1-1] user= LOG:  streaming
replication successfully connected to primary
2011-06-08 11:40:59 MDT [6072]: [16-1] user= LOG:  invalid record
length at 86/E3AC6010
2011-06-08 11:40:59 MDT [6076]: [2-1] user= FATAL:  terminating
walreceiver process due to administrator command
2011-06-08 11:40:59 MDT [6072]: [17-1] user= LOG:  invalid record
length at 86/E3ACC010
2011-06-08 11:41:03 MDT [6072]: [18-1] user= LOG:  invalid record
length at 86/E3B32010
2011-06-08 11:41:03 MDT [6078]: [1-1] user= FATAL:  terminating
walreceiver process due to administrator command
[ repeats... ]

Originally I thought there might be some corner case bug  in 9.0.3 or
9.0.4. However after recompiling 9.0.2 with gcc 4.6 and hitting the
same problem-- I tried compiling 9.0.4 with gcc 4.5 and it seemed to
work great. I then tired various optimization levels on 4.6:
-O0: works
-O1: works
-O2: fails
-Os: works

I suppose the next step is to narrow it down to a specific flag -O2
uses... But I thought I would post here first-- maybe someone else has
hit this? Or maybe someone has a bright idea on how to narrow this
down?

# linux 2.6.39.1 x86_64 AMD opteron box
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: /build/src/gcc-4.6-20110603/configure --prefix=/usr
--libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man
--infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++
--enable-shared --enable-threads=posix --with-system-zlib
--enable-__cxa_atexit --disable-libunwind-exceptions
--enable-clocale=gnu --enable-gnu-unique-object
--enable-linker-build-id --with-ppl --enable-cloog-backend=isl
--enable-lto --enable-gold --enable-ld=default --enable-plugin
--with-plugin-ld=ld.gold --disable-multilib --disable-libstdcxx-pch
--enable-checking=release
Thread model: posix
gcc version 4.6.0 20110603 (prerelease) (GCC)

-- 
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] gcc 4.6 and hot standby

[Alex Hunsaker]

On Wed, Jun 8, 2011 at 12:12, Alex Hunsaker bada...@gmail.com wrote:
 So I've been delaying moving some production boxes over to 9.0.4 from
 9.0.2 because hot standby fails with:
 (this is on the hot standby machine that connects to the master)
 [ ...]
 2011-06-08 11:41:03 MDT [6072]: [18-1] user= LOG:  invalid record
 length at 86/E3B32010
 2011-06-08 11:41:03 MDT [6078]: [1-1] user= FATAL:  terminating
 walreceiver process due to administrator command
 [ repeats... ]

 [...] I then tired various optimization levels on 4.6:
 -O0: works
 -O1: works
 -O2: fails
 -Os: works

So I tracked it down to -fgcse, that is CFLAGS=-O2 -fno-gcse
./configure works. I then took a few guesses and compiled all of
postgres with -O2, then manually recompiled xlog.c with -f-no-gcse.
that combination seems to work.

[ One thing im not sure is why -Os works, I tried -O2 and added all
the -fno-XXX options it says -Os adds. I suppose its either they turn
off/on other optimizations the man page does not mention, or I guess
thats compiler bugs for ya ]

-- 
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] gcc 4.6 and hot standby

[Alex Hunsaker]

On Wed, Jun 8, 2011 at 12:49, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 So I've been delaying moving some production boxes over to 9.0.4 from
 2011-06-08 11:41:03 MDT [6078]: [1-1] user= FATAL:  terminating
 walreceiver process due to administrator command
 [ repeats... ]

 I suppose the next step is to narrow it down to a specific flag -O2
 uses... But I thought I would post here first-- maybe someone else has
 hit this? Or maybe someone has a bright idea on how to narrow this
 down?

 Maybe using a prerelease gcc version isn't such a hot idea for
 production.  It's very, very, very difficult to see how the behavior you
 describe isn't a compiler bug.

Yeah :-). However ill note it looks like its the default compiler for
fedora 15, ubuntu natty and debian sid.

 It might be useful to strace the postmaster and walreceiver processes
 just to see if any signal is actually being sent or received.

Will do.

-- 
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] gcc 4.6 and hot standby

[Alex Hunsaker]

On Wed, Jun 8, 2011 at 16:20, Mark Kirkwood
mark.kirkw...@catalyst.net.nz wrote:
 On 09/06/11 06:58, Alex Hunsaker wrote:

 Yeah :-). However ill note it looks like its the default compiler for
 fedora 15, ubuntu natty and debian sid.


 FWIW Ubuntu natty uses gcc 4.5.2, probably just as as well in the light of
 your findings :-)

Yeah I was just looking at distrowatch, its snapshot natty that uses
4.6.0. ubuntu 11.04 uses 4.5.2 like you said.

http://distrowatch.com/table.php?distribution=ubuntu

-- 
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] [Pgbuildfarm-members] CREATE FUNCTION hang on test machine polecat on HEAD

[Alex Hunsaker]

On Mon, Jun 6, 2011 at 21:16, Robert Creager robert.crea...@oracle.com wrote:

 That's weird. Why it should hang there I have no idea. Did it hang at the
 same spot both times? Can you get a backtrace?

 I think so, but I didn't pay much attention :-(
 GNU gdb 6.3.50-20050815 (Apple version gdb-1518) (Sat Feb 12 02:52:12 UTC
 2011)
 Copyright 2004 Free Software Foundation, Inc.
 GDB is free software, covered by the GNU General Public License, and you are
 welcome to change it and/or distribute copies of it under certain
 conditions.
 Type show copying to see the conditions.
 There is absolutely no warranty for GDB.  Type show warranty for details.
 This GDB was configured as x86_64-apple-darwin...Reading symbols for
 shared libraries .. done

 Attaching to program: `/Volumes/High
 Usage/usr/local/src/build-farm-4.4/builds/HEAD/inst/bin/postgres', process
 24698.
 Reading symbols for shared libraries .+. done
 0x000100a505e4 in Perl_get_hash_seed ()
 (gdb) bt
 #0  0x000100a505e4 in Perl_get_hash_seed ()
 #1  0x000100a69b94 in perl_parse ()

Perl_get_hash_seed is basically:

Perl_get_hash_seed {
char *s = getenv(PERL_HASH_SEED);
unsigned long myseed = 0;
if(s) {
  
  myseed = atoul(s);
}
srand(Perl_seed());
myseed = rand() *  UV_MAX;
return myseed;
}

U32 Perl_seed()
{
U32 u;
struct timeval when;
...
open(fd, /dev/urandom...)
read(fd, u, sizeof(u));
gettimeofday(when, NULL);
u = when[0] + SEED_C2 * when[1];
u += getpid();
u += PTR2UV(PL_stack_sp);
return u;
}

I don't suppose /dev/urandom blocks on OS X?  Granted, I may have
missed something in translation with the macro fest that is perl...

-- 
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] [Pgbuildfarm-members] CREATE FUNCTION hang on test machine polecat on HEAD

[Alex Hunsaker]

On Tue, Jun 7, 2011 at 11:48, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 On Mon, Jun 6, 2011 at 21:16, Robert Creager robert.crea...@oracle.com 
 wrote:
 (gdb) bt
 #0  0x000100a505e4 in Perl_get_hash_seed ()
 #1  0x000100a69b94 in perl_parse ()

 I don't suppose /dev/urandom blocks on OS X?

 The man page for it avers not, and besides it's hard to believe that
 there wouldn't be a libc routine or two on the stack if we were blocked
 in a kernel call,

Yeah.

 and also Robert showed that the process was consuming
 CPU time, so it's not blocked.  Tis puzzling if there's no loop in the
 function.

Well there is one, I snipped it out for brevity (I don't see how it
could be at fault):

const char *s = PerlEnv_getenv(PERL_HASH_SEED);
if (s)
   while (isSPACE(*s))
 s++;
if (s  isDIGIT(*s))
myseed = (UV)Atoul(s);
else
{
  srand(Perl_seed());
  myseed = rand() *UV_MAX;
   
}

Im looking at the raw perl 5.10.0 source... I wonder if apple is
shipping a modified version?

-- 
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] [Pgbuildfarm-members] CREATE FUNCTION hang on test machine polecat on HEAD

[Alex Hunsaker]

On Tue, Jun 7, 2011 at 12:22, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 Im looking at the raw perl 5.10.0 source... I wonder if apple is
 shipping a modified version?

 You could find out by digging around at
 http://www.opensource.apple.com/
 polecat appears to be running OSX 10.6.7, so this is what you want:
 http://www.opensource.apple.com/tarballs/perl/perl-63.tar.gz

Thanks!

 Another question worth asking here is whether PG is picking up perl
 5.10.0 or 5.8.9, both of which are shipped in this OSX release.

I was looking at
http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=polecatdt=2011-06-07%2015%3A23%3A34stg=config
which seems to point at 5.10.0.

Robert: perl -V might be useful

-- 
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] Fix for Perl 5.14

[Alex Hunsaker]

On Sat, Apr 23, 2011 at 07:00, Andrew Dunstan and...@dunslane.net wrote:

 This looks OK, but I think we need to wait until they remove the RC tag.

5.14.0 is out now, Ive retested with 5.14.0 (x86-64), 5.12.3 (x86-64)
and 5.10.1 (i386). No changes are needed.

[ if you missed it ]
The Devel::PPPort guy said patches are welcome and he won't be able to
look into this until at least june.

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

[HACKERS] Fix for Perl 5.14

[Alex Hunsaker]

Perl 5.14.0-RC1 came out a few days ago...

There is a minor compile time error due to the API changing a bit:
plperl.c:929:3: error: lvalue required as left operand of assignment

This is due to GvCV() no longer returning an lvalue, instead they want
us to use the new GvCV_set macro. (see
http://search.cpan.org/~jesse/perl-5.14.0-RC1/pod/perldelta.pod#GvCV()_and_GvGP()_are_no_longer_lvalues)

Unfortunately  that macro is not available on older perls so the
attached provides our own macro when GvCV_set is not defined.

Tested with 5.14.0-rc1 and 5.12.3.

The -head patch applies with fuzz to 9.0. The 8.4 patch applies clean
to 8.4 and with fuzz to 8.3 and 8.2.
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 926,932  plperl_trusted_init(void)
  		if (!isGV_with_GP(sv) || !GvCV(sv))
  			continue;
  		SvREFCNT_dec(GvCV(sv)); /* free the CV */
! 		GvCV(sv) = NULL;		/* prevent call via GV */
  	}
  	hv_clear(stash);
  
--- 926,932 
  		if (!isGV_with_GP(sv) || !GvCV(sv))
  			continue;
  		SvREFCNT_dec(GvCV(sv)); /* free the CV */
! 		GvCV_set(sv, NULL);		/* prevent call via GV */
  	}
  	hv_clear(stash);
  
*** a/src/pl/plperl/plperl.h
--- b/src/pl/plperl/plperl.h
***
*** 49,54 
--- 49,59 
  (U32)HeKUTF8(he))
  #endif
  
+ /* supply GvCV_set if it's missing - ppport.h doesn't supply it, unfortunately */
+ #ifndef GvCV_set
+ #define GvCV_set(gv, cv)		(GvCV(gv) = cv)
+ #endif
+ 
  /* declare routines from plperl.c for access by .xs files */
  HV		   *plperl_spi_exec(char *, int);
  void		plperl_return_next(SV *);
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 700,706  plperl_trusted_init(void)
  		if (!isGV_with_GP(sv) || !GvCV(sv))
  			continue;
  		SvREFCNT_dec(GvCV(sv)); /* free the CV */
! 		GvCV(sv) = NULL;		/* prevent call via GV */
  	}
  	hv_clear(stash);
  	/* invalidate assorted caches */
--- 700,706 
  		if (!isGV_with_GP(sv) || !GvCV(sv))
  			continue;
  		SvREFCNT_dec(GvCV(sv)); /* free the CV */
! 		GvCV_set(sv, NULL);		/* prevent call via GV */
  	}
  	hv_clear(stash);
  	/* invalidate assorted caches */
*** a/src/pl/plperl/plperl.h
--- b/src/pl/plperl/plperl.h
***
*** 43,48 
--- 43,53 
  #undef bool
  #endif
  
+ /* supply GvCV_set if it's missing - ppport.h doesn't supply it, unfortunately */
+ #ifndef GvCV_set
+ #define GvCV_set(gv, cv)		(GvCV(gv) = cv)
+ #endif
+ 
  /* routines from spi_internal.c */
  int			spi_DEBUG(void);
  int			spi_LOG(void);

-- 
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] Fix for Perl 5.14

[Alex Hunsaker]

On Sat, Apr 23, 2011 at 07:00, Andrew Dunstan and...@dunslane.net wrote:


 On 04/23/2011 03:02 AM, Alex Hunsaker wrote:
 ...
 There is a minor compile time error due to the API changing a bit:
 plperl.c:929:3: error: lvalue required as left operand of assignment
 ...
 Unfortunately  that macro is not available on older perls so the
 attached provides our own macro when GvCV_set is not defined.

 How nice of them not to fix it in ppport.h. I thought this is exactly the
 sort of thing it's for.

Hrm, I have the latest released version of Devel::PPPort, 3.19. I went
poking around and found the have a newer developer release (3.19_03)
at http://search.cpan.org/~mhx/Devel-PPPort-3.19_03/ (released
2011-04-13). I see a few things for 5.14 but nothing about GvCV_set.
Maybe I'm doing something wrong? I'm just diffing its ppport.h with
ours. For the curious its attached.

 This looks OK, but I think we need to wait until they remove the RC tag.

Makes sense, Ill repost once they do ;).


ppport.patch.gz
Description: GNU Zip compressed data

-- 
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] Formatting Curmudgeons WAS: MMAP Buffers

[Alex Hunsaker]

On Mon, Apr 18, 2011 at 19:50, Josh Berkus j...@agliodbs.com wrote:
 You'll notice that this has been a complaint of veteran contributors as
 well; WIP patches either get no review, or get reviewed as if they were
 expected to be committable.

I don't see this changing anytime in the future. We have a hard enough
time getting finished patches reviewed.

 The person who e-mailed me suggests some form of PostgreSQL Incubator as
 a solution.   I'm not sure about that, but it does seem to me that we
 need somewhere or some way that people can submit patches, ideas, git
 forks, etc., for discussion without that discussion needing to
 immediately move to the cleanliness/maintainability/supportable status
 of the patch.

Reminds me a bit of what linux is doing with the staging tree. I
don't see anyway for that to work with postgres (lower the bar for
-contrib?).

You can fork fairly easy with github nowdays. For example the replace
GEQ with SA is on one of those git sites. Does that mean it gets any
attention? *shrug*

-- 
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] arrays as pl/perl input arguments [PATCH]

[Alex Hunsaker]

On Thu, Feb 17, 2011 at 16:18, Alvaro Herrera
alvhe...@commandprompt.com wrote:
 Excerpts from Alex Hunsaker's message of sáb feb 12 04:53:14 -0300 2011:

 - make plperl.o depend on plperl_helpers.h (should have been done in
 the utf8 patch)

 Incidentally, I think this bit was lost, no?

It was, yes.

-- 
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] arrays as pl/perl input arguments [PATCH]

[Alex Hunsaker]

On Wed, Feb 16, 2011 at 12:21, Alvaro Herrera
alvhe...@commandprompt.com wrote:
 Excerpts from Tim Bunce's message of mié feb 16 14:08:11 -0300 2011:
 I'd suggest encode_typed_literal() as a better name.

 FYI I'm looking at this patch (v10), and I'll incorporate Tim's suggestion.

Looks good to me.

-- 
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] CommitFest 2011-01 as of 2011-02-04

[Alex Hunsaker]

On Mon, Feb 14, 2011 at 09:49, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
 Here's where I think we are with this CommitFest.

  Subject: Re: [HACKERS] CommitFest 2011-01 as of 2011-02-04

 I'm gonna go out on a limb and hope you meant '2011-02-14' there. :)

 So there are two basic difficulties with wrapping the CommitFest up.

 [ ... ] It's been working really rather well, which is due in
 great part to the excellent CF managers (thanks again for being that,
 again).
[ ... ]

        Thanks again, Robert, you've done an excellent job managing the CF.

I'd like to second this sentiment. I'm positive no one can really
appreciate the amount of work that goes into managing a commitfest
other than the elite few who have.

I imagine its not unlike being a mother: constantly bitching, ( out of
necessity mind you-- don't commit that! its HOT!, did you finish all
of your reviews? no? there are starving people in china! etc.) and yet
underrated, under-thanked and often undervalued.

Anyway, good job and thanks for volunteering to be the bad guy that
tries to keep the 9.1 time table sane.

-- 
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] why two dashes in extension load files

[Alex Hunsaker]

On Tue, Feb 15, 2011 at 14:12, Robert Haas robertmh...@gmail.com wrote:
 On Tue, Feb 15, 2011 at 3:26 PM, marcin mank marcin.m...@gmail.com wrote:
 how about : we use a single dash as the separator, and if the
 extension author insists on having a dash in the name, as a punishment
 he must duplicate the dash, i.e.:
 uuid--ossp-1.0--5.5.sql

 That has a certain poetic justice to it.

Im not sure I see the poetic justice in trying to punish others for
*our* arbitrary naming rules. *shrug*

-- 
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] pl/python tracebacks

[Alex Hunsaker]

On Sat, Feb 12, 2011 at 01:50, Jan Urbański wulc...@wulczer.org wrote:
 On 12/02/11 04:12, Alex Hunsaker wrote:
 In PLy_traceback fname and prname look like they will leak (well as
 much as a palloc() in an error path can leak I suppose).

 But they're no palloc'd, no? fname is either a static module string,
 or PyString_AsString, which also doesn't allocate memory, AFAIK.

Yeah, I was flat out wrong about proname :-(.

As for fname, I must be missing some magic. We have:

#if PY_MAJOR_VERSION  3
...
#define PyString_AsString(x) PLyUnicode_AsString(x)

PLyUnicode_AsString(PyObject *unicode)
{
PyObject   *o = PLyUnicode_Bytes(unicode);
char   *rv = pstrdup(PyBytes_AsString(o));

Py_XDECREF(o);
return rv;
}

PyString_AsString is used all over the place without any pfrees. But I
have no Idea how that pstrdup() is getting freed if at all.

Care to enlighten me ?

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

[HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Sun, Feb 6, 2011 at 15:31, Andrew Dunstan and...@dunslane.net wrote:
 Force strings passed to and from plperl to be in UTF8 encoding.

 String are converted to UTF8 on the way into perl and to the
 database encoding on the way back. This avoids a number of
 observed anomalies, and ensures Perl a consistent view of the
 world.

So I noticed a problem while playing with this in my discussion with
David Wheeler. pg_do_encoding() does nothing when the src encoding ==
the dest encoding. That means on a UTF-8 database we fail make sure
our strings are valid utf8.

An easy way to see this is to embed a null in the middle of a string:
= create or replace function zerob() returns text as $$ return
abcd\0efg; $$ language plperl;
= SELECT zerob();
abcd

Also It seems bogus to bogus to do any encoding conversion when we are
SQL_ASCII, and its really trivial to fix.

With the attached:
- when we are on a utf8 database make sure to verify our output string
in sv2cstr (we assume database strings coming in are already valid)

- Do no string conversion when we are SQL_ASCII in or out

- add plperl_helpers.h as a dep to plperl.o in our makefile

- remove some redundant calls to pg_verify_mbstr()

- as utf_e2u only as one caller dont pstrdup() instead have the caller
check (saves some cycles and memory)
*** a/doc/src/sgml/plperl.sgml
--- b/doc/src/sgml/plperl.sgml
***
*** 127,135  $$ LANGUAGE plperl;
  
note
  para
!   Arguments will be converted from the database's encoding to UTF-8 
!   for use inside plperl, and then converted from UTF-8 back to the 
!   database encoding upon return. 
  /para
/note
  
--- 127,136 
  
note
  para
!   Arguments will be converted from the database's encoding to
!   UTF-8 for use inside plperl, and then converted from UTF-8 back
!   to the database encoding upon return.  Unless the database
!   encoding is SQL_ASCII, then no conversion is done.
  /para
/note
  
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 54,60  PSQLDIR = $(bindir)
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h
  
  plperl_opmask.h: plperl_opmask.pl
  	$(PERL) $ $@
--- 54,60 
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h plperl_helpers.h
  
  plperl_opmask.h: plperl_opmask.pl
  	$(PERL) $ $@
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 2308,2315  plperl_spi_exec(char *query, int limit)
  	{
  		int			spi_rv;
  
- 		pg_verifymbstr(query, strlen(query), false);
- 
  		spi_rv = SPI_execute(query, current_call_data-prodesc-fn_readonly,
  			 limit);
  		ret_hv = plperl_spi_execute_fetch_result(SPI_tuptable, SPI_processed,
--- 2308,2313 
***
*** 2555,2563  plperl_spi_query(char *query)
  		void	   *plan;
  		Portal		portal;
  
- 		/* Make sure the query is validly encoded */
- 		pg_verifymbstr(query, strlen(query), false);
- 
  		/* Create a cursor for the query */
  		plan = SPI_prepare(query, 0, NULL);
  		if (plan == NULL)
--- 2553,2558 
***
*** 2767,2775  plperl_spi_prepare(char *query, int argc, SV **argv)
  			qdesc-argtypioparams[i] = typIOParam;
  		}
  
- 		/* Make sure the query is validly encoded */
- 		pg_verifymbstr(query, strlen(query), false);
- 
  		/
  		 * Prepare the plan and check for errors
  		 /
--- 2762,2767 
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 20,27  static inline char *
  utf_e2u(const char *str)
  {
  	char *ret = (char*)pg_do_encoding_conversion((unsigned char*)str, strlen(str), GetDatabaseEncoding(), PG_UTF8);
- 	if (ret == str)
- 		ret = pstrdup(ret);
  	return ret;
  }
  
--- 20,25 
***
*** 34,46  sv2cstr(SV *sv)
  {
  	char *val;
  	STRLEN len;
  
  	/*
! 	 * get a utf8 encoded char * out of perl. *note* it may not be valid utf8!
  	 */
  	val = SvPVutf8(sv, len);
  
  	/*
  	 * we use perls length in the event we had an embedded null byte to ensure
  	 * we error out properly
  	 */
--- 32,59 
  {
  	char *val;
  	STRLEN len;
+ 	int enc = GetDatabaseEncoding();
  
  	/*
! 	 * we dont do any conversion when SQL_ASCII
  	 */
+ 	if (enc == PG_SQL_ASCII)
+ 	{
+ 		val = SvPV(sv, len);
+ 		return pstrdup(val);
+ 	}
+ 
  	val = SvPVutf8(sv, len);
  
  	/*
+ 	 * when the src encoding matches the dest encoding pg_do_encoding will not
+ 	 * do any verification. SvPVutf8() may return invalid utf8 so we need to do
+ 	 * that ourselves
+ 	 */
+ 	if (enc == PG_UTF8)
+ 		pg_verifymbstr(val, len, false);
+ 
+ 	/*
  	 * we use perls length in the event we had an embedded null byte to ensure
  	 * we error out properly
  	 */
***
*** 56,67  static inline SV *
  cstr2sv(const char *str)
  {
  	SV *sv;

Re: [HACKERS] pl/python invalidate functions with composite arguments

[Alex Hunsaker]

On Fri, Feb 11, 2011 at 08:47, Robert Haas robertmh...@gmail.com wrote:
 On Thu, Feb 10, 2011 at 9:06 PM, Alex Hunsaker bada...@gmail.com wrote:
 On Wed, Feb 9, 2011 at 02:09, Jan Urbański wulc...@wulczer.org wrote:
 On 27/01/11 22:42, Jan Urbański wrote:
 On 23/12/10 14:50, Jan Urbański wrote:
 Here's a patch implementing properly invalidating functions that have
 composite type arguments after the type changes, as mentioned in
 http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
 an incremental patch on top of the plpython-refactor patch sent eariler.

 Updated to master.

 Again.

 Looks good, it works as described, the code is clean and well
 documented and it passes the added regression tests.

 I took the liberty of looking at the other pls to see how they handled
 this to find they don't cache them in the first place. For fun, I
 hacked plpython to not cache to see if there was any performance
 difference pre patch, post patch and in the non-cached cases. I
 couldn't find any probably due to:
 1) my simple test case (select
 count(test_composite_table_input('(John, 100, (10))')) FROM
 generate_series(1, 100);)
 2) things being cached
 3) cache invalidation having to do most of the work that the non
 caching cache does. I think there is one or two more SearchSysCall's.
 4) overhead from cassert

 It seems a bit heavy handed to invalidate and remake the entire
 plpython function whenever we hit this case. I think we could get away
 with setting -is_rowtype = 2 in PLy_procedure_valid() instead. I
 suppose it should be fairly rare case anyway so... *shrug*.

 This last comment might make me think that we're looking for a new
 version of this patch, but the comment on the CommitFest application
 says looks good.  So I'm not sure whether this should be marked
 Waiting on Author or Ready for Committer, but the current status of
 Needs Review looks wrong.

Drat, Ive been found it. I just didn't want to make things to easy for you. :)

-- 
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] pl/python tracebacks

[Alex Hunsaker]

On Fri, Feb 11, 2011 at 08:45, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański wulc...@wulczer.org wrote:
 On 06/02/11 20:12, Jan Urbański wrote:
 On 27/01/11 22:58, Jan Urbański wrote:
 On 23/12/10 14:56, Jan Urbański wrote:
 Here's a patch implementing traceback support for PL/Python mentioned in
 http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
 an incremental patch on top of the plpython-refactor patch sent eariler.

 Updated to master.

 Updated to master again.

 Once more.

 Alex Hunsaker is listed as the reviewer for this patch, but I don't
 see a review posted.  If this feature is still wanted for 9.1, can
 someone jump in here?

Goodness... I picked up this patch the day before yesterday because
no-one was listed. That being said, if anyone else wants to beat me to
the punch of reviewing this, have at it! The more eyes the merrier!

I wish I could squeeze
the lime of my time to find
a few more hours

-- 
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] Careful PL/Perl Release Not Required

[Alex Hunsaker]

On Fri, Feb 11, 2011 at 10:16, David E. Wheeler da...@kineticode.com wrote:
 On Feb 10, 2011, at 11:43 PM, Alex Hunsaker wrote:

 Like I said, the terminology is awful.

Yeah I use encode and decode to mean the same thing frequently :-(.

 In the the cited case he was passing %C3%A9 to uri_unescape() and
 expecting it to return 1 character. The additional utf8::decode() will
 tell perl the string is in utf8 so it will then return 1 char. The
 point being, decode is needed and with it, the function will work pre
 and post 9.1.

 Why wouldn't the string be decoded already when it's passed to the function, 
 as it would be in 9.0 if the database was utf-8, and should be in 9.1 if the 
 database isn't sql_ascii?

It is decoded... the input string %C3%A9 actually is the _same_
string utf-8, latin1 and SQL_ASCII decoded or not. Those are all ascii
characters. Calling utf8::decode(%C3%A9) is essentially a noop.

 In-fact on a latin-1 database it sure as heck better return two
 characters, it would be a bug if it only returned 1 as that would mean
 it would be treating a series of latin1 bytes as a series of utf8
 bytes!

 If it's a latin-1 database, in 9.1, the argument should be passed decoded. 
 That's not a utf-8 string or bytes. It's Perl's internal representation.

 If I understand the patch correctly, the decode() will no longer be needed. 
 The string will *already* be decoded.

Ok, I think i figured out why we seem to be talking past each other, we have:
CREATE OR REPLACE FUNCTION url_decode(Vkw varchar) RETURNS varchar  AS $$
use strict;
use URI::Escape;
utf8::decode($_[0]);
return uri_unescape($_[0]); $$ LANGUAGE plperlu;

That *looks* like it is decoding the input string, which it is, but
actually that will double utf8 encode your string. It does not seem to
in this case because we are dealing with all ascii input. The trick
here is its also telling perl to decode/treat the *output* string as
utf8.

uri_unescape() returns the same string you passed in, which thanks to
the utf8::decode() above has the utf8 flag set. Meaning we end up
treating it as 1 character instead of two. Or basically that it has
the same effect as calling utf8::decode() on the return value.

The correct way to write that function pre 9.1 and post 9.1 would be
(in a utf8 database):
CREATE OR REPLACE FUNCTION url_decode(Vkw varchar) RETURNS varchar  AS $$
use strict;
use URI::Escape;
my $str = uri_unescape($_[0]);
utf8::decode($str);
return $str;
$$ LANGUAGE plperlu;

The last utf8::decode being optional (as we said, it might not be
utf8), but granting the sought behavior by the op.

-- 
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] Careful PL/Perl Release Not Required

[Alex Hunsaker]

On Fri, Feb 11, 2011 at 10:44, Alex Hunsaker bada...@gmail.com wrote:
 On Fri, Feb 11, 2011 at 10:16, David E. Wheeler da...@kineticode.com wrote:

 That *looks* like it is decoding the input string, which it is, but
 actually that will double utf8 encode your string. It does not seem to
 in this case because we are dealing with all ascii input. The trick
 here is its also telling perl to decode/treat the *output* string as
 utf8.

Urp, this is a bit of a fib. The problem is actual in plperl not perl
persay. Pre 9.1 we always fetched perls internal string *ignoring* the
utf8 flag. So if you had octets that were utf8 things would work. The
utf8::decode($_[0]); uri_unescape($_[0]); happened to make the return
string internally be utf8 and so it would only return 1 char. Thats
what the op wanted and why it seemed to fix his problem. But thats
actually a bug! utf8::decode($_[0]) should not have changed anything
at all on the output side. It should still have returned 2 characters
instead of 1.

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