Re: [HACKERS] Another review of URI for libpq, v7 submission
Daniel Farina writes: > >> Finally, attached is v8. Hopefully I didn't mess things up too much. > > I'll give it another look-over. Do you have these in git somewhere? It > will help me save time on some of the incremental changes. Yes, I've just pushed my dev branch to this fork of mine: https://github.com/a1exsh/postgres/commits/uri -- 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] Another review of URI for libpq, v7 submission
Marko Kreen writes:
> On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
>> https://github.com/a1exsh/postgres/commits/uri
>
> The point of the patch is to have one string with all connection options,
> in standard format, yes? So why does not this work:
>
> db = PQconnectdb("postgres://localhost");
>
> ?
Good catch.
I've figured out that we'll need a bit more intrusive change than simply
overriding the expand_dbname check in conninfo_array_parse (like the
current version does) to support URIs in all PQconnect* variants.
I still need to figure out some details, but this is to give people a
status update.
--
Alex
--
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] Another review of URI for libpq, v7 submission
Alex writes:
> Marko Kreen writes:
>
>> On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
>>> https://github.com/a1exsh/postgres/commits/uri
>>
>> The point of the patch is to have one string with all connection options,
>> in standard format, yes? So why does not this work:
>>
>> db = PQconnectdb("postgres://localhost");
>>
>> ?
>
> Good catch.
>
> I've figured out that we'll need a bit more intrusive change than simply
> overriding the expand_dbname check in conninfo_array_parse (like the
> current version does) to support URIs in all PQconnect* variants.
Okay, at last here's v9, rebased against current master branch.
What's new in this version is parse_connection_string function to be
called instead of conninfo_parse. It will check for possible URI in the
connection string and dispatch to conninfo_uri_parse if URI was found,
otherwise it will fall back to conninfo_parse.
In two places in code we don't want to parse the string unless it is
deemed a real connection string and not plain dbname. The new function
recognized_connection_string is added to check this.
Thanks Marko for spotting the problem and thanks Tom for committing the
refactoring patch!
--
Alex
binFI5wAVF1ys.bin
Description: libpq-uri-v9.patch
--
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] Another review of URI for libpq, v7 submission
Peter Eisentraut writes: > On tor, 2012-03-22 at 23:42 +0200, Alex wrote: >> Okay, at last here's v9, rebased against current master branch. > > Attached is a patch on top of your v9 with two small fixes: > > - Don't provide a check target in libpq/Makefile if it's not > implemented. > > - Use the configured port number for running the tests (otherwise it > runs against my system installation, for example). Neat, thank 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] Another review of URI for libpq, v7 submission
Alex writes: > Peter Eisentraut writes: >> >> Attached is a patch on top of your v9 with two small fixes: >> >> - Don't provide a check target in libpq/Makefile if it's not >> implemented. >> >> - Use the configured port number for running the tests (otherwise it >> runs against my system installation, for example). > > Neat, thank you. Attached is a gzipped v10, complete with the above fixes and fixes to bugs found by Heikki. Documentation and code comments are also finally updated. Of all the command-line utilities which can accept conninfo string, only psql mentions that, so only its documentation was updated. Other utilities, like pg_dump and pg_restore can also work with either conninfo or URI, however this remains undocumented. -- Alex libpq-uri-v10.patch.gz Description: libpq-uri-v10.patch.gz -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Display output file name in psql prompt?
Hello fellow hackers, Today when I get back home and connected to my psql prompt I've left running under $terminal_multiplexor I've run some SQL, but no output did appear on my screen. I have immediately realized that it must be sending it to a file instead of STDOUT as I have instructed it earlier today using \o command, but have totally forgot about the fact. So I wonder if there's a way to make psql display the current output file/command in the prompt? After reading the current docs, I don't see any option. Would a patch be accepted for submission (make it into default prompt, unless stdout?) Something like this: \set PROMPT1 '%/:%o%R%# ' postgres:/path/to/output/file.txt=> -- Regards, Alex PS: upon reading the docs more carefully, I guess I should have used \g instead of \o, but still this might be not a bad feature to have. -- 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] Display output file name in psql prompt?
Josh Berkus writes: > Tom, > >> If you're proposing changing the contents of the default prompt, I think >> that has very little chance of passing. A new option for something to >> add into a custom prompt might get accepted. I'm not sure that that >> approach would do much for the scenario you describe, since it's >> unlikely that anyone would add it to their prompt (at least not before >> they'd gotten burnt...). However, I don't recall hearing of anyone >> getting confused like this before, and you say you figured it out pretty >> quickly, so it doesn't seem like a big problem. > > Well, I think having a \ command to show the current \o setting would be > fine. Actually, it might be better to show *all* psql settings with a > general command, including things like \timing and \pset. Actually in my case, I've also used \t and \a psql commands to produce undecorated machine-readable output and now I think it would be nice to also show the status of these in the prompt. What if we add '%\' substitute to expand to the list of all settings with non-default values? Like this: postgres=# \set PROMPT1 '%/%\%R%# ' postgres=# -- Nothing is set, so '%\' expands to an empty string. postgres=# \t Showing only tuples. postgres\t=# \a Output format is unaligned. postgres\a\t=# \o /path/to/output/file.out postgres\a\o=/path/to/output/file.out\t=# Not sure how \pset could fit there, it might be a bit excessive. Another idea is we could add \O and \G commands that will act like the plain \o and \g, but will set \a and \t automatically. I mean it must be quite often that you don't need all the decoration when you save query results to a file, so instead of doing \a, \t, \g (then setting \a and \t back) you can just do \G and move on. -- Alex -- 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] Last gasp
Peter Eisentraut writes: > On ons, 2012-04-11 at 23:30 -0400, Robert Haas wrote: >> Now what would be sort of neat is if we had a way to keep all the >> versions of patch X plus author and reviewer information, links to >> reviews and discussion, etc. in some sort of centralized place. > > Well, a properly linked email thread contains all this. I have seen a > couple of anti-patterns evolving, though, including patch authors > starting a new email thread for each patch version, and reviewers > starting a new email thread for each review. When that happens, the > existence of the commitfest app makes things worse, in a way. (Of > course, any discussion here about bug trackers emphasizes the need for > email to be the primary communications method for this very reason.) I didn't follow this whole thread, but have we considered Redmine[1]? We at CMD rely on it as the primary means of customer communication, via email. New email to a mailing list backed by Redmine triggers new ticket creation and any followups result into an update to the ticket. The ticket history is searchable, browsable (and updateable) on the web. It's GPL-ed, uses Ruby on Rails, and comes with a broad range of plugins[2]. Also it is quite hackable (for a RoR guy, of course.) We could integrate Redmine with the CommitFest or even replace it completely with a custom-written plugin. A command line interface[3] exists, so 'cf close ###' suggested further in this thread should be also possible. -- Regards, Alex [1] http://www.redmine.org/ [2] http://www.redmine.org/plugins [3] https://github.com/diasjorge/redmine-cli -- 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] Last gasp
Greg Smith writes: > On 04/14/2012 03:02 AM, Alex wrote: >> I didn't follow this whole thread, but have we considered Redmine[1]? > > It comes up every couple of years in contexts near this one, such as > http://wiki.postgresql.org/wiki/TrackerDiscussion Oh, I see. I wonder maybe it is time to actually take some action? Given that the list of disadvantages on that wiki page looks really bad (to me.) -- Alex -- 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] Last gasp
Alex writes: > Greg Smith writes: > >> On 04/14/2012 03:02 AM, Alex wrote: >>> I didn't follow this whole thread, but have we considered Redmine[1]? >> >> It comes up every couple of years in contexts near this one, such as >> http://wiki.postgresql.org/wiki/TrackerDiscussion > > Oh, I see. I wonder maybe it is time to actually take some action? > Given that the list of disadvantages on that wiki page looks really bad > (to me.) Err, the list of Disadvantages of Current Situation I mean. -- 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] Last gasp
Jay Levitt writes: > Alex wrote: >> I didn't follow this whole thread, but have we considered Redmine[1]? > > As the resident "Ruby is shiny, let's do everything in Rails on my > MacBook" guy, I'd like to make a statement against interest: I've > tried Redmine a few times and it's been painful. Much of the codebase > is deprecated, it's slow, it has no meaningful search (in 2012?!), > I've seen wiki edits disappear, and at the moment pulling up its own > FAQ page at redmine.org times out. Yay, that's totally FUD to me. Could you please elaborate a bit on your points? Deprecated codebase? Let me guess... It runs on an outdated version of Rails (2.3) but only because Rails is changing so rapidly, I believe. There is work in progress[1] to move to the supported branch Rails-3.x. Slow? Do you have any data to back this point up? No meaningful search, eh? Works for me. Disappearing wiki edits? Never seen that, but you can always file a bug. > Maybe you've had better luck with it, but whenever I've Googled for > Redmine questions, the collective Internet has sighed and said "Yeah, > it was a really good idea, though." Certainly *you've* had some terrible luck with it. ;-) -- Regards, Alex [1] http://www.redmine.org/issues/4796 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug tracker tool we need (was: Last gasp)
Dimitri Fontaine writes: > Alvaro Herrera writes: >> I've used Redmine a lot, as you know, and I only keep using it because >> it's a requirement at work. It is certainly not close to usable for >> general pgsql stuff. (Trac, which we used to use prior to Redmine, was >> certainly much worse, though). > > Same story here, still using redmine a lot, all with custom reports etc. > >> I can't say that it's all that slow, or that there's a problem with the >> code, or that the search doesn't work right (and I've never had a wiki >> edit disappear, either, and I've used that a lot). It's just the wrong >> tool altogether. > > It's indeed slow here, and I agree that's not the problem. Not the tool > we need, +1. I still fail to see how Redmine doesn't fit into requirements summarized at that wiki page[1], so that must be something other than formal requirement of being free/open software and running postgres behind (some sort of "feeling" maybe?) Jay, Alvaro, Dimitri (and whoever else wants to speak up) could you please describe your ideal tool for the task? Given that every other existing tool likely have pissed off someone already, I guess our best bet is writing one from scratch. Or maybe there isn't really a need for a tracker? The core team have managed to live without one for so long after all... -- Regards, Alex [1] http://wiki.postgresql.org/wiki/TrackerDiscussion -- 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] Bug tracker tool we need
Magnus Hagander writes: > One thing to note is that the referenced wiki page is over a year old. > And that many more things have been said on email lists than are > actually in that page. Yeah, I went through it briefly and rather important concern seem to have been raised by Tom Lane in this msg: http://archives.postgresql.org/pgsql-hackers/2011-05/msg01480.php This paragraph: > The real question is, who is going to keep it up to date? GSM has the > right point of view here: we need at least a couple of people who are > willing to invest substantial amounts of time, or it's not going to go > anywhere. Seeing that we can barely manage to keep the mailing list > moderator positions staffed, I'm not hopeful. > But as one note - I don't believe you can drive redmine completely > from email, which is certainly a requirement that has been discussed, > but is not entirely listed on that page. Ugh, what do you mean by that? You can change any attribute (like status, priority, assigned person, etc.) of a ticket via email. Anything else? > FWIW, I think the closest thing we've found so far would be debbugs - > which IIRC doesn't have any kind of reasonable database backend, which > would be a strange choice for a project like ours :) And makes many > things harder... What stops us from writing a postgres backend for debbugs if it is so brilliant on handing email and stuff? -- Alex -- 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] Bug tracker tool we need
> > I believe the biggest hurdle for many hackers is that in redmine, > email is not a first class citizen. The majority of hackers are never > going to want to go into a web interface to get something done, they > live in VI/Emacs and the command line. > > One thing that redmine definitely breaks is proper handling of > attachments in email, thus the first thing moving to redmine would > break would be patch submission. Right. This is not too hard to fix, however, and I didn't suggest we move to vanilla Redmine. It's hackable, so we can just bash it into shape we need (and we need a few Ruby hackers, so there's always someone in the postgres community to fix it when it breaks, of course.) -- Alex -- 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] libpq URI and regression testing
Peter Eisentraut writes: > On tis, 2012-04-17 at 10:47 -0300, Alvaro Herrera wrote: >> What's the preferred way to make it automatically tested as much as >> possible? I know the buildfarm does not run "installcheck-world", so if >> we want it there, it'd need a bit more code on the client side. I think >> it would be wise to have it also run on installcheck-world. > > It was agreed during the patch discussion that it shouldn't be run > automatically. Hm... the testing program we've actually committed avoids any connection attempts and only deals with testing the parser routine. Does it change that? -- Regards, Alex -- 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] libpq URI and regression testing
Andrew Dunstan writes:
>>> That's one reason for that, but there are probably others in the way of
>>> making this fully portable and automatable.
>
> This test setup also appears to labor under the illusion that we live
> in a Unix-only world. And for no good reason that I can tell. The
> shell script should be ripped out and replaced by a perl script which
> could actually be used on any windows build host. The MSVC build
> system also needs adjusting to make it build the test driver, at
> least.
Good catch. Attached is my first take at writing Perl: replaces the
shell script, adds $libpq_uri_regress project to Mkvcbuild.pm.
I don't have access to a win32 box unfortunately, so if anyone who does
could try this out that'd be great.
--
Regards,
Alex
diff --git a/src/interfaces/libpq/test/Makefile b/src/interfaces/libpq/test/Makefile
index b9023c3..f569fc2 100644
--- a/src/interfaces/libpq/test/Makefile
+++ b/src/interfaces/libpq/test/Makefile
@@ -15,7 +15,7 @@ all: $(PROGS)
installcheck: all
SRCDIR='$(top_srcdir)' SUBDIR='$(subdir)' \
- $(SHELL) $(top_srcdir)/$(subdir)/regress.sh
+ $(top_srcdir)/$(subdir)/regress.pl
clean distclean maintainer-clean:
rm -f $(PROGS)
diff --git a/src/interfaces/libpq/test/regress.pl b/src/interfaces/libpq/test/regress.pl
new file mode 100755
index 000..74a877a
--- /dev/null
+++ b/src/interfaces/libpq/test/regress.pl
@@ -0,0 +1,56 @@
+#!/usr/bin/env perl
+use strict;
+
+# use of SRCDIR/SUBDIR is required for supporting VPath builds
+my $srcdir = $ENV{'SRCDIR'} or die '$SRCDIR environment variable is not set';
+my $subdir = $ENV{'SUBDIR'} or die '$SUBDIR environment variable is not set';
+
+my $regress_in = "$srcdir/$subdir/regress.in";
+my $expected_out = "$srcdir/$subdir/expected.out";
+
+# the output file should land in the build_dir of VPath, or just in
+# the current dir, if VPath isn't used
+my $regress_out = "regress.out";
+
+# open input file first, so possible error isn't sent to redirected STDERR
+open(REGRESS_IN, "<$regress_in") or die "Can't open <$regress_in: $!";
+
+# save STDOUT/ERR and redirect both to regress.out
+open(OLDOUT, ">&STDOUT") or die "Can't dup STDOUT: $!";
+open(OLDERR, ">&STDERR") or die "Can't dup STDERR: $!";
+
+open(STDOUT, ">$regress_out") or die "Can't open >$regress_out: $!";
+open(STDERR, ">&STDOUT") or die "Can't dup STDOUT: $!";
+
+# read lines from regress.in and run uri-regress on them
+while () {
+ chomp;
+ print "trying $_\n";
+ system("./uri-regress \"$_\"");
+ print "\n";
+}
+
+# restore STDOUT/ERR so we can print the outcome to the user
+open(STDERR, ">&OLDERR") or die; # can't complain as STDERR is still duped
+open(STDOUT, ">&OLDOUT") or die "Can't restore STDOUT: $!";
+
+# just in case
+close REGRESS_IN;
+
+my $diff_status = system("diff -c \"$srcdir/$subdir/expected.out\" regress.out >regress.diff");
+if ($diff_status == 0) {
+ print <regress.out 2>&1
-
-if diff -c "${SRCDIR}/${SUBDIR}/"expected.out regress.out >regress.diff; then
- echo ""
- echo "All tests passed"
- exit 0
-else
- echo ""
- echo "FAILED: the test result differs from the expected output"
- echo
- echo "Review the difference in ${SUBDIR}/regress.diff"
- echo ""
- exit 1
-fi
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index f0fad43..e65971c 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -229,6 +229,15 @@ sub mkvcbuild
$libpq->ReplaceFile('src\interfaces\libpq\libpqrc.c','src\interfaces\libpq\libpq.rc');
$libpq->AddReference($libpgport);
+my $libpq_uri_regress = $solution->AddProject('libpq_uri_regress','exe','misc');
+$libpq_uri_regress->AddFile('src\interfaces\libpq\test\uri-regress.c');
+$libpq_uri_regress->AddIncludeDir('src\port');
+$libpq_uri_regress->AddIncludeDir('src\interfaces\libpq');
+$libpq_uri_regress->AddLibrary('wsock32.lib');
+$libpq_uri_regress->AddDefine('HOST_TUPLE="i686-pc-win32vc"');
+$libpq_uri_regress->AddDefine('FRONTEND');
+$libpq_uri_regress->AddReference($libpq,$libpgport);
+
my $libpqwalreceiver = $solution->AddProject('libpqwalreceiver', 'dll', '',
'src\backend\replication\libpqwalreceiver');
$libpqwalreceiver->AddIncludeDir('src\interfaces\libpq');
--
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] libpq URI and regression testing
Peter Eisentraut writes:
> On tor, 2012-04-19 at 00:13 +0300, Alex wrote:
>> +#!/usr/bin/env perl
>
> Don't do that. Call the script using $(PERL) from the makefile.
Thank you for the suggestion. Attached v2 does just this (while keeping
a more commonly found shebang line in the perl script for running it w/o
the makefile.)
I figure src/tools/msvc/vcregress.pl will need to be updated too, but
trying to model all this after ecpg regression tests I'm stuck at
replicating ecpg_regression.proj for libpq's uri-regress. I'd
appreciate any help from the Windows guys at this point.
--
Alex
diff --git a/src/interfaces/libpq/test/Makefile b/src/interfaces/libpq/test/Makefile
index b9023c3..048f092 100644
--- a/src/interfaces/libpq/test/Makefile
+++ b/src/interfaces/libpq/test/Makefile
@@ -15,7 +15,7 @@ all: $(PROGS)
installcheck: all
SRCDIR='$(top_srcdir)' SUBDIR='$(subdir)' \
- $(SHELL) $(top_srcdir)/$(subdir)/regress.sh
+ $(PERL) $(top_srcdir)/$(subdir)/regress.pl
clean distclean maintainer-clean:
rm -f $(PROGS)
diff --git a/src/interfaces/libpq/test/regress.pl b/src/interfaces/libpq/test/regress.pl
new file mode 100755
index 000..a19f793
--- /dev/null
+++ b/src/interfaces/libpq/test/regress.pl
@@ -0,0 +1,56 @@
+#!/usr/bin/perl
+use strict;
+
+# use of SRCDIR/SUBDIR is required for supporting VPath builds
+my $srcdir = $ENV{'SRCDIR'} or die '$SRCDIR environment variable is not set';
+my $subdir = $ENV{'SUBDIR'} or die '$SUBDIR environment variable is not set';
+
+my $regress_in = "$srcdir/$subdir/regress.in";
+my $expected_out = "$srcdir/$subdir/expected.out";
+
+# the output file should land in the build_dir of VPath, or just in
+# the current dir, if VPath isn't used
+my $regress_out = "regress.out";
+
+# open input file first, so possible error isn't sent to redirected STDERR
+open(REGRESS_IN, "<$regress_in") or die "Can't open <$regress_in: $!";
+
+# save STDOUT/ERR and redirect both to regress.out
+open(OLDOUT, ">&STDOUT") or die "Can't dup STDOUT: $!";
+open(OLDERR, ">&STDERR") or die "Can't dup STDERR: $!";
+
+open(STDOUT, ">$regress_out") or die "Can't open >$regress_out: $!";
+open(STDERR, ">&STDOUT") or die "Can't dup STDOUT: $!";
+
+# read lines from regress.in and run uri-regress on them
+while () {
+ chomp;
+ print "trying $_\n";
+ system("./uri-regress \"$_\"");
+ print "\n";
+}
+
+# restore STDOUT/ERR so we can print the outcome to the user
+open(STDERR, ">&OLDERR") or die; # can't complain as STDERR is still duped
+open(STDOUT, ">&OLDOUT") or die "Can't restore STDOUT: $!";
+
+# just in case
+close REGRESS_IN;
+
+my $diff_status = system("diff -c \"$srcdir/$subdir/expected.out\" regress.out >regress.diff");
+if ($diff_status == 0) {
+ print <regress.out 2>&1
-
-if diff -c "${SRCDIR}/${SUBDIR}/"expected.out regress.out >regress.diff; then
- echo ""
- echo "All tests passed"
- exit 0
-else
- echo ""
- echo "FAILED: the test result differs from the expected output"
- echo
- echo "Review the difference in ${SUBDIR}/regress.diff"
- echo ""
- exit 1
-fi
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index f0fad43..e65971c 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -229,6 +229,15 @@ sub mkvcbuild
$libpq->ReplaceFile('src\interfaces\libpq\libpqrc.c','src\interfaces\libpq\libpq.rc');
$libpq->AddReference($libpgport);
+my $libpq_uri_regress = $solution->AddProject('libpq_uri_regress','exe','misc');
+$libpq_uri_regress->AddFile('src\interfaces\libpq\test\uri-regress.c');
+$libpq_uri_regress->AddIncludeDir('src\port');
+$libpq_uri_regress->AddIncludeDir('src\interfaces\libpq');
+$libpq_uri_regress->AddLibrary('wsock32.lib');
+$libpq_uri_regress->AddDefine('HOST_TUPLE="i686-pc-win32vc"');
+$libpq_uri_regress->AddDefine('FRONTEND');
+$libpq_uri_regress->AddReference($libpq,$libpgport);
+
my $libpqwalreceiver = $solution->AddProject('libpqwalreceiver', 'dll', '',
'src\backend\replication\libpqwalreceiver');
$libpqwalreceiver->AddIncludeDir('src\interfaces\libpq');
--
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] libpq URL syntax vs SQLAlchemy
Peter Eisentraut writes: > I have been reviewing how our new libpq URL syntax compares against > existing implementations of URL syntaxes in other drivers or > higher-level access libraries. In the case of SQLAlchemy, there is an > incompatibility regarding how Unix-domain sockets are specified. > > First, here is the documentation on that: > http://docs.sqlalchemy.org/en/latest/dialects/postgresql.html > > The recommended way to access a server over a Unix-domain socket is to > leave off the host, as in: > > postgresql://user:password@/dbname > > In libpq, this is parsed as host='/dbname', no database. Ah, good catch: thanks for heads up. I believe this was introduced lately in the dev cycle when we've noticed that users will have to specify some defaults explicitly to be able to override other defaults, while avoiding the whole "?keyword=value&..." business. I'll give this another look and will get back with a proposal to fix this in form of a patch. -- Regards, Alex -- 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] libpq URL syntax vs SQLAlchemy
karave...@mail.bg writes: > - Цитат от Alex Shulgin (a...@commandprompt.com), на 14.05.2012 в 18:16 > - > >> Alex writes: >> >> >> The host part in this case is empty (it is "hidden" between the "//" and >> the following "/",) thus local socket connection is employed for this >> type of URIs. To specify non-standard path to the local sockets >> directory use the familiar URI parameter: >> >> postgres:///db?host=/path/to/socket/dir >> > > And why are we calling "host" the parameter that specifies the path for socket > dir - it is not host and could be confused with the host part of the URI > (the > part between // and /). Why do not call it "path" ? So it will become: > > postgres:///db?path=/path/to/socket/dir We call it that way since we rely on existing libpq code to interpret the value of every parameter in the URI (well, almost: with notable exception of translating "ssl=true" for JDBC compatibility.) I don't think anyone would confuse host part of the URI with URI parameter "?host=..." if we care to express things clearly in the documentation (which we do I believe.) Existing implementations, like that mentioned by Peter in the top message of this thread (SQLAlchemy or was it psycopg2?) already use this notation, so I don't think we can or should do anything about this, i.e. there's little point in renaming to "path" or merely supporting it as an alternative syntax. -- Alex -- 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] libpq URL syntax vs SQLAlchemy
Alex Shulgin writes: > > Upon closer inspection of the issue I came to believe that the proper > fix is to drop support for special treatment of "host part" starting > with slash altogether. > > Attached is a patch to do that. Well, I understand I might be asking for too much, but did anyone had a chance to look at the correcting patch? We're having the first CommitFest of 9.3 in three weeks, so this better be dealt with before it's too late. I believe the correcting patch makes our implementation comply to RFC 3986. I can produce a patch against 9.1 for improved readability: the removal of special handling of '/' at the start of URI created a bit of mess in the correcting patch, so it might be easier to look at the combined effect of the committed and this one. Comments please? -- Regards, Alex -- 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] libpq URL syntax vs SQLAlchemy
Peter Eisentraut writes: > On mån, 2012-05-14 at 18:16 +0300, Alex Shulgin wrote: >> Upon closer inspection of the issue I came to believe that the proper >> fix is to drop support for special treatment of "host part" starting >> with slash altogether. >> >> Attached is a patch to do that. > > Committed. Many thanks! > I also updated the documentation and tests to show that percent-encoding > a host part starting with slash also works, as discussed upthread. Yes, that's a side-effect, but still might be useful to know. -- Regards, Alex -- 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] Inconsistency in libpq connection parameters, and extension thereof
Daniel Farina writes: > On Tue, Jun 5, 2012 at 6:43 PM, Tom Lane wrote: >> >> Um. We oughta fix that. I'm not necessarily wedded to the old >> throw-an-error definition, but there seems no good reason for these >> two syntaxes to act inconsistently. > > I agree with that. The URIs may have been done this way as a > concession to some small fragmentation that may have taken place > before URIs were standardized, but perhaps the author can speak to > that (he has been put on the To: list for this mail). Sorry for the silence. The original intent was to not error out on any extra parameters from JDBC or other existing URI implementations. The example of a possible typo in sslmode=require clearly demonstrates that this was not a well-thought decision. Anyway, I can see you've already sorted this out. -- Alex -- 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] Listen / Notify rewrite
On Thu, 12 Nov 2009 11:22:32 -0500 Andrew Chernow wrote: > > > However I share Greg's concerns that people are trying to use NOTIFY > > as a message queue which it is not designed to be. > > When you have an established libpq connection waiting for notifies it > is not unreasonable to expect/desire a payload. ISTM, the problem is > that the initial design was half-baked. NOTIFY is event-driven, ie. > no polling! > I agree. Wouldn't it make sense to allow the user to pass libpq a callback function which is executed when NOTIFY events happen? Currently we are forced to poll the connection, which means that we'll be checking for a NOTIFY every time we have new data. That just doesn't make sense. -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Something wrong after file system level backup
The situation is like that: [19:24] Who can help with file system level backup? [19:24] Press_Enter: shut down postgres, tar the data dir [19:25] Press_Enter: apparently filesystem snapshots might work as well [19:25] Press_Enter: you're supposed to use pg_dumpall [19:25] KL, my question is some deeper =) I`ve backup /var/lib/pgsql/data, then reinstall Postres (the same version) and then copy all data back. But psql dosn`t see any tables [19:26] Press_Enter: exact same version? [19:26] KL, yes [19:26] Press_Enter: did you stop postgres while you backed it up? [19:26] KL, yes [19:27] Press_Enter: then there shouldn't be any problem... [19:27] Press_Enter: try select * from information_schema.tables; [19:29] KL, ERROR: Namespace "information_schema" does not exist [19:29] Press_Enter: what pgsql version? [19:30] KL, 7.3.4 [19:30] Press_Enter: ah sorry [19:30] Press_Enter: try select * from pg_tables; (or pg_table) [19:28] ERROR: Namespace "information_schema" does not exist [19:28] ERROR: Namespace "information_schema" does not exist [19:33] only rows from schemaname "pg_catalog" (29 rows) [19:33] hmm [19:33] are you sure you're in the right db? [19:34] because if something was wrong with your backup i wouldn't expect ANYTHING to work [19:34] not end up with a fresh clean database [19:34] you didn't re-run initdb did you? [19:34] After installation i only copy all the /var/lib/pgsql/data [19:34] And didn`t did initdb [19:35] it's a mystery to me [19:35] did you install from packages? [19:35] yes [19:36] mayeb the packages did their own initdb somewhere [19:36] in /var/lib/pgsql/data i see manually all the data, but psql doesn`t [19:36] and then when you ran pgsql it used a fresh database INSTEAD of looking at your restored dir? [19:36] ie. pgsql isn't even using /var/lib/pgsql/data [19:36] you can test that by stopping postgresql [19:36] then running "postmaster -D /var/lib/pgsql/data" [19:37] then in another window see if you can connect [19:37] then just ctrl-c the postmaster when you're done [19:39] [19:37] then in another window see if you can connect - yes, i can connect, but it`s the same situation [19:39] i have no idea then [19:39] you'll have to email the developers list [19:39] pgsql-hackers@postgresql.org [19:40] if you subscribe first from the pgsql website then you won't have to wait for your post to be approved [19:40] ok, thx If in two words - i have file system level backup and i can see manually all the data in it, but PostreSQL doesn`t. What can i do? ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[HACKERS] DBD::PgSPI 0.02
Hello, A short note that I've updated DBD::PgSPI version 0.02 to CPAN. There are no new features - but the code now expects (and works with) reasonably decent versions of perl (5.8.x) and pgsql (8.x). No warranty is given, this code compiles and 'scratches my itch'. If it happens to scratch yours, more the merrier. -alex ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] DBD::PgSPI 0.02
On Sun, 5 Dec 2004, Michael Fuhr wrote: > Using PostgreSQL 8.0.0rc1 and Perl 5.8.6 on FreeBSD 4.10-STABLE and > Solaris 9, I had to make a couple of changes to get DBD::PgSPI to > build: > > 1. Add -I$POSTGRES_HOME/include/server to Makefile.PL. Otherwise > the build fails with: You should point POSTGRES_HOME to "src" directory of your pgsql tree. > > In file included from PgSPI.xs:14: > PgSPI.h:16:22: postgres.h: No such file or directory > PgSPI.h:17:21: funcapi.h: No such file or directory > PgSPI.h:18:26: executor/spi.h: No such file or directory > > 2. Remove or comment out #include "ppport.h" from PgSPI.h. Neither of > my systems have this file and the module builds without it. Strange that 'make tardist' didn't include it. I'm not sure if its even required or not to have backward compatibility (to perl 5.4 for example) or not. Or whether I even care about backward compatibility. I'll remove it in next release, I suppose. -alex ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] DBD::PgSPI 0.02
On Sun, 5 Dec 2004, Michael Fuhr wrote: > On Mon, Dec 06, 2004 at 01:38:27AM -0500, [EMAIL PROTECTED] wrote: > > On Sun, 5 Dec 2004, Michael Fuhr wrote: > > > > > Using PostgreSQL 8.0.0rc1 and Perl 5.8.6 on FreeBSD 4.10-STABLE and > > > Solaris 9, I had to make a couple of changes to get DBD::PgSPI to > > > build: > > > > > > 1. Add -I$POSTGRES_HOME/include/server to Makefile.PL. Otherwise > > > the build fails with: > > > > You should point POSTGRES_HOME to "src" directory of your pgsql tree. > > Why should the module need the PostgreSQL source code? It builds > fine using the headers under the PostgreSQL install directory (e.g., > /usr/local/pgsql/include), at least with 8.0.0rc1. I don't think the headers I need (postgres.h funcapi.h) are installed in a normal install. At least they aren't on my machine. Intrinsically, this module needs to know a little bit more about pgsql internals (such as TupleDesc definition) than just something that uses libpq... If I'm wrong, please correct me. -alex ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] DBD::PgSPI 0.02
On Sun, 5 Dec 2004, Michael Fuhr wrote: > 1. Add -I$POSTGRES_HOME/include/server to Makefile.PL. Otherwise the > build fails with: On second thought: Apparently that if I do 'make install-all-headers' I would get the files I need in include/server. I didn't know this - make install won't install by default. I'll add proper path to makefile for next release (sooner than 3 years this time ;) -alex ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Mike Rylander wrote: > Just so that you have some info, I've been using DBD::PgSPI with Pg 8.0 > since beta 1. The only restriction I've run into with the old code is > that it doesn't like the DBD 'do' method. I have to use execute/fetchX > or selectX, but other than that it seems to work. I'll be grabbing the > update to test soon. 'do' seems to work for me. What happens for you? I put a version of code with a bit more fixes from comments onlist to www.pilosoft.com/PgSPI/DBD-PgSPI-0.03pre.tar.gz Please download and try it. -alex ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Mike Rylander wrote: > With v. 0.01 the statement just doesn't seem to execute. I know that's > odd, because it's supposed to be doing a prepare/execute internally, but > nothing happens. Wierd - the testsuite (make test) has some 'dos' in the code and it works (and it checks the result of inserts that are done with do). > > I'll grab it now, but I'm not sure when I'll have time to test it. I'm > mired in Unicode work right now... I'll get back to you as soon as I've > had a chance to work with it, though. Yeah - I'm not doing any utf translation in pgspi - this might become necessary for you... > > Again, thanks for all your work on PgSPI. I'll be moving all my plperlu > functions out to a module and using PgSPI to allow me to run the code > from my middleware layer as well as in-DB. That way I only need to > write the logic once. Yep, that's kind of was the point, making postgresql itself a middleware server. Make perl stored procedures take serialized objects and return serialized objects back. -alex ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Andrew Dunstan wrote: > BTW, I would like to get these capabilities into core plperl. There is already spi_exec_query in pl/perl for quick and dirty DB access for this purpose, no? > There are some obstacles to overcome. For example: > > . not every perl installation has DBI Sure - I suppose if you don't have DBI, you would use spi_exec_query, no? > . how to turn it on for trusted plperl Eh, you don't turn it on. You install the package and it works ;) > . DBD::PgSPI is covered by GPL, which means it can't be used in the core > distribution of postgres - we'd have to reinvent it in a clean room fashion. Actually, its both GPL and Artistic license - identical to DBD::Pg (where most of the code is taken from). I don't think this needs to be in core distribution - much like DBD::Pg doesn't need to be there either... -alex ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Tom Lane wrote: > [EMAIL PROTECTED] writes: > > On Mon, 6 Dec 2004, Andrew Dunstan wrote: > >> . how to turn it on for trusted plperl > > > Eh, you don't turn it on. You install the package and it works ;) > > Really? If the plperl Safe opmask allows that, we've got some problems. Errr my bad. I keep confusing trusted/untrusted. It does not allow it, nor should it. The purpose of PgSPI is to write 'middleware' solutions in perl - the idea is that you can take a piece of existing client-side code and make a server-side stored procedure out of it in a minute without any changes to the code. For quick access from trusted code, spi_exec should just do fine. -alex ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Andrew Dunstan wrote: > I disagree. The crucial difference is that DBD::Pg is a client side > library and plperl is not. > > I would like all perl programmers to be able to use the same (or > similar) idioms on both the client side and the server side. (Just as > one can use JDBC idioms in PL/Java, iirc). So I want a DBI handle to be > available in core plperl if possible. Just like perl programmers need to download DBD::Pg to deal with Pg on client side, I don't see what's so wrong with them having to download DBD::PgSPI to deal with it on server side. Besides the fact is that they have to download DBI to get *anywhere* in the first place ;0 -alex ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Michael Fuhr wrote:
> On Mon, Dec 06, 2004 at 02:34:33PM -0500, [EMAIL PROTECTED] wrote:
> >
> > For quick access from trusted code, spi_exec should just do fine.
>
> BTW, does stock PL/Perl have functions for escaping identifiers,
> strings, and binary strings?
non-DBI? no.
DBI? yes, $pg_dbh->quote('foo')
---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [HACKERS] DBD::PgSPI 0.02
On Mon, 6 Dec 2004, Tom Lane wrote: > Sure. But you don't run your middleware as root (I hope ;-)) and you > shouldn't run it in untrusted server-side languages either. I agree Actually - I don't practically care, and in fact I'm doing things that are unsafe...But, I agree, others may think differently ;) > with Andrew that it's important to figure out how to make DBI usable > in trusted plperl. Obviously this isn't happening in time for 8.0, > but it deserves a place on the TODO list. It's interesting but its probably a untrivial effort to make DBI itself Safe-safe. Probably it would require starting with DBI::PurePerl (non-XS version) and adding a mode that would disable all unSafe activity (such as file operations etc etc)... -alex ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
[HACKERS] Explicit Transaction Priority
Hello.. I was wondering if it's possible to make postgresql backend to adjust his priority based by an parameter to begin.. i.e: For linux , a "BEGIN -20" to renice backend priority to -20.. This should be easy (i hope) to implement using setpriority(..) , but there are a few questions. 1) How portable is it ? 2) What implications does it have on the postgres backend ? This could be useful in some situations (like logtables , where I don't need instant inserts or updates..) Thanks Alex
Re: [HACKERS] empty array case in plperl_ref_from_pg_array not handled correctly
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&dt=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] Sanity checking for ./configure options?
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Looks good to me. It only allows valid number between 1 and 65535, disallows leading zero, empty string, or non-digit chars. Error messages looks good. Marking this Ready for Committer. -- Alex The new status of this patch is: Ready for Committer -- 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] Block level parallel vacuum WIP
On 23.08.2016 15:41, Michael Paquier wrote: On Tue, Aug 23, 2016 at 8:02 PM, Masahiko Sawada wrote: As for PoC, I implemented parallel vacuum so that each worker processes both 1 and 2 phases for particular block range. Suppose we vacuum 1000 blocks table with 4 workers, each worker processes 250 consecutive blocks in phase 1 and then reclaims dead tuples from heap and indexes (phase 2). So each worker is assigned a range of blocks, and processes them in parallel? This does not sound performance-wise. I recall Robert and Amit emails on the matter for sequential scan that this would suck performance out particularly for rotating disks. Rotating disks is not a problem - you can always raid them and etc. 8k allocation per relation once per half an hour that is the problem. Seq scan is this way = random scan... Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Parallel sec scan in plpgsql
Hello! Does parallel secscan works in plpgsql? -- Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- 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] Parallel sec scan in plpgsql
Aggregate (cost=289035.43..289035.44 rows=1 width=8) -> HashAggregate (cost=288697.59..288847.74 rows=15015 width=28) Group Key: test.a, test.b, test.c, test.d, test.e -> Seq Scan on test (cost=0.00..163696.15 rows=1115 width=20) So as we can see parallel secscan doesn't works in plpgsql and sql functions. Can somebody explains me where I was wrong? Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company On 16.09.2016 07:27, Ashutosh Bapat wrote: On Thu, Sep 15, 2016 at 9:15 PM, Alex Ignatov wrote: Hello! Does parallel secscan works in plpgsql? Parallel seq scan is a query optimization that will work independent of the source of the query - i.e whether it comes directly from a client or a procedural language like plpgsql. So, I guess, answer to your question is yes. If you are expecting something else, more context will help. -- 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] Parallel sec scan in plpgsql
On 16.09.2016 16:50, Amit Kapila wrote:
On Fri, Sep 16, 2016 at 6:57 PM, Alex Ignatov wrote:
No it doesn't.
Paralleling neither sql function nor plpgsql:
Here is example :
ipdr=> show max_worker_processes ;
max_worker_processes
--
128
(1 row)
ipdr=> set max_parallel_workers_per_gather to 128;
SET
ipdr=> set force_parallel_mode=on;
SET
ipdr=> set min_parallel_relation_size =0;
SET
ipdr=> set parallel_tuple_cost=0;
SET
Can you try by setting force_parallel_mode = off;? I think it is
sending the whole function execution to worker due to
force_parallel_mode.
No changes:
ipdr=> set max_parallel_workers_per_gather to 128;
SET
ipdr=> set min_parallel_relation_size =0;
SET
ipdr=> set parallel_tuple_cost=0;
SET
ipdr=> set force_parallel_mode = off;
SET
ipdr=> select name,setting from pg_settings where name
in('max_parallel_workers_per_gather',
ipdr(>
'min_parallel_relation_size',
ipdr(>'parallel_tuple_cost',
ipdr(>
'force_parallel_mode');
name | setting
-+-
force_parallel_mode | off
max_parallel_workers_per_gather | 128
min_parallel_relation_size | 0
parallel_tuple_cost | 0
(4 rows)
ipdr=> explain (analyze,buffers) select count(*) from (select
a,b,c,d,e,sum(bytes) from test group by a,b,c,d,e)t;
QUERY PLAN
-
Aggregate (cost=87702.33..87702.34 rows=1 width=8) (actual
time=709.643..709.643 rows=1 loops=1)
Buffers: shared hit=65015
-> Finalize HashAggregate (cost=87364.49..87514.64 rows=15015 width=28)
(actual time=706.382..708.456 rows=15015 loops=1)
Group Key: test.a, test.b, test.c, test.d, test.e
Buffers: shared hit=65015
-> Gather (cost=85149.78..85299.93 rows=165165 width=20) (actual
time=478.626..645.209 rows=180180 loops=1)
Workers Planned: 11
Workers Launched: 11
Buffers: shared hit=65015
-> Partial HashAggregate (cost=84149.78..84299.93 rows=15015
width=20) (actual time=473.890..478.309 rows=15015 loops=12)
Group Key: test.a, test.b, test.c, test.d, test.e
Buffers: shared hit=63695
-> Parallel Seq Scan on test (cost=0.00..72786.01
rows=909101 width=20) (actual time=0.021..163.120 rows=83 loops=12)
Buffers: shared hit=63695
Planning time: 0.318 ms
Execution time: 710.600 ms
(16 rows)
ipdr=> explain (analyze,buffers) select parallel_test_plpgsql();
QUERY PLAN
--
Result (cost=0.00..0.26 rows=1 width=8) (actual time=4003.719..4003.720
rows=1 loops=1)
Buffers: shared hit=63869
Planning time: 0.021 ms
Execution time: 4003.769 ms
(4 rows)
auto_explain:
2016-09-16 18:02:29 MSK [29353]: [53-1]
user=ipdr,db=ipdr,app=psql,client=[local] LOG: duration: 4001.275 ms plan:
Query Text: select count(*) from (select a,b,c,d,e,sum(bytes)
from test group by a,b,c,d,e)t
Aggregate (cost=289035.43..289035.44 rows=1 width=8)
-> HashAggregate (cost=288697.59..288847.74 rows=15015 width=28)
Group Key: test.a, test.b, test.c, test.d, test.e
-> Seq Scan on test (cost=0.00..163696.15 rows=1115
width=20)
2016-09-16 18:02:29 MSK [29353]: [54-1] user=ipdr,db=ipdr,app=psql,client=[local]
CONTEXT: SQL statement "select count(*) from (select a,b,c,d,e,sum(bytes)
from test group by a,b,c,d,e)t"
PL/pgSQL function parallel_test_plpgsql() line 5 at SQL statement
ipdr=> explain (analyze,buffers) select parallel_test_plpgsql();
QUERY PLAN
--
Result (cost=0.00..0.26 rows=1 width=8) (actual time=4497.820..4497.822
rows=1 loops=1)
Buffers: shared hit=63695
Planning time: 0.023 ms
Execution time: 4497.872 ms
(4 rows)
auto_explain:
2016-09-16 18:03:23 MSK [29353]: [57-1]
user=ipdr,db=ipdr,app=psql,client=[local] LOG: duration: 4497.050 ms plan:
Query Text: select count(*) from (select a,b,c,d,e,sum(bytes)
from test group by a,b,c,d,e)t
Aggregate (cost=289035.43..289035.44 rows=1 width=8)
-> HashAggregate (cost=288697.59..288847.74 rows=15015 width=28)
Group Key: test.a, test.b, test.c, test.d, test.e
Re: [HACKERS] Parallel sec scan in plpgsql
On 18.09.2016 06:54, Amit Kapila wrote:
On Fri, Sep 16, 2016 at 8:48 PM, Alex Ignatov wrote:
On 16.09.2016 16:50, Amit Kapila wrote:
Can you try by setting force_parallel_mode = off;? I think it is
sending the whole function execution to worker due to
force_parallel_mode.
No changes:
Okay, it just skipped from my mind that we don't support parallel
queries for SQL statement execution (or statements executed via
exec_stmt_execsql) from plpgsql. For detailed explanation of why that
is not feasible you can refer one of my earlier e-mails [1] on similar
topic. I think if we can somehow get the results via Perform
statement, then it could be possible to use parallelism via plpgsql.
However, you can use it via SQL functions, an example is below:
set min_parallel_relation_size =0;
set parallel_tuple_cost=0;
set parallel_setup_cost=0;
Load 'auto_explain';
set auto_explain.log_min_duration = 0;
set auto_explain.log_analyze = true;
set auto_explain.log_nested_statements = true;
create table test_plpgsql(c1 int, c2 char(1000));
insert into test_plpgsql values(generate_series(1,10),'aaa');
create or replace function parallel_test_set_sql() returns
setof bigint as $$
select count(*) from test_plpgsql;
$$language sql PARALLEL SAFE STRICT STABLE;
Then execute function as: select * from parallel_test_set_sql(); You
can see below plan if auto_explain module is loaded.
Finalize Aggregate (cost=14806.85..14806.86 rows=1 width=8) (actual tim
e=1094.966..1094.967 rows=1 loops=1)
-> Gather (cost=14806.83..14806.84 rows=2 width=8) (actual time=472.
216..1094.943 rows=3 loops=1)
Workers Planned: 2
Workers Launched: 2
-> Partial Aggregate (cost=14806.83..14806.84 rows=1 width=8)
(actual time=177.867..177.868 rows=1 loops=3)
-> Parallel Seq Scan on test_plpgsql (cost=0.00..14702.6
7 rows=41667 width=0) (actual time=0.384..142.565 rows=3 loops=3)
CONTEXT: SQL function "parallel_test_set_sql" statement 1
LOG: duration: 2965.040 ms plan:
Query Text: select * from parallel_test_set_sql();
Function Scan on parallel_test_set_sql (cost=0.25..10.25 rows=1000 widt
h=8) (actual time=2538.620..2776.955 rows=1 loops=1)
In general, I think we should support the cases as required (or
written) by you from plpgsql or sql functions. We need more work to
support such cases. There are probably two ways of supporting such
cases, we can build some intelligence in plpgsql execution such that
it can recognise such queries and allow to use parallelism or we need
to think of enabling parallelism for cases where we don't run the plan
to completion. Most of the use cases from plpgsql or sql function
fall into later category as they don't generally run the plan to
completion.
[1] -
https://www.postgresql.org/message-id/CAA4eK1K8kaO_jRk42-o2rmhSRbKV-3mR%2BiNVcONLdbcSXW5TfQ%40mail.gmail.com
Thank you for you sugestion! That works.
But what we can do with this function:
create or replace function parallel_test_sql(t int) returns setof bigint as
$$
select count(*) from (select a,b,c,d,e,sum(bytes) from test where a>= $1
group by a,b,c,d,e)t;
$$ language sql PARALLEL SAFE STRICT STABLE;
explain (analyze,buffers) select * from parallel_test_sql(2);
"Function Scan on parallel_test_sql (cost=0.25..10.25 rows=1000 width=8) (actual
time=2410.789..2410.790 rows=1 loops=1)"
" Buffers: shared hit=63696"
"Planning time: 0.082 ms"
"Execution time: 2410.841 ms"
2016-09-20 14:09:04 MSK [13037]: [75-1] user=ipdr,db=ipdr,app=pgAdmin III -
Query Tool,client=127.0.0.1 LOG: duration: 2410.135 ms plan:
Query Text:
select count(*) from (select a,b,c,d,e,sum(bytes) from test where
a>= $1 group by a,b,c,d,e)t;
Aggregate (cost=230701.42..230701.43 rows=1 width=8)
-> HashAggregate (cost=230363.59..230513.74 rows=15015 width=28)
Group Key: test.a, test.b, test.c, test.d, test.e
-> Seq Scan on test (cost=0.00..188696.44 rows=372
width=20)
Filter: (a >= $1)
No parallelism again. Looks like that Filter: (a >= $1) breaks parallelism
Alex Ignatov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
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] GSOC'17 project introduction: Parallel COPY execution with errors handling
Hi Alexander! I've missed your reply, since proposal submission deadline have passed last Monday and I didn't check hackers mailing list too frequently. (1) It seems that starting new subtransaction at step 4 is not necessary. We can just gather all error lines in one pass and at the end of input start the only one additional subtransaction with all safe-lines at once: [1, ..., k1 - 1, k1 + 1, ..., k2 - 1, k2 + 1, ...], where ki is an error line number. But assuming that the only livable use-case is when number of errors is relatively small compared to the total rows number, because if the input is in totally inconsistent format, then it seems useless to import it into the db. Thus, it is not 100% clear for me, would it be any real difference in performance, if one starts new subtransaction at step 4 or not. (2) Hmm, good question. As far as I know it is impossible to get stdin input size, thus it is impossible to distribute stdin directly to the parallel workers. The first approach which comes to the mind is to store stdin input in any kind of buffer/query and next read it in parallel by workers. The question is how it will perform in the case of large file, I guess poor, at least from the memory consumption perspective. But would parallel execution still be faster is the next question. Alexey On Thu, Apr 6, 2017 at 4:47 PM, Alexander Korotkov < a.korot...@postgrespro.ru> wrote: > Hi, Alexey! > > On Tue, Mar 28, 2017 at 1:54 AM, Alexey Kondratov < > kondratov.alek...@gmail.com> wrote: > >> Thank you for your responses and valuable comments! >> >> I have written draft proposal https://docs.google.c >> om/document/d/1Y4mc_PCvRTjLsae-_fhevYfepv4sxaqwhOo4rlxvK1c/edit >> >> It seems that COPY currently is able to return first error line and error >> type (extra or missing columns, type parse error, etc). >> Thus, the approach similar to the Stas wrote should work and, being >> optimised for a small number of error rows, should not >> affect COPY performance in such case. >> >> I will be glad to receive any critical remarks and suggestions. >> > > I've following questions about your proposal. > > 1. Suppose we have to insert N records >> 2. We create subtransaction with these N records >> 3. Error is raised on k-th line >> 4. Then, we can safely insert all lines from 1st and till (k - 1) >> > 5. Report, save to errors table or silently drop k-th line >> 6. Next, try to insert lines from (k + 1) till N with another >> subtransaction >> 7. Repeat until the end of file > > > Do you assume that we start new subtransaction in 4 since subtransaction > we started in 2 is rolled back? > > I am planning to use background worker processes for parallel COPY >> execution. Each process will receive equal piece of the input file. Since >> file is splitted by size not by lines, each worker will start import from >> the first new line to do not hit a broken line. > > > I think that situation when backend is directly reading file during COPY > is not typical. More typical case is \copy psql command. In that case > "COPY ... FROM stdin;" is actually executed while psql is streaming the > data. > How can we apply parallel COPY in this case? > > -- > Alexander Korotkov > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company >
[HACKERS] Parallel COPY FROM execution
Greetings pgsql-hackers, I am a GSOC student this year, my initial proposal has been discussed in the following thread https://www.postgresql.org/message-id/flat/7179F2FD-49CE-4093-AE14-1B26C5DFB0DA%40gmail.com Patch with COPY FROM errors handling seems to be quite finished, so I have started thinking about parallelism in COPY FROM, which is the next point in my proposal. In order to understand are there any expensive calls in COPY, which can be executed in parallel, I did a small research. First, please, find flame graph of the most expensive copy.c calls during the 'COPY FROM file' attached (copy_from.svg). It reveals, that inevitably serial operations like CopyReadLine (<15%), heap_multi_insert (~15%) take less than 50% of time in summary, while remaining operations like heap_form_tuple and multiple checks inside NextCopyFrom probably can be executed well in parallel. Second, I have compared an execution time of 'COPY FROM a single large file (~300 MB, 5000 lines)' vs. 'COPY FROM four equal parts of the original file executed in the four parallel processes'. Though it is a very rough test, it helps to obtain an overall estimation: Serial: real 0m56.571s user 0m0.005s sys 0m0.006s Parallel (x4): real 0m22.542s user 0m0.015s sys 0m0.018s Thus, it results in a ~60% performance boost per each x2 multiplication of parallel processes, which is consistent with the initial estimation. After several discussions I have two possible solutions on my mind: 1) Simple solution Let us focus only on the 'COPY FROM file', then it is relatively easy to implement, just give the same file and offset to each worker. ++ Simple; more reliable solution; probably it will give us the most possible performance boost - - Limited number of use cases. Though 'COPY FROM file' is a frequent case, even when one use it with psql \copy, client-side file read and stdin streaming to the backend are actually performed 2) True parallelism Implement a pool of bg_workers and simple shared_buffer/query. While main COPY process will read an input data and put raw lines into the query, parallel bg_workers will take lines from there and process. ++ More general solution; support of various COPY FROM use-cases - - Much more sophisticated solution; probably less performance boost compared to 1) I will be glad to any comments and criticism. Alexey -- 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] Parallel COPY FROM execution
On Fri, Jun 30, 2017 at 3:35 PM, Pavel Stehule wrote: > > > 2017-06-30 14:23 GMT+02:00 Alex K : >> >> Thus, it results in a ~60% performance boost per each x2 multiplication of >> parallel processes, which is consistent with the initial estimation. >> > > the important use case is big table with lot of indexes. Did you test > similar case? Not yet, I will try it, thank you for a suggestion. But how much is it 'big table' and 'lot of indexes' in numbers approximately? Also, index updates and constraint checks performance are what I cannot control during COPY execution, so probably I have not to care too much about that. But of course, it is interesting, how does COPY perform in that case. Alexey -- 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] Parallel COPY FROM execution
Greetings pgsql-hackers, I have completed the general infrastructure for parallel COPY FROM execution, consisting of Main (master) process and multiple BGWorkers connected with master using a personal message query (shm_mq). Master process does: - Dynamic shared memory allocation with parallel state across BGWorkers and master - Attaching every worker to the personal message query (shm_mq) - Wait workers initialization using Latch - Read raw text lines using CopyReadLine and puts them into shm_mq's via round-robin to balance queries load - When EOF is reached sends zero-length message and workers are safely shut down when receive it - Wait for worker until they complete their jobs using ConditionalVariable Each BGWorker does: - Signal master on initialization via Latch - Receive raw text lines over the personal shm_mq and put them into the log (for now) - Reinitialize db connection using the same db_id and user_id as main process - Signal master via ConditionalVariable on job done All parallel state modifications are done under LWLocks. You can find actual code here https://github.com/ololobus/postgres/pull/2/files (it is still in progress, so has a lot of duplications and comments, which are to-be-deleted) To go forward I have to overcome some obstacles: - Currently all copy.c methods are designed to work with one giant structure – CopyState. It includes buffers, many initial parameters which stay unchanged and a few variables which vary during COPY FROM execution. Since I need all these parameters, I have to obtain them somehow inside each BGWorker process. I see two possible solutions here: 1) Perform BeginCopyFrom initialization inside master and put required parameters into shared memory. However, many of them are arrays of a variable size (e.g. partition_tupconv_maps, force_notnull_flags), so I cannot put them into shmem inside one single struct. The best idea I have is to put each parameter under the personal shmem TOC key, which seems to be quite ugly. 2) Perform BeginCopyFrom initialization inside each BGWorker. However, it also opens input file/pipe for read, which is not necessary for workers and may cause some interference with master, but I can modify BeginCopyFrom. - I have used both Latch and ConditionalVariable for the same purpose–wait until some signal occurs–and for me as an end user they perform quite similar. I looked into the condition_variable.c code and it uses Latch and SpinLock under the hood. So what are differences and dis-/advantages between Latch and ConditionalVariable? I will be glad if someone will help me to find an answer to my question; also any comments and remarks to the overall COPY FROM processing architecture are very welcome. Alexey -- 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] GSOC'17 project introduction: Parallel COPY execution with errors handling
Hi pgsql-hackers, Thank you again for all these replies. I have started working under this project and learnt a lot of new stuff last month, so here are some new thoughts about ERRORS handling in COPY. I decided to stick to the same thread, since it has a neutral subject. (1) One of my mentors--Alvaro Herrera--suggested me to have a look on the UPSERT. It may be a good point to be able to achieve the same functionality as during the ON CONFLICT DO NOTHING, when COPY actually inserts tuples and errors handling is turned on. It could additionally reduce number of failed subtransactions and reduce XIDs consumption, while still ignoring some common errors like unique index violation. Adding a full support of ON CONFLICT DO NOTHING/UPDATE to COPY seems to be a large separated task and is out of the current project scope, but maybe there is a relatively simple way to somehow perform internally tuples insert with ON CONFLICT DO NOTHING? I have added Peter Geoghegan to cc, as I understand he is the major contributor of UPSERT in PostgreSQL. It would be great if he will answer this question. (2) Otherwise, I am still going to use subtransactions via BeginInternalSubTransaction and PG_TRY / PG_CATCH with ReleaseCurrentSubTransaction / RollbackAndReleaseCurrentSubTransaction. To minimize XIDs consumption I will try to insert tuples in batches and pre-validate them as much as possible (as was suggested in the thread before). Alexey
Re: [HACKERS] Why restore_command is called for existing files in pg_xlog?
On Fri, Jun 2, 2017, at 11:51 AM, Alexander Kukushkin wrote: > Hello hackers, > There is one strange and awful thing I don't understand about > restore_command: it is always being called for every single WAL > segment postgres wants to apply (even if such segment already exists > in pg_xlog) until replica start streaming from the master. The real problem this question is related to is being unable to bring a former master, demoted after a crash, online, since the WAL segments required to get it to the consistent state were not archived while it was still a master, and local segments in pg_xlog are ignored when a restore_command is defined. The other replicas wouldn't be good candidates for promotion as well, as they were way behind the master (because the last N WAL segments were not archived and streaming replication had a few seconds delay). Is this a correct list for such questions, or would it be more appropriate to ask elsewhere (i.e. pgsql-bugs?) > > If there is no restore_command in the recovery.conf - it perfectly > works, i.e. postgres replays existing wal segments and at some point > connects to the master and start streaming from it.> > When recovery_conf is there, starting of a replica could become a real > problem, especially if restore_command is slow.> > Is it possible to change this behavior somehow? First look into > pg_xlog and only if file is missing or "corrupted" call > restore_command.> > > Regards, > --- > Alexander Kukushkin Sincerely, Alex
Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept
-Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Kapila Sent: Monday, September 4, 2017 3:32 PM To: i.kartys...@postgrespro.ru Cc: pgsql-hackers Subject: Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept On Mon, Sep 4, 2017 at 4:34 PM, wrote: > Our clients complain about this issue and therefore I want to raise > the discussion and suggest several solutions to this problem: > > I. Why does PG use Fatal when Error is enough to release lock that > rises lock conflict? > "If (RecoveryConflictPending && DoingCommandRead)" > > II. Do we really need to truncate the table on hot standby exactly at > the same time when truncate on master occurs? > > In my case conflict happens when the autovacuum truncates table tbl1 > on master while backend on replica is performing a long transaction > involving the same table tbl1. This happens because truncate takes an > AccessExclusiveLock. To tackle this issue we have several options: > > 1. We can postpone the truncate on the master until all the replicas > have finished their transactions (in this case, feedback requests to > the master should be sent frequently) Patch 1 > vacuum_lazy_truncate.patch > > 2. Maybe there is an option somehow not to send AccessExclusiveLock > and not to truncate table on the replica right away. We could try to > wait a little and truncate tbl1 on replica again. > Can max_standby_streaming_delay help in this situation (point number - 2)? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com Hello! In this situation this parameter (max_standby_streaming_delay) wont help because if you have subsequent statement on standby (following info is from documentation and from our experience ): Thus, if one query has resulted in significant delay, subsequent conflicting queries will have much less grace time until the standby server has caught up again. And you never now how to set this parameter exept to -1 which mean up to infinity delayed standby. On our experience only autovacuum on master took AccesExclusiveLock that raise this Fatal message on standby. After this AccessExclusive reached standby and max_standby_streaming_delay > -1 you definitely sooner or later get this Fatal on recovery . With this patch we try to get rid of AccessEclusiveLock applied on standby while we have active statement on it. -- Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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
On Tue, Feb 25, 2014 at 6:56 AM, Sergey Burladyan 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] [BUGS] BUG #9223: plperlu result memory leak
On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera 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
On Wed, Mar 5, 2014 at 12:55 PM, Alex Hunsaker wrote: > On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera > 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
[HACKERS] Review of pg_archivecleanup -x option patch
Hello, This is my first patch review ever, so please bear with me. The patch[1] is in the context diff format and applies cleanly to current git HEAD. Documentation is updated by the patch. The code does implement what's the patch is supposed to do. Do we want that? According to Greg's original mail, yes. One problem I've found while trying the example workflow is this: ~/local/postgresql/HEAD$ ./bin/pg_archivecleanup -d -x .gz data/archive/ 00010002.0020.backup.gz pg_archivecleanup: invalid filename input Try "pg_archivecleanup --help" for more information. I think we could accept the suffixed WAL filename and strip the suffix; another idea is to actually detect the suffix (if any) from the last command line argument, thus rendering the -x option unnecessary. By the way, I for one would use the word 'suffix' instead of 'extension' which sounds like some MS-DOS thingy, but after briefly grepping the docs I can see that both words are used in this context already (and the ratio is not in favor of my choice of wording.) Other than that the patch looks good. -- Alex [1] http://archives.postgresql.org/pgsql-hackers/2011-02/msg00547.php -- 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
On Thu, Feb 9, 2012 at 03:21, Christoph Berg 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: Add new keywords SNAPSHOT and TYPES to the keyword list in gram.
On Thu, Feb 9, 2012 at 11:30, Tom Lane wrote:
> Alvaro Herrera 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 () {
$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] WIP: URI connection string support for libpq
Greg Smith writes: Thank you for the review, Greg! > Given that, there really isn't a useful path forward that helps out > all those developers without supporting both prefixes. That's where > this left off before, I just wanted to emphasize how clear that need > seems now. OK, I've used the code from your earlier review to support the short prefix. I sincerely hope we don't make the situation any worse by being flexible about the prefix... > Next thing, also mentioned at that Flask page. SQLite has > standardized the idea that sqlite:absolute/path/to/foo.db is a URI > pointing to a file. Given that, I wonder if Alex's syntax for > specifying a socket file name might adopt that syntax, rather than > requiring the hex encoding: postgresql://%2Fvar%2Fpgsql%2Ftmp/mydb > It's not a big deal, but it would smooth another rough edge toward > making the Postgres URI implementation look as close as possible to > others. Yeah, this is really appealing, however how do you tell if the part after the last slash is a socket directory name or a dbname? E.g: psql postgres:///path/to/different/socket/dir(default dbname) psql postgres:///path/to/different/socket/dir/other (dbname=other ?) If we treat the whole URI string as the path to the socket dir (which I find the most intuitive way to do it,) the only way to specify a non-default dbname is to use query parameter: psql postgres:///path/to/different/socket/dir?dbname=other or pass another -d flag to psql *after* the URI: psql [-d] postgres:///path/to/different/socket/dir -d other Reasonable? > So far I've found only one syntax that I expected this to handle that > it rejects: > > psql -d postgresql://gsmith@localhost > > It's picky about needing that third slash, but that shouldn't be hard > to fix. Yeah, good that you've spotted it. If my reading of the URI RFC (2396) is correct, the question mark and query parameters may follow the hostname, w/o that slash too, like this: psql -d postgresql://localhost?user=gsmith So this made me relax some checks and rewrite the code a bit. > I started collecting up all the variants that do work as an initial > shell script regression test, so that changes don't break something > that already works. Here are all the variations that already work, > setup so that a series of "1" outputs is passing: > [snip] Yes, the original code was just a bit too picky about URI component separators. Attached also is a simplified test shell script. I have also added a warning message for when a query parameter is not recognized and being ignored. Not sure if plain fprintf to stderr is accepted practice for libpq, please correct if you have better idea. -- Regards, Alex *** a/src/interfaces/libpq/fe-connect.c --- b/src/interfaces/libpq/fe-connect.c *** *** 282,287 static const PQEnvironmentOption EnvironmentOptions[] = --- 282,290 } }; + /* The connection URI must start with one the following designators: */ + static const char uri_designator[] = "postgresql://"; + static const char short_uri_designator[] = "postgres://"; static bool connectOptions1(PGconn *conn, const char *conninfo); static bool connectOptions2(PGconn *conn); *** *** 297,303 static PQconninfoOption *conninfo_parse(const char *conninfo, static PQconninfoOption *conninfo_array_parse(const char *const * keywords, const char *const * values, PQExpBuffer errorMessage, bool use_defaults, int expand_dbname); ! static char *conninfo_getval(PQconninfoOption *connOptions, const char *keyword); static void defaultNoticeReceiver(void *arg, const PGresult *res); static void defaultNoticeProcessor(void *arg, const char *message); --- 300,325 static PQconninfoOption *conninfo_array_parse(const char *const * keywords, const char *const * values, PQExpBuffer errorMessage, bool use_defaults, int expand_dbname); ! static PQconninfoOption *conninfo_uri_parse(const char *uri, ! PQExpBuffer errorMessage); ! static bool conninfo_uri_parse_options(PQconninfoOption *options, ! const char *uri, char *buf, ! PQExpBuffer errorMessage); ! static bool conninfo_uri_parse_params(char *params, ! PQconninfoOption *connOptions, ! PQExpBuffer errorMessage); ! static char *conninfo_uri_decode(const char *str, PQExpBuffer errorMessage); ! static bool get_hexdigit(char digit, int *value); ! static const char *conninfo_getval(PQconninfoOption *connOptions, ! const char *keyword); ! static PQconninfoOption *conninfo_storeval(PQconninfoOption *connOptions, ! const char *keyword, const char *value, ! PQExpBuffer errorMessage, bool ignoreMissing); ! static PQconninfoOption *conninfo_store_uri_encoded_value( ! PQconninfoOption *connOptions, ! const char *keyword, const char *encoded_
Re: [HACKERS] WIP: URI connection string support for libpq
Florian Weimer writes: > * Alex Shulgin: > >> Yeah, this is really appealing, however how do you tell if the part >> after the last slash is a socket directory name or a dbname? E.g: >> >> psql postgres:///path/to/different/socket/dir(default dbname) >> psql postgres:///path/to/different/socket/dir/other (dbname=other ?) > > The HTTP precent is to probe the file system until you find something. > Most HTTP servers have something similar to the PATH_INFO variable which > captures trailing path segments. > > It's ugly, but it's standard practice, and seems better than a separate > -d parameter (which sort of defeats the purpose of URIs). Hm, do you see anything what's wrong with "?dbname=other" if you don't like a separate -d? -- Alex -- 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] Reviewing patch "URI connection string support for libpq"
Harold Giménez writes: >I have interest in the URI connection string support patch[1], so I'm >in the process of reviewing it. I have a couple of comments and >questions: Hello, thank you for your interest and the review! >1. I see no tests in the patch. I'd like to start getting together a >set of tests, likely based on the connection string permutations found >on Greg Smith's response[2]. However I don't find an obvious place to >put them. They could possibly live in the test/examples directory. >Another thought is to use dblink in a test, although it may be >problematic to depend on a contrib package for a test, to say the >least. Any thoughts on how to test this are most welcome. I was also curious on how to add any sort of regression testing (likely using psql,) but didn't get any advice as far as I can recall. >2. The documentation/manual was not updated as part of this patch, so >this is pending. I've marked the patch as Work-In-Progress and specifically omitted documentation changes until we settle on functionality. >3. I for one do prefer the `postgres` prefix, as opposed to >`postgresql` for the reasons stated on an earlier thread [3]. In my >opinion the best way to move forward is to support them both. The updated v4 version of the patch does cover this: http://archives.postgresql.org/pgsql-hackers/2012-02/msg01141.php -- Alex -- 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] WIP: URI connection string support for libpq
Peter Eisentraut writes:
>> I've included a (separate) test shell script based on Greg's cases in
>> one of the updates. What would be preferred place to plug it in?
>> Override installcheck in libpq Makefile?
>
> I think that would be the right place.
I figured that adding this right into src/interfaces/libpq is polluting
the source dir, so I've used src/test instead.
Attached v6 adds src/test/uri directory complete with the test script,
expected output file and a Makefile which responds to installcheck.
README file also included.
--
Alex
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 282,287 static const PQEnvironmentOption EnvironmentOptions[] =
--- 282,290
}
};
+ /* The connection URI must start with either of the following designators: */
+ static const char uri_designator[] = "postgresql://";
+ static const char short_uri_designator[] = "postgres://";
static bool connectOptions1(PGconn *conn, const char *conninfo);
static bool connectOptions2(PGconn *conn);
***
*** 297,303 static PQconninfoOption *conninfo_parse(const char *conninfo,
static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
const char *const * values,
PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
! static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
static void defaultNoticeProcessor(void *arg, const char *message);
--- 300,333
static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
const char *const * values,
PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
! static PQconninfoOption *conninfo_uri_parse(const char *uri,
! PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_options(PQconninfoOption *options,
! const char *uri, char *buf,
! PQExpBuffer errorMessage);
! static char *conninfo_uri_parse_local_socket_dir(PQconninfoOption *options,
! const char *uri,
! char *buf, char *lastc,
! PQExpBuffer errorMessage);
! static char *conninfo_uri_parse_remote_host(PQconninfoOption *options,
! const char *uri,
! char *buf, char *lastc,
! PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_params(char *params,
! PQconninfoOption *connOptions,
! PQExpBuffer errorMessage);
! static char *conninfo_uri_decode(const char *str, PQExpBuffer errorMessage);
! static bool get_hexdigit(char digit, int *value);
! static const char *conninfo_getval(PQconninfoOption *connOptions,
! const char *keyword);
! static PQconninfoOption *conninfo_storeval(PQconninfoOption *connOptions,
! const char *keyword, const char *value,
! PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_store_uri_encoded_value(
! PQconninfoOption *connOptions,
! const char *keyword, const char *encoded_value,
! PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_find(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
static void defaultNoticeProcessor(void *arg, const char *message);
***
*** 580,586 PQconnectStart(const char *conninfo)
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
! char *tmp;
/*
* Move option values into conn structure
--- 610,616
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
! const char *tmp;
/*
* Move option values into conn structure
***
*** 3741,3747 ldapServiceLookup(const char *purl, PQconninfoOption
*options,
static int
parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
{
! char *service = conninfo_getval(options, "service");
charserviceFile[MAXPGPATH];
char *env;
boolgroup_found = false;
--- 3771,3777
static int
parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
{
! const char *service = conninfo_getval(options, "service");
Re: [HACKERS] Another review of URI for libpq, v7 submission
Daniel Farina writes: > I reviewed this and so far have not found any serious problems, > although as is par for the course it contains some of the fiddly bits > involved in any string manipulations in C. I made a few edits -- none > strictly necessary for correctness -- that the original author is free > audit and/or include[0]. Thank you for the review, Daniel! Apparently, I was on drugs when I've submitted v7, as it still contains the bug for which to fix I was forced to move parts of the code back into the main parser routine... > I did put in some defensive programming choices (instead of if/else > if/elseif/else raise an error, even if the latter is allegedly > impossible) that I think are a good idea. Yes, this is a good idea, I'll incorporate them in the patch. However, this one doesn't work: https://github.com/fdr/postgres/commit/4fad90fb243d9266b1003cfbcf8397f67269fad3 Neither '@' or '/' are mandatory in the URI anywhere after "scheme://", so we can't just say it "should never happen." Running the regression test with this commit applied highlights the problem. > One thing I found puzzling was that in the latest revision the tests > appeared to be broken for me: all "@" signs were translated to "(at)". > Is that mangling applied by the archives, or something? This is definitely mangling by the lists. I can see this has happened to people before, e.g.: http://archives.postgresql.org/pgsql-hackers/2010-01/msg00938.php But that discussion didn't seem to lead to a solution for the problem. I wonder if there's any evidence as to that mangling the email addresses helps to reduce spam at all? I mean replacing "(at)" back to "@" and "(dot)" to "." is piece of cake for a spam crawler. What I see though, is that most of the message-id URLs are broken by the mangling. Also I don't think everyone should just tolerate that it messes with the attachments. Should we all suddenly use application/octet-stream or application/gzip instead of text/plain? > The test suite neatly tries to copy pg_regress's general "make > installcheck" style, but it likes to use my username as the database > rather than the standard "regression" as seen by pg_regress. It is > nice that a place to test connection strings and such is there, where > there was none before. Oh, I don't see why we can't use "regression" too. While testing this I've also noticed that there was some funny behavior when you left out the final slash after hostname, e.g.: "postgres://localhost/" vs. "postgres://localhost". It turned out in the former case we were setting dbname conninfo parameter to an empty string and in the latter one we didn't set it at all. The difference is that when we do set it, the default dbname is derived from username, but when we don't--$PGDATABASE is used. I've fixed this, so now both URIs work the same way (both do not set dbname.) It also appeared to me that with the current code, a wide range of funny-looking URIs are considered valid, e.g.: postgres://user@ postgres://@host postgres://host:/ and, taking approach to the extreme: postgres://:@: This specifies empty user, password, host and port, and looks really funny. I've added (actually revived) some checks to forbid setting empty URI parts where it doesn't make much sense. Finally, attached is v8. Hopefully I didn't mess things up too much. -- Regards, Alex binaKreQKSDSW.bin Description: libpq-uri-v8.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Trivial libpq refactoring patch (was: Re: [HACKERS] Another review of URI for libpq, v7 submission)
Alex writes:
> Marko Kreen writes:
>
>> On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
>>> https://github.com/a1exsh/postgres/commits/uri
>>
>> The point of the patch is to have one string with all connection options,
>> in standard format, yes? So why does not this work:
>>
>> db = PQconnectdb("postgres://localhost");
>>
>> ?
>
> Good catch.
>
> I've figured out that we'll need a bit more intrusive change than simply
> overriding the expand_dbname check in conninfo_array_parse (like the
> current version does) to support URIs in all PQconnect* variants.
>
> I still need to figure out some details, but this is to give people a
> status update.
While working on this fix I've figured out that I need my
conninfo_uri_parse to support use_defaults parameter, like
conninfo(_array)_parse functions do.
The problem is that the block of code which does the defaults handling
is duplicated in both of the above functions. What I'd like to do is
extract it into a separate function to call. What I wouldn't like is
bloating the original URI patch with this unrelated change.
So here's a trivial patch to do the refactoring. Also, it uses the
newly added conninfo_fill_defaults directly in PQconndefaults, instead
of doing the "parse empty conninfo string" trick. If this could be
applied, I'd rebase my patch against the updated master branch and
submit the new version.
As it goes, the patch adds a little duplication when it comes to
creating a working copy of PQconninfoOptions array. Attached is the
second patch on top of the first one to extract this bit of code into
conninfo_init. Not sure if we should go all the way through this, so
it's not critical if this one is not applied.
--
Regards,
Alex
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 297,302 static PQconninfoOption *conninfo_parse(const char *conninfo,
--- 297,304
static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
const char *const * values,
PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
+ static bool conninfo_fill_defaults(PQconninfoOption *options,
+ PQExpBuffer errorMessage);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
***
*** 836,842 PQconndefaults(void)
initPQExpBuffer(&errorBuf);
if (PQExpBufferDataBroken(errorBuf))
return NULL;/* out of memory already :-( */
! connOptions = conninfo_parse("", &errorBuf, true);
termPQExpBuffer(&errorBuf);
return connOptions;
}
--- 838,860
initPQExpBuffer(&errorBuf);
if (PQExpBufferDataBroken(errorBuf))
return NULL;/* out of memory already :-( */
!
! /* Make a working copy of PQconninfoOptions */
! connOptions = malloc(sizeof(PQconninfoOptions));
! if (connOptions == NULL)
! {
! printfPQExpBuffer(&errorBuf,
! libpq_gettext("out of
memory\n"));
! return NULL;
! }
! memcpy(connOptions, PQconninfoOptions, sizeof(PQconninfoOptions));
!
! if (!conninfo_fill_defaults(connOptions, &errorBuf))
! {
! PQconninfoFree(connOptions);
! connOptions = NULL;
! }
!
termPQExpBuffer(&errorBuf);
return connOptions;
}
***
*** 4002,4008 conninfo_parse(const char *conninfo, PQExpBuffer
errorMessage,
char *pname;
char *pval;
char *buf;
- char *tmp;
char *cp;
char *cp2;
PQconninfoOption *options;
--- 4020,4025
***
*** 4170,4245 conninfo_parse(const char *conninfo, PQExpBuffer
errorMessage,
free(buf);
/*
!* Stop here if caller doesn't want defaults filled in.
!*/
! if (!use_defaults)
! return options;
!
! /*
!* If there's a service spec, use it to obtain any not-explicitly-given
!* parameters.
*/
! if (parseServiceInfo(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
}
- /*
-* Get the fallback resources for parameters not specified in the
conninfo
-* string nor the service.
-*/
- for (option = options; option->keyword != NULL; option++)
- {
- if (option->val != NULL)
- contin
Re: [HACKERS] Another review of URI for libpq, v7 submission
Heikki Linnakangas writes: > On 22.03.2012 23:42, Alex wrote: >> Okay, at last here's v9, rebased against current master branch. > > Some quick comments on this patch: Heikki, thank you for taking a look at this! > I see a compiler warning: > fe-connect.c: In function ‘conninfo_parse’: > fe-connect.c:4113: warning: unused variable ‘option’ The warning is due to the earlier commit e9ce658b. I believe the above line supposed to go away. > Docs are missing. Yes, my plan is to focus on the documentation and code comments while sorting out any remaining issues with the code. > I wonder if you should get an error if you try specify the same option > multiple times. At least the behavior needs to be documented. Since conninfo strings may contain duplicated keywords and the latter just takes precedence, I think we should just do the same with URIs (which we already do.) I don't see the behavior of conninfo strings documented anywhere, however. > Should %00 be forbidden? Probably yes, good spot. > The error message is a bit confusing for > "postgres://localhost?dbname=%XXfoo": > WARNING: ignoring unrecognized URI query parameter: dbname > There is in fact nothing wrong with "dbname", it's the %XX in the > value that's bogus. Hm, yes, that's a bug. Looks like conninfo_uri_parse_params needs to be adjusted to properly pass the error message generated by conninfo_store_uri_encoded_value. I wonder if examining the errorMessage buffer to tell if it's a hard error (it's going to be empty in the case of ignoreMissing=true) is a good practice. > Looking at conninfo_uri_decode(), I think it's missing a bounds check, > and peeks at two bytes beyond the end of string if the input ends in a > %'. No, in that case what happens on L4919 is this: we dereference q which is pointing at NUL terminator and pass the value to the first get_hexdigit in the "if" condition, the pointer itself is then incremented and does point beyond the end of string, but since get_hexdigit returns FALSE we don't call the second get_hexdigit, so we don't dereference the invalid pointer. There is a comment right before that "if" which says just that. -- Alex -- 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] Trailing comma support in SELECT statements
On Tue, Oct 21, 2014 at 10:16 AM, Tom Lane wrote: > (Of course, I'm not for the feature w.r.t. SQL either. But breaking data > compatibility is just adding an entire new dimension of trouble. > Another dimension of the trouble is breaking the operation of the tools that parse SQL statements for various purposes, e.g. for dependency analysis. This is a misfeature for the benefit of edit-lazy users only. -- Alex
[HACKERS] Idle transaction cancel/timeout and SSL revisited
Hello Hackers,
After reading up through archives on the two $subj related TODO items
I'm under impression that the patches[1,2] didn't make it mainly because
of the risk of breaking SSL internals if we try to longjump out of the
signal handler in the middle of a blocking SSL read and/or if we try to
report that final "transaction/connection aborted" notice to the client.
What if instead of trying to escape from the signal handler we would set
a flag and use it to simulate EOF after the recv() is restarted due to
EINTR? From the backend perspective it should look like the client has
just hang up.
Both SSL and cleartext connections are routed through secure_raw_read,
so it seems like a good candidate for checking the flag we would set in
StatementCancelHandler(). (Should we also add the same check in
interactive_getc()?)
Ultimately, in PostgresMain() the ReadCommand() would return EOF and we
can either let the existing code shutdown the backend, or add special
handling that will only abort the transaction and report the case to the
client, which was the intent in[1]. And if that works out, the timeout
handler in[2] can simply reuse the flag.
A potential problem with this is that SSL might perform some eager
cleanup when seeing EOF, so the backend might need to reset/restart the
connection (is that possible?)
I'm attaching a patch (that doesn't compile yet :-p) in hope it can be
useful for the purpose of the discussion. Notably, secure_raw_read()
can't know about IdleTransactionCancelPending and I'm not sure what
would be the best way to let it see that (is it too much of a shortcut
anyway?)
--
Alex
[1] http://www.postgresql.org/message-id/1262268211.19367.10736.camel@ebony
[2] http://www.postgresql.org/message-id/538dc843.2070...@dalibo.com
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
new file mode 100644
index b05364c..377984d
*** a/src/backend/libpq/be-secure-openssl.c
--- b/src/backend/libpq/be-secure-openssl.c
*** rloop:
*** 543,550
errmsg("SSL error: %s", SSLerrmessage(;
/* fall through */
case SSL_ERROR_ZERO_RETURN:
! errno = ECONNRESET;
! n = -1;
break;
default:
ereport(COMMERROR,
--- 543,553
errmsg("SSL error: %s", SSLerrmessage(;
/* fall through */
case SSL_ERROR_ZERO_RETURN:
! if (!IdleTransactionCancelPending) /* HACK */
! {
! errno = ECONNRESET;
! n = -1;
! }
break;
default:
ereport(COMMERROR,
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
new file mode 100644
index 41ec1ad..44079ff
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
*** secure_raw_read(Port *port, void *ptr, s
*** 145,155
{
ssize_t n;
! prepare_for_client_read();
! n = recv(port->sock, ptr, len, 0);
! client_read_ended();
return n;
}
--- 145,160
{
ssize_t n;
! if (IdleTransactionCancelPending)
! n = 0; /* simulate EOF */
! else
! {
! prepare_for_client_read();
! n = recv(port->sock, ptr, len, 0);
! client_read_ended();
! }
return n;
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index cc62b2c..37437fc
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*** interactive_getc(void)
*** 310,318
{
int c;
! prepare_for_client_read();
! c = getc(stdin);
! client_read_ended();
return c;
}
--- 310,323
{
int c;
! if (IdleTransactionCancelPending)
! c = EOF;
! else
! {
! prepare_for_client_read();
! c = getc(stdin);
! client_read_ended();
! }
return c;
}
*** StatementCancelHandler(SIGNAL_ARGS)
*** 2629,2642
if (!proc_exit_inprogress)
{
InterruptPending = true;
! QueryCancelPending = true;
/*
* If it's safe to interrupt, and we're waiting for input or a lock,
* service the interrupt immediately
*/
if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
! CritSectionCount == 0)
{
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
--- 2634,2655
if (!proc_exit_inprogress)
{
InterruptPending = true;
!
! if (!DoingCommandRead)
! QueryCancelPending = true;
! else if (IsTransactionOrTransactionBlock())
! IdleTransactionCancelPending = true;
/*
* If it's safe to interrupt, and we're waiting for input or a lock,
* service the interrupt immediately
*/
if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
! CritSectionCount == 0 && QueryCancelPending)
! /*
! * ProcessInterrupts() does nothing in case of
! * IdleTransactionCancelPending, it is handled in PostgresMain
! */
{
/* bump holdo
Re: [HACKERS] Idle transaction cancel/timeout and SSL revisited
Andres Freund writes: > > On 2014-11-15 00:11:36 +0300, Alex Shulgin wrote: >> After reading up through archives on the two $subj related TODO items >> I'm under impression that the patches[1,2] didn't make it mainly because >> of the risk of breaking SSL internals if we try to longjump out of the >> signal handler in the middle of a blocking SSL read and/or if we try to >> report that final "transaction/connection aborted" notice to the client. > > I've written a patch that allows that - check > http://archives.postgresql.org/message-id/20140927225421.GE5423%40alap3.anarazel.de > > I feel pretty confident that that's the way to go. I just need some time > to polish it. > >> What if instead of trying to escape from the signal handler we would set >> a flag and use it to simulate EOF after the recv() is restarted due to >> EINTR? From the backend perspective it should look like the client has >> just hang up. > > That's pretty much what the above does. Except that it actually deals > with blocking syscalls by not using them and relying on latches/select > instead. Neat, I must have missed it. -- Alex -- 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] Idle transaction cancel/timeout and SSL revisited
Andres Freund writes: > On 2014-11-15 00:11:36 +0300, Alex Shulgin wrote: >> After reading up through archives on the two $subj related TODO items >> I'm under impression that the patches[1,2] didn't make it mainly because >> of the risk of breaking SSL internals if we try to longjump out of the >> signal handler in the middle of a blocking SSL read and/or if we try to >> report that final "transaction/connection aborted" notice to the client. > > I've written a patch that allows that - check > http://archives.postgresql.org/message-id/20140927225421.GE5423%40alap3.anarazel.de > > I feel pretty confident that that's the way to go. I just need some time > to polish it. > >> What if instead of trying to escape from the signal handler we would set >> a flag and use it to simulate EOF after the recv() is restarted due to >> EINTR? From the backend perspective it should look like the client has >> just hang up. > > That's pretty much what the above does. Except that it actually deals > with blocking syscalls by not using them and relying on latches/select > instead. Yay, that's nice, because my EOF approach is sort of fishy. I've checked the patches: they apply and compile on top of current HEAD (one failed hunk in pqcomm.c), so I can help with testing if needed. There is a (short) list of items that caught my attention. I will post in reply to your patches thread. -- Alex -- 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] Escaping from blocked send() reprised.
Andres Freund writes:
>
> I've invested some more time in this:
> 0002 now makes sense on its own and doesn't change anything around the
> interrupt handling. Oh, and it compiles without 0003.
In this patch, the endif appears to be misplaced in PostgresMain:
+ if (MyProcPort != NULL)
+ {
+#ifdef WIN32
+ pgwin32_noblock = true;
+#else
+ if (!pg_set_noblock(MyProcPort->sock))
+ ereport(COMMERROR,
+ (errmsg("could not set socket to
nonblocking mode: %m")));
+ }
+#endif
+
pqinitmask();
> 0003 Sinval/notify processing got simplified further. There really isn't
> any need for DisableNotifyInterrupt/DisableCatchupInterrupt
> anymore. Also begin_client_read/client_read_ended don't make much
> sense anymore. Instead introduce ProcessClientReadInterrupt (which
> wants a better name).
> There's also a very WIP
> 0004 Allows secure_read/write be interrupted when ProcDiePending is
> set. All of that happens via the latch mechanism, nothing happens
> inside signal handlers. So I do think it's quite an improvement
> over what's been discussed in this thread.
> But it (and the other approaches) do noticeably increase the
> likelihood of clients not getting the error message if the client
> isn't actually dead. The likelihood of write() being blocked
> *temporarily* due to normal bandwidth constraints is quite high
> when you consider COPY FROM and similar. Right now we'll wait till
> we can put the error message into the socket afaics.
>
> 1-3 need some serious comment work, but I think the approach is
> basically sound. I'm much, much less sure about allowing send() to be
> interrupted.
After re-reading these I don't see the rest of items I wanted to inqury
about anymore, so it just makes more sense now.
One thing I did try is sending a NOTICE to the client when in
ProcessInterrupts() and DoingCommandRead is true. I think[1] it was
expected to be delivered instantly, but actually the client (psql) only
displays it after sending the next statement.
While I'm reading on FE/BE protocol someone might want to share his
wisdom on this subject. My guess: psql blocks on readline/libedit call
and can't effectively poll the server socket before complete input from
user.
--
Alex
[1] http://www.postgresql.org/message-id/1262173040.19367.5015.camel@ebony
``AFAIK, NOTICE was suggested because it can be sent at any time,
whereas ERRORs are only associated with statements.''
--
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] Removing unreferenced files
Bruce Momjian writes: > > Here is a cleaned-up version of the unreference file patch that was > discussed extensively in May of 2005. I want to get it into the > archives in case someone else want to work on it. > > Here is a reference to the work still needed on the patch: > > http://archives.postgresql.org/pgsql-patches/2005-05/msg00024.php Hello, This is a TODO item, but I'd like to know if there's general interest in this feature at the moment. I think it can be useful in a situation when the DBA knows that the database was shutdown or crashed during extensive data load and has a way to proceed without the need to start over with the clean database. In such a case having a tool to report or remove any stale files makes perfect sense. My other concern is that if at some point there is a bug in DROP TABLE/INDEX that leaves the files it's supposed to remove, we would likely want to know it. Regardless of the outcome of this discussion it is interesting to know if such functionality can only be programmed in backend/startup or making it a separate tool is feasible (the tool will only run with the stopped server)? There was once a tool named pgfsck[1], which was never updated after 8.2, so the name is already taken... Yet another point is that one might be interested not only in reporting stale files, but any *missing* ones too. Currently, you would only know if a relation data file is missing when actually trying to read from it and no attempt is made to verify this on startup. This might be not a very useful check since if the file is not missing, but corrupted the check doesn't buy you much. (I am likely kicking a dead horse here.) Thank you. -- Alex [1] http://svana.org/kleptog/pgsql/pgfsck.html -- 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] add ssl_protocols configuration option
Dag-Erling Smørgrav writes: > > The attached patches add an ssl_protocols configuration option which > control which versions of SSL or TLS the server will use. The syntax is > similar to Apache's SSLProtocols directive, except that the list is > colon-separated instead of whitespace-separated, although that is easy > to change if it proves unpopular. Hello, Here is my review of the patch against master branch: * The patch applies and compiles cleanly, except for sgml docs: postgres.xml:66141: element varlistentry: validity error : Element varlistentry content does not follow the DTD, expecting (term+ , listitem), got (term indexterm listitem) under the tag in the material added to config.sgml by the patch. * The patch works as advertised, though the only way to verify that connections made with the protocol disabled by the GUC are indeed rejected is to edit fe-secure-openssl.c to only allow specific TLS versions. Adding configuration on the libpq side as suggested in the original discussion might help here. * The code allows specifying SSLv2 and SSLv3 in the GUC, but removes them forcibly after parsing the complete string (a warning is issued). Should we also add a note about this to the documentation? * In case the list of allowed protocols comes empty a FATAL error message is logged: "could not set the protocol list (no valid protocols available)". I think it's worth changing that to "could not set SSL protocol list..." All other related error/warning logs include "SSL". * The added code follows conventions and looks good to me. I didn't spot any problems in the parser. I've tried a good deal of different values and all seem to be parsed correctly. I would try to apply patches for older branches if there is consensus that we really need this patch and we want to back-patch it. Thanks. -- Alex -- 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] add ssl_protocols configuration option
Dag-Erling Smørgrav writes: > Alex Shulgin writes: >> * The patch works as advertised, though the only way to verify that >> connections made with the protocol disabled by the GUC are indeed >> rejected is to edit fe-secure-openssl.c to only allow specific TLS >> versions. Adding configuration on the libpq side as suggested in the >> original discussion might help here. > > I can easily do that, but I won't have time until next week or so. I can do that too, just need a hint where to look at in libpq/psql to add the option. For SSL we have sslmode and sslcompression, etc. in conninfo, so adding sslprotocols seems to be an option. As an aside note: should we also expose a parameter to choose SSL ciphers (would be a separate patch)? -- Alex -- 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] Merging recovery.conf into PostgreSQL.conf -- where did this go?
Andres Freund writes: > > On 2014-11-19 15:09:10 -0800, Josh Berkus wrote: >> One patch that got deferred from 9.4 was the merger of recovery.conf and >> postgresql.conf, due to conflicts with ALTER SYSTEM SET. However, this >> is still a critical TODO, because recovery.conf is still an ongoing >> major management problem for PostgreSQL DBAs. >> >> So, what happened to this? I kinda expected it to get committed right >> after we opened 9.5. > > Nobody did the remaining work. I'd like to work on this. AFAICT, this CommitFest entry is the latest on the subject, right? https://commitfest.postgresql.org/action/patch_view?id=1293 Should/may I move it to the next Open fest? -- Alex -- 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] add ssl_protocols configuration option
Dag-Erling Smørgrav writes: > Alex Shulgin writes: >> I can do that too, just need a hint where to look at in libpq/psql to >> add the option. > > The place to *enforce* the option is src/interfaces/libpq/fe-secure.c > (look for SSLv23_method() and SSL_CTX_set_options()). I haven't looked > into how to set it. Yes, I've figured it out. Guess we'd better share the ssl_protocol value parser code between libpq and the backend. Any precedent? -- Alex -- 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] Turning recovery.conf into GUCs
mptz_in() and even
> DateTimeParseError(), replacing the ereport()s by the guc error
> reporting.
The use of PG_TRY/CATCH does protect from FATALs in SIGHUP indeed.
Using CopyErrorData() we can also fetch the actual error message from
timestamptz_in, though I wonder we really have to make a full copy.
>> > * Why do you include xlog_internal.h in guc.c and not xlog.h?
>
>> we actually need both but including xlog_internal.h also includes xlog.h
>> i added xlog.h and if someone things is enough only putting
>> xlog_internal.h let me know
>
> What's required from xlog_internal.h?
Looks like this was addressed before me.
>> diff --git a/src/backend/access/transam/xlog.c
>> b/src/backend/access/transam/xlog.c
>> index b53ae87..54f6a0d 100644
>> --- a/src/backend/access/transam/xlog.c
>> +++ b/src/backend/access/transam/xlog.c
>> @@ -64,11 +64,12 @@
>> extern uint32 bootstrap_data_checksum_version;
>>
>> /* File path names (all relative to $PGDATA) */
>> -#define RECOVERY_COMMAND_FILE "recovery.conf"
>> -#define RECOVERY_COMMAND_DONE "recovery.done"
>> +#define RECOVERY_ENABLE_FILE"standby.enabled"
>
> Imo the file and variable names should stay coherent.
Yes, once we settle on the name (and if we really need that extra
trigger file.)
>> +/* recovery.conf is not supported anymore */
>> +#define RECOVERY_COMMAND_FILE "recovery.conf"
>
>> +boolStandbyModeRequested = false;
>> +static TimestampTz recoveryDelayUntilTime;
>
> This imo should be lowercase now, the majority of GUC variables are.
This is not a GUC variable, though it's calculated based on a GUC
recovery_min_apply_delay.
>> +/* are we currently in standby mode? */
>> +bool StandbyMode = false;
>
> Why did you move this?
It was easy to move it back though.
>> -if (rtliGiven)
>> +if (strcmp(recovery_target_timeline_string, "") != 0)
>> {
>
> Why not have the convention that NULL indicates a unset target_timeline
> like you use for other GUCs? Mixing things like this is confusing.
>
> Why is recovery_target_timeline stored as a string? Because it's a
> unsigned int? If so, you should have an assign hook setting up a)
> rtliGiven, b) properly typed variable.
Yes, I believe setting these to NULL by default makes a lot more sense.
Then one can check if there was a non-default setting by checking the
*_string variable, which is not possible with int or TimestampTz.
>> -if (rtli)
>> +if (recovery_target_timeline)
>> {
>> /* Timeline 1 does not have a history file, all else
>> should */
>> -if (rtli != 1 && !existsTimeLineHistory(rtli))
>> +if (recovery_target_timeline != 1 &&
>> +
>> !existsTimeLineHistory(recovery_target_timeline))
>> ereport(FATAL,
>> (errmsg("recovery target
>> timeline %u does not exist",
>> -rtli)));
>> -recoveryTargetTLI = rtli;
>> +
>> recovery_target_timeline)));
>> +recoveryTargetTLI = recovery_target_timeline;
>> recoveryTargetIsLatest = false;
>
> So, now we have a recoveryTargetTLI and recovery_target_timeline
> variable? Really? Why do we need recoveryTargetTLI at all now?
Looks like we still do need both of them. The initial value of
recoveryTargetTLI is derived from pg_control, but later it can be
overriden by this setting.
However, if the recovery_target_timeline was "latest" we need to find
the latest TLI, based on the initial value from pg_control. But we
can't set final recoveryTargetTLI value in the GUC assign hook, because
we might need to fetch some history files first.
>> +static void
>> +assign_recovery_target_time(const char *newval, void *extra)
>> +{
>> +recovery_target_time = *((TimestampTz *) extra);
>> +
>> +if (recovery_target_xid != InvalidTransactionId)
>> +recovery_target = RECOVERY_TARGET_XID;
>> +else if (recovery_target_name[0])
>> +recovery_target = RECOVERY_TARGET_NAME;
>> +else if (recovery_target_time != 0)
>> +recovery_target = RECOVERY_TARGET_TIME;
>> +else
>> +recovery_target = RECOVERY_TARGET_UNSET;
>> +}
>> +
>
> I don't think it's correct to do such han
Re: [HACKERS] Turning recovery.conf into GUCs
Alex Shulgin writes: > >>> > * Why do you include xlog_internal.h in guc.c and not xlog.h? >> >>> we actually need both but including xlog_internal.h also includes xlog.h >>> i added xlog.h and if someone things is enough only putting >>> xlog_internal.h let me know >> >> What's required from xlog_internal.h? > > Looks like this was addressed before me. Actually, it's there to bring in MAXFNAMELEN which is used in check_recovery_target_name. Not sure how it even compiled without that, but after some branch switching it doesn't anymore. :-p Maybe we should move these check/assign hooks to xlog.c, but that's likely going to create header files dependency problem due to use of GucSource in the hook prototype... -- Alex -- 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] Turning recovery.conf into GUCs
Josh Berkus writes: >> >> Well, the latest version of this patch fails to start when it sees >> 'recovery.conf' in PGDATA: >> >> FATAL: "recovery.conf" is not supported anymore as a recovery method >> DETAIL: Refer to appropriate documentation about migration methods >> >> I've missed all the discussion behind this decision and after reading >> the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like >> someone more knowledgeable to speak up on the status of this. > > The argument was that people wanted to be able to have an empty > recovery.conf file as a "we're in standby" trigger so that they could > preserve backwards compatibility with external tools. I don't agree > with this argument, but several people championed it. It doesn't look like we can provide for 100% backwards compatibility with existing tools, here's why: a) The only sensible way to use recovery.conf as GUC source is via include/include_dir from postgresql.conf. b) The server used to rename $PGDATA/recovery.conf to $PGDATA/recovery.done so when it is re-started it doesn't accidentally start in recovery mode. We can't keep doing (b) because that will make postgres fail to start on a missing include. Well, it won't error out if we place the file under include_dir, as only files with the ".conf" suffix are included, but then again the server wouldn't know which file to rename unless we tell it or hard-code the the filename. Either way this is not 100% backwards compatible. Now the question is if we are going to break compatibility anyway: should we try to minimize the difference or will it disserve the user by providing a false sense of compatibility? For one, if we're breaking things, the trigger_file GUC should be renamed to end_recovery_trigger_file or something like that, IMO. That's if we decide to keep it at all. >> The docs use the term "continuous recovery". >> >> Either way, from the code it is clear that we only stay in recovery if >> standby_mode is directly turned on. This makes the whole check for a >> specially named file unnecessary, IMO: we should just check the value of >> standby_mode (which is off by default). > > So, what happens when someone does "pg_ctl promote"? Somehow > standby_mode needs to get set to "off". Maybe we write "standby_mode = > off" to postgresql.auto.conf? Well, standby_mode is only consulted at startup after crash recovery. But suddenly, a tool that *writes* recovery.conf needs to also consult/edit postgresql.auto.conf... Maybe that's inevitable, but still a point to consider. >> By the way, is there any use in setting standby_mode=on and any of the >> recovery_target* GUCs at the same time? > > See my thread on this topic from last week. Short answer: No. > >> I think it can only play together if you set the target farther than the >> latest point you've got in the archive locally. So that's sort of >> "Point-in-Future-Recovery". Does that make any sense at all? > > Again, see the thread. This doesn't work in a useful way, so there's no > reason to write code to enable it. Makes sense. Should we incorporate the actual tech and doc fix in this patch? >>>> /* File path names (all relative to $PGDATA) */ >>>> -#define RECOVERY_COMMAND_FILE "recovery.conf" >>>> -#define RECOVERY_COMMAND_DONE "recovery.done" >>>> +#define RECOVERY_ENABLE_FILE "standby.enabled" >>> >>> Imo the file and variable names should stay coherent. >> >> Yes, once we settle on the name (and if we really need that extra >> trigger file.) > > Personally, if we have three methods of promotion: > > 1) pg_ctl promote > 2) edit postgresql.conf and reload > 3) ALTER SYSTEM SET and reload > > ... I don't honestly think we need a 4th method for promotion. Me neither. It is tempting to make everything spin around GUCs without the need for any external trigger files. > The main reason to want a "we're in recovery file" is for PITR rather > than for replication, where it has a number of advantages as a method, > the main one being that recovery.conf is unlikely to be overwritten by > the contents of the backup. > > HOWEVER, there's a clear out for this with conf.d. If we enable conf.d > by default, then we can simply put recovery settings in conf.d, and > since that specific file (which can have whatever name the user chooses) > will not be part of backups, it would have the same advantage as > recovery.conf, without the drawbacks. > > Discuss? Well, I don't readily see how conf.d is special with regard to base backup, wouldn't you need to exclude it explicitly still? -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Replication connection URI?
Hackers, It appears that replication connection doesn't support URI but only the traditional conninfo string. src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect(): snprintf(conninfo_repl, sizeof(conninfo_repl), "%s dbname=replication replication=true fallback_application_name=walreceiver", conninfo); A patch to fix this welcome? -- Alex PS: I wrote the original URI parser used in libpq. -- 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] Replication connection URI?
Heikki Linnakangas writes:
>>
>> It appears that replication connection doesn't support URI but only the
>> traditional conninfo string.
>>
>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in
>> libpqrcv_connect():
>>
>> snprintf(conninfo_repl, sizeof(conninfo_repl),
>> "%s dbname=replication replication=true
>> fallback_application_name=walreceiver",
>> conninfo);
>>
>> A patch to fix this welcome?
>
> Yeah, seems like an oversight. Hopefully you can fix that without
> teaching libpqwalreceiver what connection URIs look like..
Please see attached. We're lucky that PQconnectdbParams has an option
to parse and expand the first dbname parameter if it looks like a
connection string (or a URI).
The first patch is not on topic, I just spotted this missing check.
The second one is a self-contained fix, but the third one which is the
actual patch depends on the second one, because it specifies the dbname
keyword two times: first to parse the conninfo/URI, then to override any
dbname provided by the user with "replication" pseudo-database name.
Have a nice day!
--
Alex
>From 156e6faa96ad6a2ce58055ad72883ed78c576e5b Mon Sep 17 00:00:00 2001
From: Alex Shulgin
Date: Mon, 24 Nov 2014 16:55:50 +0300
Subject: [PATCH 1/3] Add missing check on OOM in expand_dbname path of
conninfo_array_parse().
---
src/interfaces/libpq/fe-connect.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 3fe8c21..d7f2ec2
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_array_parse(const char *const *
*** 4402,4407
--- 4402,4415
if (options[k].val)
free(options[k].val);
options[k].val = strdup(str_option->val);
+ if (!options[k].val)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("out of memory\n"));
+ PQconninfoFree(options);
+ PQconninfoFree(dbname_options);
+ return NULL;
+ }
break;
}
}
--
2.1.0
>From 44d9d6a2c9cf5af83988f9d3b6eeb39c36104ef9 Mon Sep 17 00:00:00 2001
From: Alex Shulgin
Date: Mon, 24 Nov 2014 18:12:51 +0300
Subject: [PATCH 2/3] Allow further dbname=value to override conninfo parsed
from an expanded dbname in conninfo_array_parse().
---
src/interfaces/libpq/fe-connect.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index d7f2ec2..5b45128
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_parse(const char *conninfo, PQE
*** 4302,4311
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
*
! * If expand_dbname is non-zero, and the value passed for keyword "dbname" is a
! * connection string (as indicated by recognized_connection_string) then parse
! * and process it, overriding any previously processed conflicting
! * keywords. Subsequent keywords will take precedence, however.
*/
static PQconninfoOption *
conninfo_array_parse(const char *const * keywords, const char *const * values,
--- 4302,4313
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
*
! * If expand_dbname is non-zero, and the value passed for the first occurrence
! * of "dbname" keyword is a connection string (as indicated by
! * recognized_connection_string) then parse and process it, overriding any
! * previously processed conflicting keywords. Subsequent keywords will take
! * precedence, however. In particular, a further occurrence of "dbname" may
! * override the dbname provided in the connection string.
*/
static PQconninfoOption *
conninfo_array_parse(const char *const * keywords, const char *const * values,
*** conninfo_array_parse(const char *const *
*** 4381,4387
}
/*
! * If we are on the dbname parameter, and we have a parsed
* connection string, copy those parameters across, overriding any
* existing previous settings.
*/
--- 4383,4389
}
/*
! * If we are on the *first* dbname parameter, and we have a parsed
* connection string, copy those parameters across, overriding any
* existing previous settings.
*/
*** conninfo_array_parse(const char *const *
*** 4415,4420
--- 4417,4428
}
}
}
+ /*
+ * Clear out the parsed dbname, so that a possible further
+ * occu
Re: [HACKERS] Turning recovery.conf into GUCs
Jaime Casanova writes: >>> >>> Either way, from the code it is clear that we only stay in recovery if >>> standby_mode is directly turned on. This makes the whole check for a >>> specially named file unnecessary, IMO: we should just check the value of >>> standby_mode (which is off by default). > > no. currently we enter in recovery mode when postgres see a > recovery.conf and stays in recovery mode when standby_mode is on or an > appropiate restore_command is provided. > > which means recovery.conf has two uses: > 1) start in recovery mode (not continuous) > 2) provide parameters for recovery mode and for streaming > > we still need a "recovery trigger" file that forces postgres to start > in recovery mode and acts accordingly to recovery GUCs Yes, these were my latest findings also, but if instead of removing the trigger file upon successful recovery the server would set standby_mode=off, that would also work. -- Alex -- 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] Turning recovery.conf into GUCs
Josh Berkus writes: >> >> only that you need to start in recovery mode to start replication > > Right, but my point is that having a trigger file *is not necessary for > replication, only for PITR* -- and maybe not necessary even for PITR. > That is, in a streaming replication configuration, having a > "standby_mode = on|off" parameter is 100% sufficient to control > replication with the small detail that "pg_ctl promote" needs to set it > in pg.auto.conf or conf.d. > > And, now, having given it some thought, I'm going to argue that it's not > required for PITR either, provided that we can use the auto.conf method. > > Before I go into my ideas, though, what does the current patch do > regarding non-replication PITR? It removes that $PGDATA/standby.enable trigger file it relies on to start the PITR in the first place. -- Alex -- 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] Turning recovery.conf into GUCs
Josh Berkus writes: >>> >>> Before I go into my ideas, though, what does the current patch do >>> regarding non-replication PITR? >> >> It removes that $PGDATA/standby.enable trigger file it relies on to >> start the PITR in the first place. > > OK, and that's required for replication too? I'm OK with that if it > gets the patch in. In the current form of the patch, yes. Thought I don't think I like it. -- Alex -- 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] Replication connection URI?
Alex Shulgin writes: > Heikki Linnakangas writes: >>> >>> It appears that replication connection doesn't support URI but only the >>> traditional conninfo string. >>> >>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in >>> libpqrcv_connect(): >>> >>> snprintf(conninfo_repl, sizeof(conninfo_repl), >>> "%s dbname=replication replication=true >>> fallback_application_name=walreceiver", >>> conninfo); >>> >>> A patch to fix this welcome? >> >> Yeah, seems like an oversight. Hopefully you can fix that without >> teaching libpqwalreceiver what connection URIs look like.. > > Please see attached. We're lucky that PQconnectdbParams has an option > to parse and expand the first dbname parameter if it looks like a > connection string (or a URI). > > The first patch is not on topic, I just spotted this missing check. > > The second one is a self-contained fix, but the third one which is the > actual patch depends on the second one, because it specifies the dbname > keyword two times: first to parse the conninfo/URI, then to override any > dbname provided by the user with "replication" pseudo-database name. These patches are really simple, I hope a committer will pick them up? Or should I add them to the commitfest? Also, I'd rather get this committed first, then rebase that recovery.conf->GUC patch onto it and submit an updated version. Thanks. -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)
Heikki Linnakangas writes:
> On 11/24/2014 06:05 PM, Alex Shulgin wrote:
>> The first patch is not on topic, I just spotted this missing check.
>
>> *** a/src/interfaces/libpq/fe-connect.c
>> --- b/src/interfaces/libpq/fe-connect.c
>> *** conninfo_array_parse(const char *const *
>> *** 4402,4407
>> --- 4402,4415
>> if
>> (options[k].val)
>>
>> free(options[k].val);
>> options[k].val
>> = strdup(str_option->val);
>> +if
>> (!options[k].val)
>> +{
>> +
>> printfPQExpBuffer(errorMessage,
>> +
>> libpq_gettext("out of memory\n"));
>> +
>> PQconninfoFree(options);
>> +
>> PQconninfoFree(dbname_options);
>> +return
>> NULL;
>> +}
>> break;
>> }
>> }
>
> Oh. There are actually many more places in connection option parsing
> that don't check the return value of strdup(). The one in fillPGConn
> even has an XXX comment saying it probably should check it. You can
> get quite strange behavior if one of them fails. If for example the
> strdup() on dbname fails, you might end up connecting to different
> database than intended. And if the "conn->sslmode =
> strdup(DefaultSSLMode);" call in connectOptions2 fails, you'll get a
> segfault later because at least connectDBstart assumes that sslmode is
> not NULL.
>
> I think we need to fix all of those, and backpatch. Per attached.
Yikes! Looks sane to me.
I've checked that patches #2 and #3 can be applied on top of this, w/o
rebasing.
--
Alex
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)
Heikki Linnakangas writes: >>> >>> I think we need to fix all of those, and backpatch. Per attached. >> >> Yikes! Looks sane to me. > > Ok thanks, committed. It didn't apply cleanly to 9.0, 9.1 and 9.2, so > the patch for those branches looks a bit different. Great. Are you looking at the actual replication URI patch? Or should I add it to commitfest so we don't lose track of it? -- Alex -- 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] Replication connection URI?
Heikki Linnakangas writes: >>> >>> The second one is a self-contained fix, but the third one which is the >>> actual patch depends on the second one, because it specifies the dbname >>> keyword two times: first to parse the conninfo/URI, then to override any >>> dbname provided by the user with "replication" pseudo-database name. >> >> Hmm. Should we backpatch the second patch? It sure seems like an >> oversight rather than deliberate that you can't override dbname from the >> connection string with a later dbname keyword. I'd say "yes". >> >> How about the third patch? Probably not; it was an oversight with the >> connection URI patch that it could not be used in primary_conninfo, but >> it's not a big deal in practice as you can always use a non-URI >> connection string instead. > > Ok, committed the second patch to all stable branches, and the third > patch to master. > > In the second patch, I added a sentence to the docs to mention that > only the first "dbname" parameter is expanded. It's debatable if > that's what we actually want. It would be handy to be able to merge > multiple connection strings, by specifying multiple dbname > parameters. But now the docs at least match the code, changing the > behavior would be a bigger change. > > From the third patch, I removed the docs changes. It's necessary to > say "connection string or URI" everywhere, the URI format is just one > kind of a connection string. I also edited the code that builds the > keyword/value array, to make it a bit more readable. Yay, many thanks! :-) -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Tom, First of all, thanks for your help on IRC last time with that CREATE INDEX memory consumption problem. As has been pointed out in a stackexchange answer to my question[1], it is indeed the limitation of pre-9.4 versions, but the limit is imposed on memtuples array, rather than total memory the sort in CREATE INDEX may allocate. The memtuples won't grow further than MaxAllocSize and I've got 24x50x10^6 = 1200MB, which just doesn't fit. We've got a customer who is testing a migration to PostgreSQL-9.3 (from $some_other_db), thus they load the tables first (some of their tables have 10-100 million rows), then create the indexes and they constantly see disk sort being used despite lots of available RAM and maintenance_work_mem set to increasingly higher values. Now my question, is it feasible to back-patch this to 9.3? Or should we tell the customer to wait before 9.4 is released? Thanks. -- Alex [1] http://dba.stackexchange.com/questions/83600/postgresql-create-index-memory-requirement -- 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] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Tom Lane writes: > Stephen Frost writes: >> * Alex Shulgin (a...@commandprompt.com) wrote: >>> Tom, >>> >>> First of all, thanks for your help on IRC last time with that CREATE >>> INDEX memory consumption problem. > >> Doubt it was Tom, but if it was, wanna share what channel on IRC it was? >> :D > > Must've been my evil twin. Sorry, I must be under false impression that RhodiumToad is *your* nick on #postgresql at freenode. I don't recall who told me that, but I was pretty sure it's you. :-p -- Alex -- 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] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Andrew Gierth writes: >>>>>> "Alex" == Alex Shulgin writes: > > > Tom Lane writes: > >> Must've been my evil twin. > > Alex> Sorry, I must be under false impression that RhodiumToad is > Alex> *your* nick on #postgresql at freenode. I don't recall who > Alex> told me that, but I was pretty sure it's you. :-p > > ... what > > People do occasionally make jokes on IRC about me being Tom's clone; I > know they mean it in a positive way but I still find it *extremely* > annoying, so I do try and discourage it. (If they're making those same > jokes elsewhere, I haven't been aware of it, but please consider this > a polite public request to stop.) > > My first name is easily visible in the irc gecos field: > > *** RhodiumToad is ~andrew@[my hostname] (Andrew) > > and there is also the IRC users list on the wiki: > http://wiki.postgresql.org/wiki/IRC2RWNames Andrew, Tom, Sorry for the confusion. And, Andrew, thanks again for the help! :-) -- Alex -- 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] add ssl_protocols configuration option
Alex Shulgin writes: >>> >>> I can do that too, just need a hint where to look at in libpq/psql to >>> add the option. >> >> The place to *enforce* the option is src/interfaces/libpq/fe-secure.c >> (look for SSLv23_method() and SSL_CTX_set_options()). I haven't looked >> into how to set it. > > Yes, I've figured it out. Guess we'd better share the ssl_protocol > value parser code between libpq and the backend. Any precedent? OK, looks like I've come up with something workable: I've added sslprotocol connection string keyword similar to pre-existing sslcompression, etc. Please see attached v2 of the original patch. I'm having doubts about the name of openssl.h header though, libpq-openssl.h? -- Alex >From 7cd60e962ce5e7fc10dc52ed9c92b0b2b5c4c7f1 Mon Sep 17 00:00:00 2001 From: Alex Shulgin Date: Wed, 19 Nov 2014 19:49:29 +0300 Subject: [PATCH] DRAFT: ssl_protocols GUC --- doc/src/sgml/config.sgml | 29 ++ doc/src/sgml/libpq.sgml | 25 ++ src/backend/libpq/Makefile| 2 +- src/backend/libpq/be-secure-openssl.c | 29 -- src/backend/libpq/be-secure.c | 3 +- src/backend/libpq/openssl.c | 124 ++ src/backend/utils/misc/guc.c | 15 src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/libpq/libpq.h | 1 + src/include/libpq/openssl.h | 50 +++ src/interfaces/libpq/Makefile | 8 +- src/interfaces/libpq/fe-connect.c | 4 + src/interfaces/libpq/fe-secure-openssl.c | 18 +++- src/interfaces/libpq/libpq-int.h | 1 + 14 files changed, 300 insertions(+), 10 deletions(-) create mode 100644 src/backend/libpq/openssl.c create mode 100644 src/include/libpq/openssl.h diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml new file mode 100644 index ab8c263..8b701df *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** include_dir 'conf.d' *** 1027,1032 --- 1027,1061 + + ssl_protocols (string) + +ssl_protocols configuration parameter + + + + + Specifies a colon-separated list of SSL protocols that are + allowed to be used on secure connections. Each entry in the list can + be prefixed by a + (add to the current list) + or - (remove from the current list). If no prefix is used, + the list is replaced with the specified protocol. + + + The full list of supported protocols can be found in the + the openssl manual page. In addition to the names of + individual protocols, the following keywords can be + used: ALL (all protocols supported by the underlying + crypto library), SSL (all supported versions + of SSL) and TLS (all supported versions + of TLS). + + + The default is ALL:-SSL. + + + + ssl_ciphers (string) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml new file mode 100644 index e23e91d..9ea71a4 *** a/doc/src/sgml/libpq.sgml --- b/doc/src/sgml/libpq.sgml *** postgresql://%2Fvar%2Flib%2Fpostgresql/d *** 1230,1235 --- 1230,1250 + + sslprotocols + + + Specifies a colon-separated list of SSL protocols that are + allowed to be used on secure connections. + See server configuration parameter + for format. + + + Defaults to ALL:-SSL. + + + + sslcompression *** myEventProc(PGEventId evtId, void *evtIn *** 6711,6716 --- 6726,6741 + + + +PGSSLPROTOCOLS + + PGSSLPROTOCOLS behaves the same as the connection parameter. + + + diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile new file mode 100644 index 09410c4..1c058ec *** a/src/backend/libpq/Makefile --- b/src/backend/libpq/Makefile *** OBJS = be-fsstubs.o be-secure.o auth.o c *** 18,24 pqformat.o pqmq.o pqsignal.o ifeq ($(with_openssl),yes) ! OBJS += be-secure-openssl.o endif include $(top_srcdir)/src/backend/common.mk --- 18,24 pqformat.o pqmq.o pqsignal.o ifeq ($(with_openssl),yes) ! OBJS += be-secure-openssl.o openssl.o endif include $(top_srcdir)/src/backend/common.mk diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c new file mode 100644 index b05364c..3801455 *** a/src/backend/libpq/be-secure-openssl.c --- b/src/backend/libpq/be-secure-openssl.c *** *** 7
Re: [HACKERS] Remove Windows crash dump support?
On 22.12.2015 18:28, Magnus Hagander wrote: On Tue, Dec 22, 2015 at 3:53 PM, Craig Ringer <mailto:cr...@2ndquadrant.com>> wrote: On 22 December 2015 at 22:50, Craig Ringer mailto:cr...@2ndquadrant.com>> wrote: Hi all Back in 2010 I submitted a small feature to allow the creation of minidumps when backends crashed; see commit dcb09b595f88a3bca6097a6acc17bf2ec935d55f . At the time Windows lacked useful support for postmortem debugging and crash-dump management in the operating system its self, especially for applications running as services. That has since improved considerably. The feature was also included in 9.4 Ahem. 9.1. This is what I get for multi-tasking between writing this and packaging an extension for 9.4. In which version(s) of Windows was this improvement added? I think that's really the part that matters here, not necessarily which version of PostgreSQL. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ Hi all! I think that you can debug crash dump since windbg exists. Also I think that Postgres on Windows number of instalations is so tiny because people even today think that it is not so solid as unix version thats why you think that nobody use your code ;). Today if my memory serves me right this code can not deal with buffer overflow. Am i right? May be we need to add this functionality instead of drop support of it entirely? -- Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] plperlu problem with utf8
On Wed, Dec 8, 2010 at 14:15, Oleg Bartunov wrote:
> On Wed, 8 Dec 2010, David E. Wheeler wrote:
>
>> On Dec 8, 2010, at 8:13 AM, Oleg Bartunov wrote:
>>
>>> adding utf8::decode($_[0]) solves the problem:
>> Hrm. Ideally all strings passed to PL/Perl functions would be decoded.
>
> yes, this is what I expected.
Erm... no. The in and out from perl AFAICT works fine (minus a caveat
I found, see the end of the mail).
The problem here is you have the url encoded utf8 bytes "%C3%A9".
URL::Encode basically does chr(hex("c3")) and chr(hex("a9"));. Perl,
generally, will treat that as two separate unicode code points. So
you end up with two characters (one with a code point of 0xc3, the
other with 0xa9) instead of the one character you expect. If you want
\xc3\xa9 to be treated as a utf8 byte sequence, you need to tell perl
those bytes are utf8 by decoding it. Heck for all we know instead of
it being a utf8 sequence, it could have been a utf16 sequence.
You might argue this is a bug with URI::Escape as I *think* all uri's
will be utf8 encoded. Anyway, I think postgres is doing the right
thing here.
In playing around I did find what I think is a postgres bug. Perl has
2 ways it can store things internally. per perldoc perlunicode:
Using Unicode in XS
... What the "UTF8" flag means is that the sequence of octets in the
representation of the scalar is the sequence of UTF-8 encoded code
points of the characters of a string. The "UTF8" flag being off means
that each octet in this representation encodes a single character with
code point 0..255 within the string.
Postgres always prints whatever the internal representation happens to
be ignoring the UTF8 flag and the server encoding.
# create or replace function chr(i int, i2 int) returns text as $$
return chr($_[0]).chr($_[1]); $$ language plperlu;
CREATE FUNCTION
# show server_encoding;
server_encoding
-
SQL_ASCII
# SELECT length(chr(128, 33));
length
2
# SELECT length(chr(128, 333));
length
4
Grr that should error out with "Invalid server encoding", or worst
case should return a length of 3 (it utf8 encoded 128 into 2 bytes
instead of leaving it as 1). In this case the 333 causes perl store
it internally as utf8.
Now on a utf8 database:
# show server_encoding;
server_encoding
-
UTF8
# SELECT length(chr(128, 33));
ERROR: invalid byte sequence for encoding "UTF8": 0x80
CONTEXT: PL/Perl function "chr"
# SELECT length(chr(128, 333));
CONTEXT: PL/Perl function "chr"
length
2
Same thing here, we just end up using the internal format. In one
case it works in the other it does not. The main point being, most of
the time it *happens* to work. But its really just by chance.
I think what we should do is use SvPVutf8() when we are UTF8 instead
of SvPV in sv2text_mbverified(). SvPV gives us a pointer to a string
in perls current internal format (maybe unicode, maybe a utf8 byte
sequence). While SvPVutf8 will always give us utf8 (may or may not be
valid!) encoded string.
Something like the attached. Thoughts? Im not very happy with the non
utf8 case-- The elog(ERROR, "invalid byte sequence") is a total
cop-out yes. But I did not see a good solution short of hand rolling
our own version of sv_utf8_downgrade(). Is it worth it?
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 5595baa..8a9d677 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -254,7 +254,31 @@ sv2text_mbverified(SV *sv)
* length, whatever uses the "verified" value might get something quite
* weird.
*/
- val = SvPV(sv, len);
+
+ /*
+ * When we are in an UTF8 encoding we want to make sure we get back a utf8
+ * byte sequence instead of whatever perls internal format happens to be.
+ *
+ * Non UTF8 will just treat everything as bytes/latin1 that is
+ * SvPVutf8(chr(170)) len == 2
+ * SvPVbyte(chr(170)) len == 1
+ * SvPV(chr(170))) len == 1 || 2
+ */
+ if (GetDatabaseEncoding() == PG_UTF8)
+ val = SvPVutf8(sv, len);
+ else
+ {
+ /*
+ * See if we can safely represent our string as bytes if not bail out
+ * otherwise perl dies with "Wide Character" and takes the backend down
+ * with it
+ */
+ if (sv_utf8_downgrade(sv, true))
+ val = SvPVbyte(sv, len);
+ else
+ elog(ERROR, "invalid byte sequence");
+ }
+
pg_verifymbstr(val, len, false);
return val;
}
--
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] plperlu problem with utf8
On Thu, Dec 16, 2010 at 20:24, David E. Wheeler wrote:
> On Dec 16, 2010, at 6:39 PM, Alex Hunsaker wrote:
>
>> You might argue this is a bug with URI::Escape as I *think* all uri's
>> will be utf8 encoded. Anyway, I think postgres is doing the right
>> thing here.
>
> No, URI::Escape is fine. The issue is that if you don't decode text to Perl's
> internal form, it assumes that it's Latin-1.
So... you are saying "\xc3\xa9" eq "\xe9" or chr(233) ?
Im saying they are not, and if you want \xc3\xa9 to be treated as
chr(233) you need to tell perl what encoding the string is in (err
well actually decode it so its in "perl space" as unicode characters
correctly).
> Maybe I'm misunderstanding, but it seems to me that:
>
> * String arguments passed to PL/Perl functions should be decoded from the
> server encoding to Perl's internal representation before the function
> actually gets them.
Currently postgres has 2 behaviors:
1) If the database is utf8, turn on the utf8 flag. According to the
perldoc snippet I quoted this should mean its a sequence of utf8 bytes
and should interpret it as such.
2) its not utf8, so we just leave it as octets.
So in "perl space" length($_[0]) returns the number of characters when
you pass in a multibyte char *not* the number of bytes. Which is
correct, so um check we do that. Right?
In the URI::Escape example we have:
# CREATE OR REPLACE FUNCTION url_decode(Vkw varchar) RETURNS varchar AS $$
use URI::Escape;
warn(length($_[0]));
return uri_unescape($_[0]); $$ LANGUAGE plperlu;
# select url_decode('comment%20passer%20le%20r%C3%A9veillon');
WARNING: 38 at line 2
Ok that length looks right, just for grins lets try add one multibyte char:
# SELECT url_decode('comment%20passer%20le%20r%C3%A9veillon☺');
WARNING: 39 CONTEXT: PL/Perl function "url_decode" at line 2.
url_decode
---
comment passer le réveillon☺
(1 row)
Still right, now lets try the utf8::decode version that "works". Only
lets look at the length of the string we are returning instead of the
one we are passing in:
# CREATE OR REPLACE FUNCTION url_decode(Vkw varchar) RETURNS varchar AS $$
use URI::Escape;
utf8::decode($_[0]);
my $str = uri_unescape($_[0]);
warn(length($str));
return $str;
$$ LANGUAGE plperlu;
# SELECT url_decode('comment%20passer%20le%20r%C3%A9veillon');
WARNING: 28 at line 5.
CONTEXT: PL/Perl function "url_decode"
url_decode
-
comment passer le réveillon
(1 row)
Looks harmless enough...
# SELECT length(url_decode('comment%20passer%20le%20r%C3%A9veillon'));
WARNING: 28 at line 5.
CONTEXT: PL/Perl function "url_decode"
length
27
(1 row)
Wait a minute... those lengths should match.
Post patch they do:
# SELECT length(url_decode('comment%20passer%20le%20r%C3%A9veillon'));
WARNING: 28 at line 5.
CONTEXT: PL/Perl function "url_decode"
length
28
(1 row)
Still confused? Yeah me too. Maybe this will help:
#!/usr/bin/perl
use URI::Escape;
my $str = uri_unescape("%c3%a9");
die "first match" if($str =~ m/\xe9/);
utf8::decode($str);
die "2nd match" if($str =~ m/\xe9/);
gives:
$ perl t.pl
2nd match at t.pl line 6.
see? Either uri_unescape() should be decoding that utf8() or you need
to do it *after* you call uri_unescape(). Hence the maybe it could be
considered a bug in uri_unescape().
> * Values returned from PL/Perl functions that are in Perl's internal
> representation should be encoded into the server encoding before they're
> returned.
> I didn't really follow all of the above; are you aiming for the same thing?
Yeah, the patch address this part. Right now we just spit out
whatever the internal format happens to be.
Anyway its all probably clear as mud, this part of perl is one of the
hardest IMO.
--
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] plperlu problem with utf8
On Fri, Dec 17, 2010 at 08:30, David Fetter wrote: > On Thu, Dec 16, 2010 at 07:24:46PM -0800, David Wheeler wrote: >> On Dec 16, 2010, at 6:39 PM, Alex Hunsaker wrote: >> >> > Grr that should error out with "Invalid server encoding", or worst >> > case should return a length of 3 (it utf8 encoded 128 into 2 bytes >> > instead of leaving it as 1). In this case the 333 causes perl >> > store it internally as utf8. >> >> Well with SQL_ASCII anything goes, no? > > Anything except a byte that's all 0s :( Keyword being byte, right? Last time I checked 333 wont fit in a byte :P. In this case perl stores "333" as the utf8 that represents the unicode code point 333. Postgres uses whatever that internal representation is, so in our SQL_ASCII database we actually end up getting back utf8 which _is_ valid SQL_ASCII, but I wouldn't call it "right". The behavior im aiming for is similar to what the built-in chr does: # SELECT chr(333); ERROR: requested character too large for encoding: 333 Also note this is just a simple test case, perl *could* elect to store completely ascii strings internally as utf8. In those cases we still "do the wrong thing", that is we get back utf8ified bytes :( Although obviously from the lack of bug reports its quite uncommon. -- 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] plperlu problem with utf8
On Fri, Dec 17, 2010 at 11:51, Alex Hunsaker wrote: > Also note this is just a simple test case, perl *could* elect to store > completely ascii strings internally as utf8. In those cases we still Erm... not ascii I mean bytes >127 -- 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] plperlu problem with utf8
On Fri, Dec 17, 2010 at 22:32, David Christensen wrote:
>
> On Dec 17, 2010, at 7:04 PM, David E. Wheeler wrote:
>
>> On Dec 16, 2010, at 8:39 PM, Alex Hunsaker wrote:
>>
>>>> No, URI::Escape is fine. The issue is that if you don't decode text to
>>>> Perl's internal form, it assumes that it's Latin-1.
>>>
>>> So... you are saying "\xc3\xa9" eq "\xe9" or chr(233) ?
>>
>> Not knowing what those mean, I'm not saying either one, to my knowledge.
>> What I understand, however, is that Perl, given a scalar with bytes in it,
>> will treat it as latin-1 unless the utf8 flag is turned on.
>
> This is a correct assertion as to Perl's behavior. As far as PostgreSQL
> is/should be concerned in this case, this is the correct handling for
> URI::Escape,
Right, so no postgres bug here.. Postgres showing é instead of é is
right as far as its concerned.
>> PostgreSQL should do everything it can to decode to Perl's internal format
>> before passing arguments, and to decode from Perl's internal format on
>> output.
>
> +1 on the original sentiment, but only for the case that we're dealing with
> data that is passed in/out as arguments. In the case that the
> server_encoding is UTF-8, this is as trivial as a few macros on the
> underlying SVs for text-like types. If the server_encoding is SQL_ASCII (=
> byte soup), this is a trivial case of doing nothing with the conversion
> regardless of data type.
Right and thats what we do for the above. Minus some mis-handling of
non character datatypes like bytea in the UTF-8 case.
> For any other server_encoding, the data would need to be converted from the
> server_encoding to UTF-8, presumably using the built-in conversions before
> passing it off to the first code path. A similar handling would need to be
> done for the return values, again datatype-dependent.
Yeah, thats what we *should* do. Right now we just leave it as byte
soup for the user to decode/encode. :(
> [ correctness of perl character ops in the non utf8 case] One thought I had
> was that we could expose the server_encoding to the plperl interpreters in a
> special variable to make it easy to explicitly decode...
Should not need to do anything as complicated as that. Can just encode
the string to utf8 before we hand it off to perl.
[...]
> $ perl -MURI::Escape -e'print
> length(uri_unescape(q{comment%20passer%20le%20r%C3%A9veillon}))'
> 28
>
> $ perl -MEncode -MURI::Escape -e'print
> length(decode_utf8(uri_unescape(q{comment%20passer%20le%20r%C3%A9veillon})))'
> 27
[...]
> As shown above, the character length for the example should be 27, while the
> octet length for the UTF-8 encoded version is 28. I've reviewed the source
> of URI::Escape, and can say definitively that: a) regular uri_escape does not
> handle > 255 code points in the encoding, but there exists a uri_escape_utf8
> which will convert the source string to UTF8 first and then escape the
> encoded value, and
And why should it? properly escaped URIs should have all those
escaped, I imagine. Anyway not really relevant for postgres.
> b) uri_unescape has *no* logic in it to automatically decode from UTF8 into
> perl's internal format (at least as far as the version that I'm looking at,
> which came with 5.10.1).
>>> Either uri_unescape() should be decoding that utf8() or you need
>>> to do it *after* you call uri_unescape(). Hence the maybe it could be
>>> considered a bug in uri_unescape().
>>
>> Agreed.
>
> -1; if you need to decode from an octets-only encoding, it's your
> responsibility to do so after you've unescaped it.
-1? thats basically what I said: "... you need to do it (decode the
utf8) *after* you call uri_unescape"
> Perhaps later versions of the URI::Escape module contain a
> uri_unescape_utf8() function, but it's trivially: sub uri_unescape_utf8 {
> Encode::decode_utf8(uri_unescape(shift))}. This is definitely not a bug in
> uri_escape, as it is only defined to return octets.
Ahh So -1 because I said maybe you could call it a bug in
uri_unescape(). Really, I was only saying you *might* be able to
consider it a bug-- or perhaps deficiency is a better word, in
uri_unescape iff URI's are defined to have escaped characters as a %
escaped utf8 sequence. I dont know that they do, so I don't know if
its a bug :)
--
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] plperlu problem with utf8
On Fri, Dec 17, 2010 at 18:22, David E. Wheeler wrote:
> On Dec 17, 2010, at 5:04 PM, David E. Wheeler wrote:
>
>>> see? Either uri_unescape() should be decoding that utf8() or you need
>>> to do it *after* you call uri_unescape(). Hence the maybe it could be
>>> considered a bug in uri_unescape().
>>
>> Agreed.
>
> On second thought, no. You can in fact encode anything in a URI. URI::Escape
> can't know what to decode to. So *I believe* it just unescapes the raw bytes.
> It might be handy for it to have a new function, though, to complement its
> uri_escape_utf() function:
>
> sub uri_unescape_utf8 { Encode::decode_utf8(uri_unescape(@_)) }
>
> Just to make things a bit clearer.
>
> But that's a separate issue from the, erm, inconsistency with which PL/Perl
> treats encoding and decoding of its inputs and outputs.
Yay! So I think we can finally agree that for Oleg's original test
case postgres was getting right. I hope ? :)
--
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] plperlu problem with utf8
On Fri, Dec 17, 2010 at 18:04, David E. Wheeler wrote:
> On Dec 16, 2010, at 8:39 PM, Alex Hunsaker wrote:
> Yeah. So I just wrote and tested this function on 9.0 with Perl 5.12.2:
>
> CREATE OR REPLACE FUNCTION perlgets(
> TEXT
> ) RETURNS TABLE(length INT, is_utf8 BOOL) LANGUAGE plperl AS $$
> my $text = shift;
> return_next {
> length => length $text,
> is_utf8 => utf8::is_utf8($text) ? 1 : 0
> };
> $$;
>
> In a utf-8 database:
>
> utf8=# select * from perlgets('foo');
> length │ is_utf8
> ┼─
> 8 │ t
> (1 row)
>
>
> In a latin-1 database:
>
> latin=# select * from perlgets('foo');
> length │ is_utf8
> ┼─
> 8 │ f
> (1 row)
>
> I would argue that in the latter case, is_utf8 should be true, too. That is,
> PL/Perl should decode from Latin-1 to Perl's internal form.
Just to reiterate in a different way what David C. said, the flag is
irrelevant in this case. Begin set on that input string is the same as
it not being set.
per perldoc perlunicode:
The "UTF8" flag being on does not mean that there are any characters
of code points greater than 255 (or 127) in the scalar or that there
are even any characters in the scalar. What the "UTF8" flag means is
that the sequence of octets in the representation of the scalar is the
sequence of UTF-8 encoded code points of the characters of a string.
The "UTF8" flag being off means that each octet in this representation
encodes a single character with code point 0..255 within the string.
Basically perl has *2* internal forms and certain strings can be
represented in both.
> Interestingly, when I created a function that takes a bytea argument, utf8
> was *still* enabled in the utf-8 database. That doesn't seem right to me.
Hrm, yeah that seems bogus. Ill have to play with that more.
--
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] plperlu problem with utf8
On Sat, Dec 18, 2010 at 20:29, David E. Wheeler wrote:
> On Dec 17, 2010, at 9:32 PM, David Christensen wrote:
> latin=# SELECT * FROM perlgets('“hello”');
> length │ is_utf8
> ┼─
> 11 │ f
>
> (Yes I used Latin-1 curly quotes in that last example).
Erm, latin1 does not have curly quotes, Windows-1252 does. Those are
utf8 quotes AFAICT so 11 is actually right (thats 3 bytes per quote so
that where 11 comes from). If latin1 did have quotes and you used
them you would have gotten the same answer as latin1 is a single byte
encoding. :) I think your terminal tripping you up here.
Postgres also gives the same length:
latin1=# select length('“hello”');
length
11
(1 row)
--
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] plperlu problem with utf8
On Sat, Dec 18, 2010 at 20:29, David E. Wheeler wrote:
> ...
> I would argue that it should output the same as the first example. That is,
> PL/Perl should have decoded the latin-1 before passing the text to the Perl
> function.
Yeah, I don't think you will find anyone who disagrees :) PL/TCL and
PL/Python get this right FWIW. Anyway find attached a patch that does
just this.
With the attached we:
- for function arguments, convert (using pg_do_encoding_conversion) to
utf8 from the current database encoding. We also turn on the utf8
flag so perl knows it was given utf8. Pre patch things only really
worked for SQL_ASCII or PG_UTF8 databases. In practice everything
worked fine for single byte charsets. However things like uc() or
lc() on bytes with high bits set were probably broken.
- for output from perl convert from perls internal format to utf8
(using SvPVutf8()), and then convert that to the current database
encoding. This sounds unoptimized, but in the common case SvPVutf8()
should be a noop. Pre patch this was "random" (dependent on how perl
decided to represent the string internally) but it worked 99% of the
time (at least in the single byte charset or UTF8 cases).
- fix errors so they properly encode their message to the current
database encoding (pre patch we were doing no conversion at all,
similar to the output case were it worked most of the time)
- always do the utf8 hack so utf8.pm is loaded (fixes utf8 regexs in
plperl). Pre patch this only happened on a UTF8 database. That meant
multi-byte character regexs were broken on non utf8 databases.
-remove some old perl version checks for 5.6 and 5.8. We require
5.8.1 so these were nothing but clutter.
Something interesting to note is when we are SQL_ASCII,
pg_do_encoding_conversion() does nothing, yet we turn on the utf8
flag. This means if you pass in valid utf8 perl will treat it as
such. It also means on output it will hand utf8 back. Both PL/Tcl
and PL/Python do the same thing so I suppose its sensible to match
their behavior (and it was the lazy thing to do). The difference
being with PL/Python if you return said string you get "ERROR:
PL/Python: could not convert Python Unicode object to PostgreSQL
server encoding". While PL/Tcl and now Pl/perl give you back a utf8
version. For example:
(SQL_ASCII database)
=# select length('☺');
length
3
=# CREATE FUNCTION tcl_len(text) returns text as $$ return [string
length $1] $$ language pltcl;
CREATE FUNCTION
postgres=# SELECT tcl_len('☺');
tcl_len
1
(1 row)
=# CREATE FUNCTION py_len(str text) returns text as $$ return
len(str) $$ language plpython3;
=# SELECT py_len('☺');
py_len
1
(1 row)
I wouldn't really say thats right, but its at least consistent...
This does *not* address the bytea issue where currently if you have
bytea input or output we try to encode that the same as any string. I
think thats going to be a bit more invasive and this patch should
stands on its own.
plperl_fix_enc.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] plperlu problem with utf8
On Sun, Dec 19, 2010 at 21:00, David Christensen wrote: > > On Dec 19, 2010, at 2:20 AM, Alex Hunsaker wrote: > >> With the attached we: >> - for function arguments, convert (using pg_do_encoding_conversion) to >> utf8 from the current database encoding. > > How does this deal with input records of composite type? Same way it worked before just encoded properly. :) >> - fix errors so they properly encode their message to the current >> database encoding > >[...] does it degrade to the current behavior, as opposed to fail or eat the >error message without outputting? No it fails with an "invalid encoding message". I think thats the best we can do. It certainly seems wrong to send invalid bytes back to the client. >> - always do the utf8 hack so utf8.pm is loaded (fixes utf8 regexs in >> plperl). [...] > > This sounds good in general, but I think should be skipped if > GetDatabaseEncoding() == SQL_ASCII. Why? What if you want to use a module that does utf8 things? >> [ PLs handling strings as utf8 in SQL_ASCII databases ] > I think this could/should be adequately handled by not calling the function > when the DatabaseEncoding is SQL_ASCII. [...] > Also, I'd argue that pltcl > and plpython should be fixed as well for the same reasons. I don't disagree, but im not volunteering for it either. Without having done any in depth analysis, the largest problem is they use strings for most arguments. Strings need an encoding and pl/python and pl/tcl want a utf8 string. So we need to convert from SQL_ASCII to UTF8. Which like you say is nonsensical. So really in the SQL_ASCII case we would need to switch to binary datatypes for those languages. Perl is a bit more flexible so it should be easier to 'fix'. >> This does *not* address the bytea issue where currently if you have >> bytea input or output we try to encode that the same as any string. I >> think thats going to be a bit more invasive and this patch should >> stands on its own. >> > > Yeah, I'm not sure how invasive that will end up being, or if there are other > datatypes which should skip the text processing. Well, I don't think it will be too bad. As for examples of other datatypes that we could skip text processing: int, float and numeric. Right now we treat them as strings, which works fine for perl... but its a tad inefficient. Ideally we could be using things like SvIV. > I noticed you moved the declaration of perl_sys_init_done; was that an > independent bug fix, or did something in the patch require that? It just squashes an unused variable "perl_sys_init_done" warning when perl is compiled with its own malloc. Of course that added another warning "warning: ISO C90 forbids mixed declarations and code" :P In further review over caffeine this morning I noticed there are a few places I missed: plperl_build_tuple_result(), plperl_modify_tuple() and Util.XS. The first 2 pass perl hash keys for the column straight to SPI_fnumber() without doing any conversion. In theory this means we might not be able to look up columns with non ascii characters. I missed this because those hash keys are declared as char* instead of SV* and do not use any of the standard perl string macros. Anyway I hope to have a fresh patch tomorrow. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
