Re: [HACKERS] Current int & float overflow checking is slow.

[Dagfinn Ilmari Mannsåker]

Robert Haas  writes:

>> 0003) Removes -fwrapv. I'm *NOT* suggesting we apply this right now, but
>>   it seems like an important test for the new facilities. Without
>>   0002, tests would fail after this, after it all tests run
>>   successfully.
>
> I suggest that if we think we don't need -fwrapv any more, we ought to
> remove it.  Otherwise, we won't find out if we're wrong.

Without -fwrapv signed overflow is undefined behaviour.  We should test
thoroughly with -ftrapv or -fsanitize=signed-integer-overflow to be
confident the code is free of such things.  We might even want to enable
-ftrapv by default in cassert-enabled builds.

- ilmari
-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


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

[HACKERS] [PATCH] Comment typo in get_collation_name() comment

[Dagfinn Ilmari Mannsåker]

Hi Hackers,

The comment for get_collation_name() seems to have been copy-pasted from
get_constraint_name(), but missed one s/constraint/collation/.

Patch attached.

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 48961e31aa..acbd9ac63e 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -958,7 +958,7 @@ get_atttypetypmodcoll(Oid relid, AttrNumber attnum,
  * get_collation_name
  *		Returns the name of a given pg_collation entry.
  *
- * Returns a palloc'd copy of the string, or NULL if no such constraint.
+ * Returns a palloc'd copy of the string, or NULL if no such collation.
  *
  * NOTE: since collation name is not unique, be wary of code that uses this
  * for anything except preparing error messages.

-- 
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] coverage analysis improvements

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:

> On 9/20/17 13:13, Dagfinn Ilmari Mannsåker wrote:
>> I have no opinion on the bulk of this patch set, but skimming it out of
>> curiosity I noticed that the plperl change seems to have lost the
>> dependency on plperl_helpers.h from the xsubpp targets:
>
> Those commands don't actually require plperl_helpers.h, do they?

No, but the .xs files #include it.  I guess it's SPI.o and Util.o that
should depend on the header..

- ilmari
-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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] coverage analysis improvements

[Dagfinn Ilmari Mannsåker]

Hi Peter,

Peter Eisentraut  writes:

> OK, I was not aware that people are using it that way.  So updated patch
> set there, which separates coverage and coverage-html into two
> independent targets.

I have no opinion on the bulk of this patch set, but skimming it out of
curiosity I noticed that the plperl change seems to have lost the
dependency on plperl_helpers.h from the xsubpp targets:

> diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile
> index 191f74067a..66a2c3d4c9 100644
> --- a/src/pl/plperl/GNUmakefile
> +++ b/src/pl/plperl/GNUmakefile
> @@ -81,13 +81,9 @@ perlchunks.h: $(PERLCHUNKS)
>  
>  all: all-lib
>  
> -SPI.c: SPI.xs plperl_helpers.h
> +%.c: %.xs
>   @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch 
> --with-perl was not specified."; exit 1; fi
> - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap 
> $(perl_privlibexp)/ExtUtils/typemap $< >$@
> -
> -Util.c: Util.xs plperl_helpers.h
> - @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch 
> --with-perl was not specified."; exit 1; fi
> - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap 
> $(perl_privlibexp)/ExtUtils/typemap $< >$@
> + $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap 
> $(perl_privlibexp)/ExtUtils/typemap -output $@ $<
>  
>  
>  install: all install-lib install-data

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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] Show backtrace when tap tests fail

[Dagfinn Ilmari Mannsåker]

Craig Ringer  writes:

> On 20 September 2017 at 06:36, David Steele  wrote:
>
>>
>> I just use:
>>
>> $SIG{__DIE__} = sub {Carp::confess @_};

That is the basic idea behind both Carp::Always and Devel::Confess, but
they also avoid breaking non-string exceptions (references and objects)
being thrown and caught.  Devel::Confess jumps through even more hoops
to add backtraces to these without breaking code catching them.

See the DESCRIPTION and CAVEATS sections (and the source, if you've got
a strong stomach) of https://metacpan.org/pod/Devel::Confess for some of
the hairy details.

> That's what I patched into my TestLib.pm too, until I learned of
> Carp::Always.
>
> I'd rather have Carp::Always, but it's definitely an OK fallback.

I agree with Tom that we should just go for Devel::Confess with no
fallbacks (because of the above-mentioned potential breakage).  This is
mainly for the convenience of developers and the buildfarm, not end
users.

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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] Show backtrace when tap tests fail

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> Andrew Dunstan  writes:
>> On 09/19/2017 01:31 PM, Andres Freund wrote:
>>> # Include module showing backtraces upon failures. As it's a
>>> non-standard module, don't fail if not installed.
>>> eval { use Carp::Always; }
>
>> Or maybe Devel::Confess ?
>
> Neither one seems to be present in a standard Perl installation :-(

No, hence making it optional via eval { }.  That way we can get more
useful output from the buildfarm (and Travis) by installing it there,
without imposing extra dependencies on end users.

We already depend on one non-core module (IPC::Run), so presumably we
could add others, but I presume the threshold for that is quite high.

- ilmari
-- 
"a disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
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] Show backtrace when tap tests fail

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> On 09/19/2017 01:31 PM, Andres Freund wrote:
>> Hi,
>>
>> I've had a couple cases where tap tests died, and I couldn't easily see
>> where / why. For development of a new test I found it useful to show
>> backtraces in that case - just adding a
>> use Carp::Always;
>> at the start of the relevant module did the trick.
>>
>> I'm wondering if we shouldn't always do so if the module is
>> installed. I.e. have PostgresNode or something do something like

I think TestLib would be a better place, since PostgresNode uses TestLib
and there are some tests that use TestLib but notf PostgresNode.

>> # Include module showing backtraces upon failures. As it's a
>> non-standard module, don't fail if not installed.
>> eval { use Carp::Always; }
>>
>> Comments?
>
> Or maybe Devel::Confess ?

Devel::Confess is more thorough, so +1 on that.

> In an eval you need a 'require' rather than a 'use', AFAIK.

Yes, because 'use' happens as soon as the statement is parsed at compile
time, long before the eval { } gets executed.  Devel::Confess does its
setup in the 'import' method (called implicitly by 'use'), so we'd need:

  eval { require Devel::Confess; Devel::Confess->import };

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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] proposal: psql: check env variable PSQL_PAGER

[Dagfinn Ilmari Mannsåker]

Pavel Stehule  writes:

> Hi
>
> I wrote a special pager for psql. Surely, this pager is not good for paging
> of man pages. So is not good to set it as global pager. We can introduce
> new env variable PSQL_PAGER for this purpose. It can work similar like
> PSQL_EDITOR variable.
>
> Notes, comments?

There's already the option to put '\setenv PAGER ...' in .psqlrc, but I
guess having it work like EDITOR makes sense for consistency, as well as
not affecting commands spawned from psql via e.g. \!.

- ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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] renaming "transaction log"

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut  writes:

> Committed, adjusting for some of the suggestions.

Looks like one occurrence was still left in:

$ ag --no-group --ignore po --ignore release-\* --ignore wal.sgml 
'\btransaction\s+log\b'
doc/src/sgml/func.sgml:18631:end of the transaction log at any instant, 
while the write location is the end of

The four other occurrences in the same paragraph were fixed, so I assume
this was just an oversight.

- ilmari
-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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] password_encryption, default and 'plain' support

[Dagfinn Ilmari Mannsåker]

Heikki Linnakangas  writes:

> On 05/06/2017 01:56 PM, Gavin Flower wrote:
>> On 06/05/17 22:44, Vik Fearing wrote:
>>> On 05/05/2017 02:42 PM, Michael Paquier wrote:
 +This option is obsolete but still accepted for backwards
 +compatibility.
 Isn't that incorrect English?
>>> No.
>>>
 It seems to me that this be non-plural,
 as "for backward compatibility".
>>> "Backwards" is not plural, it's a regional variation of "backward" (or
>>> vice versa depending on which region you come from).  Both are correct.
>>
>> I am English, born & bred, and 'Backwards' feels a lot more natural to me.
>
> Another data point:
>
> $ grep "backwards-comp" doc/src/sgml/ref/*.sgml doc/src/sgml/*.sgml | wc -l
> 7
> $ grep "backward-comp" doc/src/sgml/ref/*.sgml doc/src/sgml/*.sgml | wc -l
> 3
>
> Another important question is whether there should be a hyphen there or not?
>
> $ grep   "backward comp" doc/src/sgml/ref/*.sgml doc/src/sgml/*.sgml  |
> wc -l
> 21
> ~/git-sandbox-pgsql/master (master)$ grep   "backwards comp"
> doc/src/sgml/ref/*.sgml doc/src/sgml/*.sgml  | wc -l
> 11

This misses out instances where the phrase is broken across lines.
Using a less line-orented tool, ag (aka. The Silver Searcher), gives
more hits for the non-hyphenated case, but doesn't significantly change
the s-to-non-s ratio.

$ ag 'backwards-\s*comp' doc/**/*.sgml|grep -c backward
7
$ ag 'backward-\s*comp' doc/**/*.sgml|grep -c backward
3
$ ag 'backward\s+comp' doc/**/*.sgml | grep -c backward
29
* ag 'backwards\s+comp' doc/**/*.sgml | grep -c backward
20


-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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] PG 10 release notes

[Dagfinn Ilmari Mannsåker]

Bruce Momjian  writes:

> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.

I noticed a few niggles with the links in "my" entires, patch attached.

Cheers,

ilmari

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

>From a32336f420a4fb25bdedeb0dc8ccf68503638e93 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 27 Apr 2017 15:20:59 +0100
Subject: [PATCH] Fix some release note links

- Make max_pred_locks_per_page a link too
- The ALTER TYPE link was pointing to the ALTER TABLE page
---
 doc/src/sgml/release-10.sgml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml
index de2129758c..6f49618d07 100644
--- a/doc/src/sgml/release-10.sgml
+++ b/doc/src/sgml/release-10.sgml
@@ -665,7 +665,7 @@

 The new settings are  and
-   max_pred_locks_per_page.
+.

   
 
@@ -1830,7 +1830,7 @@
   
 
   
-   This uses the syntax ALTER
+   This uses the syntax ALTER
TYPE ... RENAME VALUE.
   
  
-- 
2.11.0


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

Re: [HACKERS] On-disk format of SCRAM verifiers

[Dagfinn Ilmari Mannsåker]

Michael Paquier  writes:

> On Fri, Apr 21, 2017 at 10:02 PM, Simon Riggs  wrote:
>> On 21 April 2017 at 10:20, Heikki Linnakangas  wrote:
>>> But looking more closely, I think I misunderstood RFC 5803. It *does* in
>>> fact specify a single string format to store the verifier in. And the format
>>> looks like:
>>>
>>> SCRAM-SHA-256$:$:
>>
>> Could you explain where you are looking? I don't see that in RFC5803
>
> From 1.  Overview:
>
>Syntax of the attribute can be expressed using ABNF [RFC5234].  Non-
>terminal references in the following ABNF are defined in either
>[AUTHPASS], [RFC4422], or [RFC5234].
>
>scram-mech = "SCRAM-SHA-1" / scram-mech-ext
>   ; Complies with ABNF for 
>   ; defined in [AUTHPASS].
>
>scram-authInfo = iter-count ":" salt
>   ; Complies with ABNF for 
>   ; defined in [AUTHPASS].
>
>scram-authValue = stored-key ":" server-key
>   ; Complies with ABNF for 
>   ; defined in [AUTHPASS].

And scram-mech, scram-authInfo and scram-authValue are used as the
"scheme", "authInfo" and "authValue" parts as specified in [AUTHPASS]
(RFC3112):

authPasswordValue   = w scheme s authInfo s authValue w
scheme  = %x30-39 / %x41-5A / %x2D-2F / %x5F
; 0-9, A-Z, "-", ".", "/", or "_"
authInfo= schemeSpecificValue
authValue   = schemeSpecificValue
schemeSpecificValue = *( %x21-23 / %x25-7E )
; printable ASCII less "$" and " "
s   = w SEP w
w   = *SP
SEP = %x24 ; "$"
SP  = %x20 ; " " (space)

> Thanks,
> -- 
> Michael


- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
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] Cutting initdb's runtime (Perl question embedded)

[Dagfinn Ilmari Mannsåker]

Tom Lane <t...@sss.pgh.pa.us> writes:

> Andres Freund <and...@anarazel.de> writes:
>> On 2017-04-17 17:49:54 +0100, Dagfinn Ilmari Mannsåker wrote:
>>> I threw Devel::NYTProf at it and picked some more low-hanging fruit.
>
>> I'm a bit doubtful about improving the performance of genbki at the cost
>> of any sort of complication - it's only executed during the actual
>> build, not during initdb...  I don't see much point in doing things like
>> 3) and 4), it's just not worth it imo.
>
> Yeah, it's only run once per build, or probably even less often than that
> considering we only re-run it when the input files change.  I could get
> interested in another 20% reduction in initdb's time, but not genbki's.

Fair enough.  I still think 1), 2) and 5) are worthwile code cleanups
regardless of the performance impact.  In fact, if we don't care that
much about the performance, we can move the duplicated code in
Gen_fmgrmtab.pl and genbki.pl that turns bki_values into a hash into
Catalogs.pm.  That regresses genbki.pl time by ~30ms on my machine.

Revised patch series attached.

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

>From f7b75bcbe993d4ddbc92da85c7148bf7fee143ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Mon, 17 Apr 2017 13:26:35 +0100
Subject: [PATCH 1/4] Avoid unnecessary regex captures in Catalog.pm

---
 src/backend/catalog/Catalog.pm | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index fa8de04..e7b647a 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -54,7 +54,7 @@ sub Catalogs
 		{
 
 			# Strip C-style comments.
-			s;/\*(.|\n)*\*/;;g;
+			s;/\*(?:.|\n)*\*/;;g;
 			if (m;/\*;)
 			{
 
@@ -80,12 +80,12 @@ sub Catalogs
 			{
 $catalog{natts} = $1;
 			}
-			elsif (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
+			elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
-check_natts($filename, $catalog{natts}, $3,
+check_natts($filename, $catalog{natts}, $2,
 			$input_file, $input_line_number);
 
-push @{ $catalog{data} }, { oid => $2, bki_values => $3 };
+push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
 			{
-- 
2.7.4

>From decd1b8c171cc508da5f79f01d2f0779a569a963 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Mon, 17 Apr 2017 14:04:20 +0100
Subject: [PATCH 2/4] Avoid repeated calls to Catalog::SplitDataLine

Both check_natts and the callers of Catalog::Catalogs were calling
Catalog::SplitDataLines, the former discarding the result.

Instead, have Catalog::Catalogs store the split fields directly and pass
the count to check_natts
---
 src/backend/catalog/Catalog.pm   | 14 +++---
 src/backend/catalog/genbki.pl|  3 +--
 src/backend/utils/Gen_fmgrtab.pl |  3 +--
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e7b647a..81513c7 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -82,10 +82,12 @@ sub Catalogs
 			}
 			elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
-check_natts($filename, $catalog{natts}, $2,
+my @bki_values = SplitDataLine($2);
+
+check_natts($filename, $catalog{natts}, scalar(@bki_values),
 			$input_file, $input_line_number);
 
-push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
+push @{ $catalog{data} }, { oid => $1, bki_values => \@bki_values };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
 			{
@@ -254,17 +256,15 @@ sub RenameTempFile
 # verify the number of fields in the passed-in DATA line
 sub check_natts
 {
-	my ($catname, $natts, $bki_val, $file, $line) = @_;
+	my ($catname, $natts, $bki_vals, $file, $line) = @_;
 
 	die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"
 		unless defined $natts;
 
-	my $nfields = scalar(SplitDataLine($bki_val));
-
 	die sprintf
 		"Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",
-		$file, $line, $natts, $nfields
-	  unless $natts == $nfields;
+		$file, $line, $natts, $bki_vals
+	  unless $natts == $bki_vals;
 }
 
 1;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 198e072..8875f6c 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -161,9 +161,8 @@ foreach my $catname (@{ $catalogs->{names} })
 		for

Re: [HACKERS] Cutting initdb's runtime (Perl question embedded)

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> There's certainly lots more that could be done in the genbki code,
> but I think all we can justify at this stage of the development
> cycle is to get the low-hanging fruit for testing speedups.

I threw Devel::NYTProf at it and picked some more low-hanging fruit.
Attached are separate patches for each change, and here are the runtimes
of genbki.pl and Gen_fmgrtab.pl, respectively, after each patch
(averages of 5 runs, in millseconds):

master (b6dd1271): 355, 182

1: Avoid unnecessary regex captures: 349, 183
2: Avoid repeated calls to SplitDataLine: 316, 158
3: Inline SplitDataLine: 291, 141
4: Inline check_natts: 287, 141

Together they shave 68ms or 19.2% off the runtime of genbki.pl and 41ms
or 22.5% off the runtime of Gen_fmgrtab.pl

Finally, one non-performance patch, which just removes the use of
Exporter in Catalog.pm, since none of the users actually import anything
from it.

- ilmari
-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen

>From 5d7f4b1e78243daa6939501b88b2644bfcf9bd77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 17 Apr 2017 13:26:35 +0100
Subject: [PATCH 1/8] Avoid unnecessary regex captures in Catalog.pm

---
 src/backend/catalog/Catalog.pm | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index fa8de04..e7b647a 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -54,7 +54,7 @@ sub Catalogs
 		{
 
 			# Strip C-style comments.
-			s;/\*(.|\n)*\*/;;g;
+			s;/\*(?:.|\n)*\*/;;g;
 			if (m;/\*;)
 			{
 
@@ -80,12 +80,12 @@ sub Catalogs
 			{
 $catalog{natts} = $1;
 			}
-			elsif (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
+			elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
-check_natts($filename, $catalog{natts}, $3,
+check_natts($filename, $catalog{natts}, $2,
 			$input_file, $input_line_number);
 
-push @{ $catalog{data} }, { oid => $2, bki_values => $3 };
+push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
 			{
-- 
2.7.4

>From 92402e0306eda209b1a2811acc41325d75add0cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 17 Apr 2017 14:04:20 +0100
Subject: [PATCH 2/8] Avoid repeated calls to Catalog::SplitDataLine

Both check_natts and the callers of Catalog::Catalogs were calling
Catalog::SplitDataLines, the former discarding the result.

Instead, have Catalog::Catalogs store the split fields directly and pass
the count to check_natts
---
 src/backend/catalog/Catalog.pm   | 14 +++---
 src/backend/catalog/genbki.pl|  3 +--
 src/backend/utils/Gen_fmgrtab.pl |  3 +--
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e7b647a..0c508b0 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -82,10 +82,12 @@ sub Catalogs
 			}
 			elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
-check_natts($filename, $catalog{natts}, $2,
+my @bki_values = SplitDataLine($2);
+
+check_natts($filename, $catalog{natts}, scalar(@bki_values),
 			$input_file, $input_line_number);
 
-push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
+push @{ $catalog{data} }, { oid => $1, bki_values => \@bki_values };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
 			{
@@ -254,17 +256,15 @@ sub RenameTempFile
 # verify the number of fields in the passed-in DATA line
 sub check_natts
 {
-	my ($catname, $natts, $bki_val, $file, $line) = @_;
+	my ($catname, $natts, $bki_vals, $file, $line) = @_;
 
 	die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"
 		unless defined $natts;
 
-	my $nfields = scalar(SplitDataLine($bki_val));
-
 	die sprintf
 		"Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",
-		$file, $line, $natts, $nfields
-	  unless $natts == $nfields;
+		$file, $line, $natts, $bki_vals
+	unless $natts == $bki_vals;
 }
 
 1;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 198e072..8875f6c 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -161,9 +161,8 @@ foreach my $catname (@{ $catalogs->{names} })
 		foreach my $row (@{ $catalog->{data} })
 		{
 
-			# Split line into tokens without interpreting their meaning.
 			my %bki_values;
-			@bki_values{@attnames} = Catalog::SplitDataLine($row->{bki_values});
+			@bki_values{@attnames} = @{$row->{bki_values}};
 
 			# Perform required substitutions on fields
 			foreach my $att (keys %bki_values)
diff --git 

Re: [HACKERS] Compiler warning in costsize.c

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> I wonder if we shouldn't just do
>
>   RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
>   ListCell   *lc;
>  
>   /* Should only be applied to base relations that are subqueries */
>   Assert(rel->relid > 0);
> -#ifdef USE_ASSERT_CHECKING
>   rte = planner_rt_fetch(rel->relid, root);
>   Assert(rte->rtekind == RTE_SUBQUERY);
> -#endif
>
> and eat the "useless" calculation of rte.

Why bother with the 'rte' variable at all if it's only used for the
Assert()ing the rtekind?

Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY);

- ilmari

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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 GUCs for predicate lock promotion thresholds

[Dagfinn Ilmari Mannsåker]

Kevin Grittner  writes:

> Unfortunately, I was unable to get the follow-on patch to allow
> setting by relation into a shape I liked.  Let's see what we can do
> for the next release.

Okay, I'll try and crete a more comprehensive version of it for the next
commitfest.

> The first patch was applied with only very minor tweaks.

Thanks!

> I realize that nothing would break if individual users could set their
> granularity thresholds on individual connections, but as someone who
> has filled the role of DBA, the thought kinda made my skin crawl.  I
> left it as PGC_SIGHUP for now; we can talk about loosening that up
> later, but I think we should have one or more use-cases that outweigh
> the opportunities for confusion and bad choices by individual
> programmers to justify that.

I agree.  The committed version is fine for my current use case.

 - ilmari

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
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] Reduce src/test/recovery verbosity

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut  writes:

> On 3/28/17 23:42, Michael Paquier wrote:
>> src/bin/pg_dump and src/test/modules/test_pgdump generate too much
>> output. If we could get tests to only print the final result, like how
>> many tests done and how many have passed, things would be much
>> friendlier.
>
> There are options to change the verbosity of things.
>
> I don't think I would like changing the default verbosity.  At least the
> precedent from the pg_regress type tests is that there should be some
> action on the screen as things run.

Non-verbose prove still lists each test script, it just doesn't list
each individual test within the script.

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
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: Clean up Perl code according to perlcritic

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:

> On 03/28/2017 05:23 AM, Dagfinn Ilmari Mannsåker wrote:
>> +@opts = grep { !/\$\(/ && /^--/ }
>> +map { (my $x = $_) =~ 
>> s/\Q$(top_builddir)\E/\"$topdir\"/; $x;}
>> +split(/\s+/, $1);
>>  
>
> The use of this lexical $x variable seems entirely pointless and
> obfuscatory. If perlcritic doesn't like it without then that's another
> black mark against it IMNSHO.

The lexical copy is not strictly necessary in this case, because the
values we're mapping over are mutable temporaries returned by split().

An alternative form would be:

@opts = grep { !/\$\(/ && /^--/ }
map { s/\Q$(top_builddir)\E/\"$topdir\"/; $_ }
split(/\s+/, $1);

Perlcritic complains about that, though:

src/tools/msvc/vcregress.pl: Don't modify $_ in list functions at line
524, column 4.  See page 114 of PBP.  (Severity: 5)

This for two reasons:

1) If we were mapping over an array varible it would modify the original
   values in-place (since $_ is an alias, not a copy).

2) If the list were to contain read-only items it'd throw a
   "Modification of a read-only value attempted" exception.

The /r modifier was introduced in perl 5.14 exactly for this reason: it
makes the substitution non-destructive and returns the modified string
instead.  However, we still support perls as ancient as 5.8, so we can't
use that.

 - ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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: Clean up Perl code according to perlcritic

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> I would try something like this:
>
> @opts = grep { $_ !~ /\$\(/ && $_ =~ /^--/  } 
>   map { s/\Q$(top_builddir)\E/\"$topdir\"/; }
>   split(/\s+/, $1);

That map is not going to work: it'll modify the values returned by
split(), but s/// (without the /r modifier, which was added in 5.14)
returns the number of substitutions made, not the modified string.

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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: Clean up Perl code according to perlcritic

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> I wrote:
>> Peter Eisentraut  writes:
>>> Clean up Perl code according to perlcritic
>
>> This seems to have broken the regression tests (specifically, dblink)
>> on at least some of the Windows buildfarm critters.
>
> I'm hardly a Perl expert, but it looks to me like the culprit is this
> hunk in vcregress.pl:
>
> @@ -521,8 +521,9 @@ sub fetchRegressOpts
>   # an unhandled variable reference.  Ignore anything that isn't 
> an
>   # option starting with "--".
>   @opts = grep {
> - s/\Q$(top_builddir)\E/\"$topdir\"/;
> - $_ !~ /\$\(/ && $_ =~ /^--/
> + my $x = $_;
> + $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
> + $x !~ /\$\(/ && $x =~ /^--/
>   } split(/\s+/, $1);
>   }
>   if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
>
> The first line in that block is actually intending to modify the value
> it's inspecting, and perlcritic's "improved" version carefully removes
> the side-effect.
>
> No doubt there are cleaner ways to do that (the comments in "man perlfunc"
> about this coding technique are not positive), but this way is not
> cleaner, it is broken.

I suggest splitting the substitution and filtering part into separate
map and grep calls, for clarity.  The substituion is crying out for the
/r regex modifier to avoid the local variable, but that's only available
since perl 5.14.

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index d9367f8..cf93a60 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -520,11 +520,9 @@ sub fetchRegressOpts
# Substitute known Makefile variables, then ignore options that 
retain
# an unhandled variable reference.  Ignore anything that isn't 
an
# option starting with "--".
-   @opts = grep {
-   my $x = $_;
-   $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
-   $x !~ /\$\(/ && $x =~ /^--/
-   } split(/\s+/, $1);
+   @opts = grep { !/\$\(/ && /^--/ }
+   map { (my $x = $_) =~ 
s/\Q$(top_builddir)\E/\"$topdir\"/; $x;}
+   split(/\s+/, $1);
}
if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
{


-  ilmari

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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] perlcritic

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:

> On 3/1/17 11:21, Dagfinn Ilmari Mannsåker wrote:
>> diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
>> index 292c9101c9..b4212f5ab2 100644
>> --- a/src/pl/plperl/plc_perlboot.pl
>> +++ b/src/pl/plperl/plc_perlboot.pl
>> @@ -81,18 +81,15 @@ sub ::encode_array_constructor
>>  } sort keys %$imports;
>>  $BEGIN &&= "BEGIN { $BEGIN }";
>>  
>> -return qq[ package main; sub { $BEGIN $prolog $src } ];
>> +# default no strict and no warnings
>> +return qq[ package main; sub { no strict; no warnings; $BEGIN 
>> $prolog $src } ];
>>  }
>>  
>>  sub mkfunc
>>  {
>> -## no critic (ProhibitNoStrict, ProhibitStringyEval);
>> -no strict;  # default to no strict for the eval
>> -no warnings;# default to no warnings for the eval
>> -my $ret = eval(mkfuncsrc(@_));
>> +my $ret = eval(mkfuncsrc(@_)); ## no critic 
>> (ProhibitStringyEval);
>>  $@ =~ s/\(eval \d+\) //g if $@;
>>  return $ret;
>> -## use critic
>>  }
>>  
>>  1;
>
> I have no idea what this code does or how to test it, so I didn't touch it.

This code compiles a string of perl source into a subroutine reference.
It's is called by plperl_create_sub() in src/pl/plperl/plperl.c, which
is called whenever a plperl function needs to be compiled, i.e. during
CREATE FUNCTION (unless check_function_bodies is off) and when the
function is executed and the compiled form is not already cached in
plperl_proc_hash.

The change reduces the scope of the stricture and warning disablement to
just the compiled code, instead of the surrounding compiling code too.
Putting them inside the sub block has no runtime overhead, since they're
compile-time directives, not runtime statements.

It can be tested by creating a plperl function with a construct that
would fall foul of warnings or strictures, which
src/pl/plperl/sql/plperl_elog.sql does.

>> diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
>> index 64227c2dce..e2653f11d8 100644
>> --- a/src/tools/msvc/gendef.pl
>> +++ b/src/tools/msvc/gendef.pl
>> @@ -174,7 +174,7 @@ sub usage
>>  
>>  my %def = ();
>>  
>> -while (<$ARGV[0]/*.obj>)  ## no critic (RequireGlobFunction);
>> +while (glob($ARGV[0]/*.obj))
>>  {
>>  my $objfile = $_;
>>  my $symfile = $objfile;
>
> I think what this code is meant to do might be better written as a
> foreach loop.  Again, can't test it.

glob("...") is exactly equivalent to <...> (except when <...> parses as
readline, which is why Perl::Critic complains).

Writing it as 'for my $objfile (glob("$ARGV[0]/*.obj")) { ... }' would
be neater, I agree.

>> difff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
>> index a6b24b5348..51d6a28953 100755
>> --- a/src/tools/pgindent/pgindent
>> +++ b/src/tools/pgindent/pgindent
>> @@ -159,8 +159,7 @@ sub process_exclude
>>  while (my $line = <$eh>)
>>  {
>>  chomp $line;
>> -my $rgx;
>> -eval " \$rgx = qr!$line!;";  ## no critic 
>> (ProhibitStringyEval);
>> +my $rgx = eval { qr!$line! };
>>  @files = grep { $_ !~ /$rgx/ } @files if $rgx;
>>  }
>>  close($eh);
>
> After further thinking, I changed this to just
>
> my $rgx = qr!$line!;
>
> which works just fine.

That changes the behaviour from silently skipping invalid regular
expressions in the exclude file to dying on encountering one.  That
might be desirable, but should be done deliberately.

>> @@ -435,7 +434,8 @@ sub diff
>>  
>>  sub run_build
>>  {
>> -eval "use LWP::Simple;";  ## no critic (ProhibitStringyEval);
>> +require LWP::Simple;
>> +LWP::Simple->import(qw(getstore is_success));
>>  
>>  my $code_base = shift || '.';
>>  my $save_dir = getcwd();
>
> I think this is mean to not fail compilation if you don't have that
> module, so I left it as is.

Yes, it's using string eval to defer the compilation of the "use"
statement to runtime.  The require+import does exactly the same thing,
since they are run-time already, so won't be called until run_build is.

While looking at this again, I realised that the 'do' statement in
src/tools/msvc/install.pl will break on the upcoming perl 5.26, which

Re: [HACKERS] PL/Python: Add cursor and execute methods to plan object

[Dagfinn Ilmari Mannsåker]

Jim Nasby  writes:

> On 2/25/17 10:27 AM, Peter Eisentraut wrote:
>> So I'm also wondering here which style people prefer so
>> I can implement it there.
>
> I think the more OO style is definitely better. I expect it would
> simplify the code as well.

I'm not a Python person, but I'd argue that the "more OO" style should
be the primary style documented, and the old style should just be
mentioned for reference.

 - ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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] make check-world output

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> For the basic build process, we've largely solved that through the
> use of "make -s".  But we don't really have a comparable "be quiet"
> option for test runs, especially not the TAP tests.  Maybe we need
> to think a bit more globally about what it is we're trying ton
> accomplish.

Removing --verbose from PROVE_FLAGS cuts the check-world output from
about 9.4k lines to 3.8k, i.e. ~60%.  Doing 'make -s' cuts it by another
1.3k lines.

Another thing I noticed is that there's a bunch of 'diag' calls in the
tests scripts (particularly ssl/t/001_ssltests.pl and
recovery/t/001_stream_rep.pl) that should probably be 'note's instead,
so they don't pollute STDERR in non-verbose mode.  'diag' should only be
used to output extra diagnostics in the case of test failures, 'note' is
for test progress/status updates.

- ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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] Need a builtin way to run all tests faster manner

[Dagfinn Ilmari Mannsåker]

Magnus Hagander  writes:

> AFAIK travis-ci would require us to use github as our hoster for all those
> things, and embrace that workflow, they don't support anything else.
>
> There might be others that do, just not travis.

It merely requires the repository to exist on GitHub, and postgresql.git
is already mirrored to https://github.com/postgres/postgres.  If there
was a .travis.yml in the repo, people who fork it could easily enable
Travis-CI for their fork, even the official repo isn't hooked up.

- ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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] Teach Catalog.pm how many attributes there should be per DATA() line

[Dagfinn Ilmari Mannsåker]

Jeff Janes  writes:

> On Thu, Mar 9, 2017 at 3:20 PM, Robert Haas  wrote:
>
>> Committed.   Hopefully this doesn't contain any Perl bits that are
>> sufficiently new as to cause problems for our older BF members ... I
>> guess we'll see.
>
> Bad luck there.  I'm getting this error on CentOS6.8, perl v5.10.1
>
> Can't locate object method "input_line_number" via package "IO::Handle" at
[...]
> I think we can just save $. and use that, as in the attached.

Another option would have been to add an explicit 'use IO::Handle;',
which makes the OO interface available on filehandles on all versions
back to 5.8 (5.14 and newer do this automatically).

- ilmari

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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] Need a builtin way to run all tests faster manner

[Dagfinn Ilmari Mannsåker]

Simon Riggs  writes:

> On 7 March 2017 at 20:36, Alvaro Herrera  wrote:
>
>> FWIW, +1 on improving matters here.
>
> +1 also.
>
> I don't see what's wrong with relying on buildfarm though; testing is
> exactly what its there for.
>
> If we had a two-stage process, where committers can issue "trial
> commits" as a way of seeing if the build farm likes things. If they
> do, we can push to the main repo.

In perl we do this by having the smoke testers (build farm) pick up
branches with a specific prefix (smoke-me/ in our case, typically
smoke-me//), in addition to the blead (master) and
maint-x.y (release) branches.

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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] perlcritic

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> Hi Peter,
>
> Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:
>
>> I posted this about 18 months ago but then ran out of steam. [ ] Here
>> is an updated patch.  The testing instructions below still apply.
>> Especially welcome would be ideas on how to address some of the places
>> I have marked with ## no critic.
>
> Attached is a patch on top of yours that addresses all the ## no critic
> annotations except RequireFilenameMatchesPackage, which can't be fixed
> without more drastic reworking of the plperl build process.
>
> Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and
> --enable-tap-tests followed by make check-world, and running pgindent
> --build.

Attached is an updated version of the patch, in which
src/tools/msvc/gendef.pl actually compiles.  If someone on Windows could
test it, that would be great.

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law

>From 2bbdd768bdbabe10e0af6b95d2d09d29095d3a8b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Wed, 1 Mar 2017 15:32:45 +
Subject: [PATCH] Fix most perlcritic exceptions

The RequireFilenameMatchesPackage ones would require reworking the
plperl build process more drastically.
---
 src/pl/plperl/plc_perlboot.pl | 9 +++--
 src/tools/msvc/gendef.pl  | 2 +-
 src/tools/pgindent/pgindent   | 6 +++---
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
index 292c9101c9..b4212f5ab2 100644
--- a/src/pl/plperl/plc_perlboot.pl
+++ b/src/pl/plperl/plc_perlboot.pl
@@ -81,18 +81,15 @@ sub ::encode_array_constructor
 		} sort keys %$imports;
 		$BEGIN &&= "BEGIN { $BEGIN }";
 
-		return qq[ package main; sub { $BEGIN $prolog $src } ];
+		# default no strict and no warnings
+		return qq[ package main; sub { no strict; no warnings; $BEGIN $prolog $src } ];
 	}
 
 	sub mkfunc
 	{
-		## no critic (ProhibitNoStrict, ProhibitStringyEval);
-		no strict;  # default to no strict for the eval
-		no warnings;# default to no warnings for the eval
-		my $ret = eval(mkfuncsrc(@_));
+		my $ret = eval(mkfuncsrc(@_)); ## no critic (ProhibitStringyEval);
 		$@ =~ s/\(eval \d+\) //g if $@;
 		return $ret;
-		## use critic
 	}
 
 	1;
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index 64227c2dce..598699e6ea 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -174,7 +174,7 @@ sub usage
 
 my %def = ();
 
-while (<$ARGV[0]/*.obj>)  ## no critic (RequireGlobFunction);
+while (glob("$ARGV[0]/*.obj"))
 {
 	my $objfile = $_;
 	my $symfile = $objfile;
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index a6b24b5348..51d6a28953 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -159,8 +159,7 @@ sub process_exclude
 		while (my $line = <$eh>)
 		{
 			chomp $line;
-			my $rgx;
-			eval " \$rgx = qr!$line!;";  ## no critic (ProhibitStringyEval);
+			my $rgx = eval { qr!$line! };
 			@files = grep { $_ !~ /$rgx/ } @files if $rgx;
 		}
 		close($eh);
@@ -435,7 +434,8 @@ sub diff
 
 sub run_build
 {
-	eval "use LWP::Simple;";  ## no critic (ProhibitStringyEval);
+	require LWP::Simple;
+	LWP::Simple->import(qw(getstore is_success));
 
 	my $code_base = shift || '.';
 	my $save_dir = getcwd();
-- 
2.11.0


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

Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line

[Dagfinn Ilmari Mannsåker]

David Christensen  writes:

>> Hi David,
>> 
>> Here's a review of your patch.
>
> Hi Ilmari, thanks for your time and review.  I’m fine with the revised 
> version.

Okay, I've marked the patch as Ready For Committer.

Thanks,

Ilmari

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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] Teach Catalog.pm how many attributes there should be per DATA() line

[Dagfinn Ilmari Mannsåker]

Hi David,

Here's a review of your patch.

David Christensen  writes:

> Throws a build error if we encounter a different number of fields in a
> DATA() line than we expect for the catalog in question.

The patch is a good idea, and as-is implements the suggested feature.
Tested by removing an attribute from a line in catalog/pg_proc.h and
observing the expected failure.  With the attribute in place, it builds and
passes make check-world.

Detailed code review:

[…]
> diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
[…]
> + check_natts($filename, $catalog{natts},$3) or
> +   die sprintf "Wrong number of Natts in DATA() 
> line %s:%d\n", $input_file,INPUT_FILE->input_line_number;

Including the expected/actual number of attributes in the error message
would be nice.  In fact, the 'die' could be moved into check_natts(),
where the actual number is already available, and would clutter the main
loop less.

> + unless ($natts)
> + {
> + die "Could not find definition for Natts_${catname} before 
> start of DATA()\n";
> + }

More idiomatically written as:

die 
  unless $natts;

> diff --git a/src/backend/utils/Gen_fmgrtab.pl 
> b/src/backend/utils/Gen_fmgrtab.pl

The changes to this file are redundant, since it calls Catalog::Catalogs(),
which already checks that the number of attributes matches.

Attached is a suggested revised patch.

- ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

>From b78b281983e7a0406c15461340391c21d6cddef9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 6 Mar 2017 14:51:50 +
Subject: [PATCH] Teach Catalog.pm how many attributes there should be per
 DATA() line

Throws a build error if we encounter a different number of fields in a
DATA() line than we expect for the catalog in question.

Previously, it was possible to silently ignore any mismatches at build
time which could result in symbol undefined errors at link time.  Now
we stop and identify the infringing line as soon as we encounter it,
which greatly speeds up the debugging process.
---
 src/backend/catalog/Catalog.pm | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e1f3c3a5ee..85a537ad95 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -46,6 +46,9 @@ sub Catalogs
 
 		open(INPUT_FILE, '<', $input_file) || die "$input_file: $!";
 
+		my ($filename) = ($input_file =~ m/(\w+)\.h$/);
+		my $natts_pat = "Natts_$filename";
+
 		# Scan the input file.
 		while ()
 		{
@@ -70,8 +73,15 @@ sub Catalogs
 			s/\s+/ /g;
 
 			# Push the data into the appropriate data structure.
-			if (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
+			if (/$natts_pat\s+(\d+)/)
+			{
+$catalog{natts} = $1;
+			}
+			elsif (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
+check_natts($filename, $catalog{natts}, $3,
+			$input_file, INPUT_FILE->input_line_number);
+
 push @{ $catalog{data} }, { oid => $2, bki_values => $3 };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
@@ -216,4 +226,20 @@ sub RenameTempFile
 	rename($temp_name, $final_name) || die "rename: $temp_name: $!";
 }
 
+# verify the number of fields in the passed-in bki structure
+sub check_natts
+{
+	my ($catname, $natts, $bki_val, $file, $line) = @_;
+	die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"
+		unless defined $natts;
+
+	# we're working with a copy and need to count the fields only, so collapse
+	$bki_val =~ s/"[^"]*?"/xxx/g;
+	my @atts = split /\s+/ => $bki_val;
+
+	die sprintf
+		"Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",
+		$file, $line, $natts, scalar @atts
+	  unless $natts == @atts;
+}
 1;
-- 
2.11.0


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

Re: [HACKERS] perlcritic

[Dagfinn Ilmari Mannsåker]

Hi Peter,

Peter Eisentraut  writes:

> I posted this about 18 months ago but then ran out of steam. [ ] Here
> is an updated patch.  The testing instructions below still apply.
> Especially welcome would be ideas on how to address some of the places
> I have marked with ## no critic.

Attached is a patch on top of yours that addresses all the ## no critic
annotations except RequireFilenameMatchesPackage, which can't be fixed
without more drastic reworking of the plperl build process.

Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and
--enable-tap-tests followed by make check-world, and running pgindent
--build.

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

>From cdf3ca19cbbf03111243f9b39eb6f402f25b4502 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 1 Mar 2017 15:32:45 +
Subject: [PATCH] Fix most perlcritic exceptions

The RequireFilenameMatchesPackage ones would require reworking the
plperl build process more drastically.
---
 src/pl/plperl/plc_perlboot.pl | 9 +++--
 src/tools/msvc/gendef.pl  | 2 +-
 src/tools/pgindent/pgindent   | 6 +++---
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
index 292c9101c9..b4212f5ab2 100644
--- a/src/pl/plperl/plc_perlboot.pl
+++ b/src/pl/plperl/plc_perlboot.pl
@@ -81,18 +81,15 @@ sub ::encode_array_constructor
 		} sort keys %$imports;
 		$BEGIN &&= "BEGIN { $BEGIN }";
 
-		return qq[ package main; sub { $BEGIN $prolog $src } ];
+		# default no strict and no warnings
+		return qq[ package main; sub { no strict; no warnings; $BEGIN $prolog $src } ];
 	}
 
 	sub mkfunc
 	{
-		## no critic (ProhibitNoStrict, ProhibitStringyEval);
-		no strict;  # default to no strict for the eval
-		no warnings;# default to no warnings for the eval
-		my $ret = eval(mkfuncsrc(@_));
+		my $ret = eval(mkfuncsrc(@_)); ## no critic (ProhibitStringyEval);
 		$@ =~ s/\(eval \d+\) //g if $@;
 		return $ret;
-		## use critic
 	}
 
 	1;
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index 64227c2dce..e2653f11d8 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -174,7 +174,7 @@ sub usage
 
 my %def = ();
 
-while (<$ARGV[0]/*.obj>)  ## no critic (RequireGlobFunction);
+while (glob($ARGV[0]/*.obj))
 {
 	my $objfile = $_;
 	my $symfile = $objfile;
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index a6b24b5348..51d6a28953 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -159,8 +159,7 @@ sub process_exclude
 		while (my $line = <$eh>)
 		{
 			chomp $line;
-			my $rgx;
-			eval " \$rgx = qr!$line!;";  ## no critic (ProhibitStringyEval);
+			my $rgx = eval { qr!$line! };
 			@files = grep { $_ !~ /$rgx/ } @files if $rgx;
 		}
 		close($eh);
@@ -435,7 +434,8 @@ sub diff
 
 sub run_build
 {
-	eval "use LWP::Simple;";  ## no critic (ProhibitStringyEval);
+	require LWP::Simple;
+	LWP::Simple->import(qw(getstore is_success));
 
 	my $code_base = shift || '.';
 	my $save_dir = getcwd();
-- 
2.11.0


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

Re: [HACKERS] timeouts in PostgresNode::psql

[Dagfinn Ilmari Mannsåker]

Michael Paquier  writes:

> On Mon, Feb 27, 2017 at 11:28 AM, Craig Ringer  wrote:
>
>> Still needs applying to pg9.6 and pg10.
>
> I did not understand at first what you meant, but after looking at the
> commit message of the patch things are clear:
> Newer Perl or IPC::Run versions default to appending the filename to string
> exceptions, e.g. the exception
> psql timed out
>  is thrown as
> psql timed out at /usr/share/perl5/vendor_perl/IPC/Run.pm line 2961.
>
> And that's good to know, I can see as well that the patch is on the CF app:
> https://commitfest.postgresql.org/13/
> And that it has been marked as ready for committer.

That was me reviewing it, but the email to the list bounced, because the
commitfest app (or the python email.message module, depending on who you
feel like blaming) doesn't handle non-ASCII characters in structured
headers correctly.  In my case, it generated the header

From: =?utf-8?q?Dagfinn_Ilmari_Manns=C3=A5ker_=3Cilmari=40ilmari=2Eorg=3E?=\n\n

when it should have been

From: =?utf-8?q?Dagfinn_Ilmari_Manns=C3=A5ker?= 

Anyway, my rewview was as follows:

Tested by copying the timeout example from the PostgresNode docs into a
test file and adding an assertion that the variable passed to the
'timed_out' parameter gets set.  Without the patch the test script dies
when the timeout occurs, with the patch the test passes.

PostgresNode itself could do with some tests (nothing in the actual
tests seems to use the timeout functionality), but that's for another
patch.

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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 GUCs for predicate lock promotion thresholds

[Dagfinn Ilmari Mannsåker]

Kevin Grittner  writes:

> It occurred to me that it would make sense to allow these settings
> to be attached to a database or table (though *not* a role).  I'll
> look at that when I get back if you don't get to it first.

Attached is a draft patch (no docs or tests) on top of the previous one,
adding reloptions for the per-relation and per-page thresholds.  That
made me realise that the corresponding GUCs don't need to be PGC_SIGHUP,
but could be PGC_SUSET or PGC_USERSET.  I'll adjust the original patch
if you agree.  I have not got around around to adding per-database
versions of the setting, but could have a stab at that.


-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

>From e2ae108ca8212790604ef7e54f278e41ca460ffb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 25 Jan 2017 00:13:53 +
Subject: [PATCH 2/2] Add max_pred_locks_per_{relation,page} reloptions

---
 src/backend/access/common/reloptions.c | 24 +-
 src/backend/storage/lmgr/predicate.c   | 37 +++---
 src/bin/psql/tab-complete.c|  2 ++
 src/include/utils/rel.h| 21 +++
 4 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 42b4ea410f..ab8977a218 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -284,6 +284,24 @@ static relopt_int intRelOpts[] =
 		},
 		-1, 0, 1024
 	},
+	{
+		{
+			"max_pred_locks_per_relation",
+			"Maximum number of pages or rows that can be predicate-locked before locking the whole relation.",
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock,
+		},
+		-1, 0, INT_MAX
+	},
+	{
+		{
+			"max_pred_locks_per_page",
+			"Maximum number of rows on a single page that can be predicate-locked before locking the whole page.",
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock,
+		},
+		-1, 0, INT_MAX
+	},
 
 	/* list terminator */
 	{{NULL}}
@@ -1310,7 +1328,11 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 		{"user_catalog_table", RELOPT_TYPE_BOOL,
 		offsetof(StdRdOptions, user_catalog_table)},
 		{"parallel_workers", RELOPT_TYPE_INT,
-		offsetof(StdRdOptions, parallel_workers)}
+		offsetof(StdRdOptions, parallel_workers)},
+		{"max_pred_locks_per_relation", RELOPT_TYPE_INT,
+		offsetof(StdRdOptions, max_predicate_locks_per_relation)},
+		{"max_pred_locks_per_page", RELOPT_TYPE_INT,
+		offsetof(StdRdOptions, max_predicate_locks_per_page)}
 	};
 
 	options = parseRelOptions(reloptions, validate, kind, );
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index ac927f2c3a..9b767d7e04 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -443,8 +443,9 @@ static void RestoreScratchTarget(bool lockheld);
 static void RemoveTargetIfNoLongerUsed(PREDICATELOCKTARGET *target,
 		   uint32 targettaghash);
 static void DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag);
-static int	MaxPredicateChildLocks(const PREDICATELOCKTARGETTAG *tag);
-static bool CheckAndPromotePredicateLockRequest(const PREDICATELOCKTARGETTAG *reqtag);
+static int	MaxPredicateChildLocks(const PREDICATELOCKTARGETTAG *tag, Relation relation);
+static bool CheckAndPromotePredicateLockRequest(const PREDICATELOCKTARGETTAG *reqtag,
+Relation relation);
 static void DecrementParentLocks(const PREDICATELOCKTARGETTAG *targettag);
 static void CreatePredicateLock(const PREDICATELOCKTARGETTAG *targettag,
 	uint32 targettaghash,
@@ -453,7 +454,8 @@ static void DeleteLockTarget(PREDICATELOCKTARGET *target, uint32 targettaghash);
 static bool TransferPredicateLocksToNewTarget(PREDICATELOCKTARGETTAG oldtargettag,
   PREDICATELOCKTARGETTAG newtargettag,
   bool removeOld);
-static void PredicateLockAcquire(const PREDICATELOCKTARGETTAG *targettag);
+static void PredicateLockAcquire(const PREDICATELOCKTARGETTAG *targettag,
+ Relation relation);
 static void DropAllPredicateLocksFromTable(Relation relation,
 			   bool transfer);
 static void SetNewSxactGlobalXmin(void);
@@ -2138,17 +2140,27 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
  * entirely arbitrarily (and without benchmarking).
  */
 static int
-MaxPredicateChildLocks(const PREDICATELOCKTARGETTAG *tag)
+MaxPredicateChildLocks(const PREDICATELOCKTARGETTAG *tag, Relation relation)
 {
 	switch (GET_PREDICATELOCKTARGETTAG_TYPE(*tag))
 	{
 		case PREDLOCKTAG_RELATION:
+		{
+			int rel_max_pred_locks = RelationGetMaxPredicateLocksPerRelation(relation, -1);
+			if (rel_max_pred_locks != -1)
+

Re: [HACKERS] [PATCH] Add tab completion for DEALLOCATE

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Manns�ker) writes:

> Here's an updated patch wich adds it as a separate stanza.

I've added this to the current commit fest:
https://commitfest.postgresql.org/13/1043/

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


-- 
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] FYI: git worktrees as replacement for "rsync the CVSROOT"

[Dagfinn Ilmari Mannsåker]

Bruce Momjian  writes:

> On Sun, Jan 15, 2017 at 03:01:47PM -0600, Jim Nasby wrote:
>> Not sure how many people still use [1], as referenced by our git wiki[2],
>> but it appears git worktrees are a viable replacement for that technique. In
>> short, if you're already in your checkout:
>> 
>> git worktree add ../9.6 REL9_6_STABLE
>> 
>> would give you a checkout of 9.6 in the ../9.6 directory.
>> 
>> BTW, I learned about this from this "git year in review" article[3].
>> 
>> 1: 
>> https://www.postgresql.org/message-id/20090602162347.gf23...@yugib.highrise.ca
>> 2: 
>> https://wiki.postgresql.org/wiki/Working_with_Git#Continuing_the_.22rsync_the_CVSROOT.22_workflow
>> 3: 
>> https://hackernoon.com/git-in-2016-fad96ae22a15?imm_mid=0ec3e0=em-prog-na-na-newsltr_20170114#.shgj609ad
>
> Uh, I don't see this in git 2.1.4:
>
>   $ git worktree
>   git: 'worktree' is not a git command. See 'git --help'.
>
> which is in Debian Jessie.  This reports worktree was added in 2.5,
> released in July 2015:

Backports has git 2.11.0. Just add the "jessie-backports" suite to your
sources list, e.g.:

deb http://ftp..debian.org/debian/ jessie-backports main

And install git from there:

sudo apt install git/jessie-backports

Apt won't upgrade other packages to backports versions, but any packages
you've manually installed from there will be kept up-to-date.

See https://backports.debian.org/ for more details.

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
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 tab completion for DEALLOCATE

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> While playing with prepared statements in psql, I noticed that EXECUTE
>> tab-completes the list of active prepared statements, but DEALLOCATE
>> does not.
>> Attached is a patch to fix this.
>
> Good idea, but I think it would be better to give DEALLOCATE its own
> entry in the list, so it could be placed in alphabetical order.

I was following the example from FETCH/MOVE further down to avoid
duplicating the body of the stanza, but I guess in this case it's simple
enough that it doesn't matter.

Here's an updated patch wich adds it as a separate stanza.

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


>From 8b5e4ed63e558475b193b1d668ed14baae0c497c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 2 Feb 2017 10:09:26 +
Subject: [PATCH] Add tab completion for DEALLOCATE

EXECUTE already tab-completes the list of prepared statements, but
DEALLOCATE was missing.
---
 src/bin/psql/tab-complete.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index d6fffcf42f..539ab1afe2 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2453,6 +2453,10 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches5("CREATE", "EVENT", "TRIGGER", MatchAny, "ON"))
 		COMPLETE_WITH_LIST3("ddl_command_start", "ddl_command_end", "sql_drop");
 
+/* DEALLOCATE */
+	else if (Matches1("DEALLOCATE"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);
+
 /* DECLARE */
 	else if (Matches2("DECLARE", MatchAny))
 		COMPLETE_WITH_LIST5("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
-- 
2.11.0


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

[HACKERS] [PATCH] Add tab completion for DEALLOCATE

[Dagfinn Ilmari Mannsåker]

Hi hackers,

While playing with prepared statements in psql, I noticed that EXECUTE
tab-completes the list of active prepared statements, but DEALLOCATE
does not.

Attached is a patch to fix this.

Cheers,

Ilmari

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen

>From ac727e99d6f06a82d6ba9a7927fed8ce179dac3f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 1 Feb 2017 17:39:43 +
Subject: [PATCH] Add tab completion for DEALLOCATE

EXECUTE already tab-completes the list of prepared statements, but
DEALLOCATE did not.
---
 src/bin/psql/tab-complete.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index d6fffcf42f..4a7bfe844a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2575,8 +2575,8 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches5("DROP", "RULE", MatchAny, "ON", MatchAny))
 		COMPLETE_WITH_LIST2("CASCADE", "RESTRICT");
 
-/* EXECUTE */
-	else if (Matches1("EXECUTE"))
+/* EXECUTE && DEALLOCATE */
+	else if (Matches1("EXECUTE|DEALLOCATE"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);
 
 /* EXPLAIN */
-- 
2.11.0


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

Re: [HACKERS] [PATCH] Add GUCs for predicate lock promotion thresholds

[Dagfinn Ilmari Mannsåker]

Kevin Grittner  writes:

> (1)  The new GUCs are named max_pred_locks_per_*, but the
> implementation passes them unmodified from a function specifying at
> what count the lock should be promoted.  We either need to change
> the names or (probably better, only because I can't think of a good
> name for the other way) return the GUC value + 1 from the function.
> Or maybe change the function name and how the returned number is
> used, if we care about eliminating the overhead of the increment
> instruction.  That actually seems least confusing.

I've done the latter, and renamed the function MaxPredicateChildLocks.
I also changed the default for max_pred_locks_per_page from 3 to 4 and
the minimum to 0, to keep the effective thresholds the same.

> (2)  The new GUCs are declared and documented to be set only on
> startup.  This was probably just copied from
> max_pred_locks_per_transaction, but that sets an allocation size
> while these don't.  I think these new GUCs should be PGC_SIGHUP,
> and documented to change on reload.

Fixed.

> (3)  The documentation for max_pred_locks_per_relation needs a fix.
> Both page locks and tuple locks for the relation are counted toward
> the limit.

Fixed.

> In releases prior to this patch, max_pred_locks_per_relation was
> calculated as "max_pred_locks_per_transaction / 2".  I know that
> people have sometimes increased max_pred_locks_per_transaction
> specifically to raise the limit on locks within a relation before
> the promotion to relation granularity occurs.  It seems kinda
> anti-social not to support a special value for continuing that
> behavior or, if we don't want to do that, at least modifying
> pg_upgrade to set max_pred_locks_per_relation to the value that was
> in effect in the old version.

This is exactly what we've been doing at my workplace, and I mentioned
this in my original email:

>> ilm...@ilmari.org (Dagfinn Ilmari Manns�ker) writes:
>>> One thing I don't like about this patch is that if a user has
>>> increased max_pred_locks_per_transaction, they need to set
>>> max_pred_locks_per_relation to half of that to retain the current
>>> behaviour, or they'll suddenly find themselves with a lot more
>>> relation locks.  If it's possible to make a GUCs default value
>>> dependent on the value of another, that could be a solution.
>>> Otherwise, the page lock threshold GUC could be changed to be
>>> expressed as a fraction of max_pred_locks_per_transaction, to keep
>>> the current behaviour.
>
>> Another option would be to have a special sentinel (e.g. -1) which is
>> the default, and keeps the current behaviour.

The attached updated patch combines the two behaviours.  Positive values
mean an absolute number of locks, while negative values mean
max_predicate_locks_per_transaction / -max_predicate_locks_per_relation.
Making the default value -2 keeps backwards compatibility.

Alternatively we could have a separate setting for the fraction (which
could then be a floating-point number, for finer control), and use the
absolute value if that is zero.

Regards,

Ilmari

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

>From 12c55660e9235e100b8802645eb8aaef7683888f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 12 Dec 2016 17:57:33 +
Subject: [PATCH] Add GUCs for predicate lock promotion thresholds

This addresses part of the TODO comment for predicate lock promotion
threshold, by making them configurable.  The default values are the same
as what used to be hardcoded.
---
 doc/src/sgml/config.sgml  | 39 +++
 doc/src/sgml/mvcc.sgml|  4 ++-
 src/backend/storage/lmgr/predicate.c  | 37 +++--
 src/backend/utils/misc/guc.c  | 22 +++
 src/backend/utils/misc/postgresql.conf.sample |  2 ++
 src/include/storage/predicate.h   |  2 ++
 6 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index fb5d6473ef..1f944aab1c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7255,6 +7255,45 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
  
 
+ 
+  max_pred_locks_per_relation (integer)
+  
+   max_pred_locks_per_relation configuration parameter
+  
+  
+  
+   
+This controls how many pages or tuples of a single relation can be
+predicate-locked before the lock is promoted to covering the whole
+relation.  Values greater than or equal to zero mean an absolute
+limit, while negative values
+mean  divided by
+the absolute value of this setting.  The default is -2, which keeps
+the behaviour from previous versions of PostgreSQL.
+This parameter can only be set in the postgresql.conf
+  

Re: [HACKERS] [PATCH] Add GUCs for predicate lock promotion thresholds

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> One thing I don't like about this patch is that if a user has increased
> max_pred_locks_per_transaction, they need to set
> max_pred_locks_per_relation to half of that to retain the current
> behaviour, or they'll suddenly find themselves with a lot more relation
> locks.  If it's possible to make a GUCs default value dependent on the
> value of another, that could be a solution.  Otherwise, the page lock
> threshold GUC could be changed to be expressed as a fraction of
> max_pred_locks_per_transaction, to keep the current behaviour.

Another option would be to have a special sentinel (e.g. -1) which is
the default, and keeps the current behaviour.

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen



-- 
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 GUCs for predicate lock promotion thresholds

[Dagfinn Ilmari Mannsåker]

Kevin Grittner <kgri...@gmail.com> writes:

> On Wed, Dec 14, 2016 at 8:16 AM, Dagfinn Ilmari Mannsåker
> <ilm...@ilmari.org> wrote:
>
>> Attached is a patch
>
> Please add this to the open CommitFest to ensure that it is reviewed.

Done.

https://commitfest.postgresql.org/12/910/

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law



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

[HACKERS] [PATCH] Add GUCs for predicate lock promotion thresholds

[Dagfinn Ilmari Mannsåker]

Hi hackers,

I have a workload using SSI that causes a lot of tuple predicate to be
promoted to page locks.  This causes a lot of spurious serialisability
errors, since the promotion happens at once three tuples on a page are
locked, and the affected tables have 30-90 tuples per page.

PredicateLockPromotionThreshold() has the following comment:

 * TODO SSI: We should do something more intelligent about what the
 * thresholds are, either making it proportional to the number of
 * tuples in a page & pages in a relation, or at least making it a
 * GUC. 

Attached is a patch that does the "at least" part of this.

One thing I don't like about this patch is that if a user has increased
max_pred_locks_per_transaction, they need to set
max_pred_locks_per_relation to half of that to retain the current
behaviour, or they'll suddenly find themselves with a lot more relation
locks.  If it's possible to make a GUCs default value dependent on the
value of another, that could be a solution.  Otherwise, the page lock
threshold GUC could be changed to be expressed as a fraction of
max_pred_locks_per_transaction, to keep the current behaviour.


Cheers,

Ilmari

>From bb81a54ee6c9a4855f6aeb52b968d188f44b14ac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 12 Dec 2016 17:57:33 +
Subject: [PATCH] Add GUCs for predicate lock promotion thresholds

This addresses part of the TODO comment for predicate lock promotion
threshold, by making them configurable.  The default values are the same
as what used to be hardcoded.
---
 doc/src/sgml/config.sgml  | 36 +++
 doc/src/sgml/mvcc.sgml|  4 ++-
 src/backend/storage/lmgr/predicate.c  | 18 +-
 src/backend/utils/misc/guc.c  | 22 
 src/backend/utils/misc/postgresql.conf.sample |  3 +++
 src/include/storage/predicate.h   |  2 ++
 6 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0fc4e57d90..6e133ffebd 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7172,6 +7172,42 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
  
 
+ 
+  max_pred_locks_per_relation (integer)
+  
+   max_pred_locks_per_relation configuration parameter
+  
+  
+  
+   
+This controls how many pages of a single relation can be
+predicate-locked before the lock is promoted to covering the whole
+relation.  The default is 32.  In previous versions
+of PostgreSQL it used to be hard-coded to half
+of , and you might
+want to raise this value if you raise that.  This parameter can only
+be set at server start.
+   
+
+  
+ 
+
+ 
+  max_pred_locks_per_page (integer)
+  
+   max_pred_locks_per_page configuration parameter
+  
+  
+  
+   
+This controls how many rows on a single page can be predicate-locked
+before the lock is promoted to covering the whole page.  The default
+is 3.  This parameter can only be set at server start.
+   
+
+  
+ 
+
  

 
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 306def4a15..4652cdf094 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -765,7 +765,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
locks into a single relation-level predicate lock because the predicate
lock table is short of memory, an increase in the rate of serialization
failures may occur.  You can avoid this by increasing
-   .
+   ,
+and/or
+   .
   
  
  
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 24ed21b487..e73b2b417c 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -355,6 +355,12 @@ static SERIALIZABLEXACT *OldCommittedSxact;
 /* This configuration variable is used to set the predicate lock table size */
 int			max_predicate_locks_per_xact;		/* set by guc.c */
 
+/* This configuration variable is used to decide when to upgrade a page lock to a relation lock */
+int			max_predicate_locks_per_relation;	/* set by guc.c */
+
+/* This configuration variable is used to decide when to upgrade a row lock to a page lock */
+int			max_predicate_locks_per_page;		/* set by guc.c */
+
 /*
  * This provides a list of objects in order to track transactions
  * participating in predicate locking.  Entries in the list are fixed size,
@@ -2124,10 +2130,10 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
  * descendants, e.g. both tuples and pages for a relation lock.
  *
  * TODO SSI: We should do something more intelligent about what the
- * thresholds are, either making it proportional to the 

Re: [HACKERS] Document how to set up TAP tests for Perl 5.8.8

[Dagfinn Ilmari Mannsåker]

Noah Misch  writes:

> On Mon, Nov 21, 2016 at 05:37:50PM +0900, Kyotaro HORIGUCHI wrote:
>> At Mon, 21 Nov 2016 16:45:10 +0900, Michael Paquier 
>>  wrote in 
>> 
>> > No objections to this version. It's a good idea to document that.
>> 
>> +1
>
> Committed after re-filling paragraphs.

I just noticed this bit in the committed patch:

+cpanm install IPC::Run

This is not the correct invocation of cpanm: it just takes a list of
module names.  This will pull in the 'install' module, which is
harmless, but unnecessary: https://metacpan.org/pod/install.

-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


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

[HACKERS] [PATCH] Add diff directives to gitattributes

[Dagfinn Ilmari Mannsåker]

Hi hackers,

Attached is a patch which adds diff= directives to .gitattributes for C,
Perl and (X|SG)ML files.  This makes word diffs and the function
indicator in the diff chunk header and more useful.

>From 57d7d4ec5b94783bf68b2959128e33c28547a6b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Tue, 29 Nov 2016 19:47:36 +
Subject: [PATCH] Add diff directives to .gitattributes

This makes the function indicator in the diff chunk header and word
diffs more useful for the specified languages.
---
 .gitattributes | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/.gitattributes b/.gitattributes
index 4dfc131..3805f90 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,11 +1,11 @@
 *		whitespace=space-before-tab,trailing-space
-*.[chly]	whitespace=space-before-tab,trailing-space,indent-with-non-tab,tabwidth=4
+*.[chly]	whitespace=space-before-tab,trailing-space,indent-with-non-tab,tabwidth=4 diff=cpp
 *.dsl		whitespace=space-before-tab,trailing-space,tab-in-indent
 *.patch		-whitespace
-*.pl		whitespace=space-before-tab,trailing-space,tabwidth=4
+*.pl		whitespace=space-before-tab,trailing-space,tabwidth=4 diff=perl
 *.po		whitespace=space-before-tab,trailing-space,tab-in-indent,-blank-at-eof
-*.sgml		whitespace=space-before-tab,trailing-space,tab-in-indent,-blank-at-eol
-*.x[ms]l	whitespace=space-before-tab,trailing-space,tab-in-indent
+*.sgml		whitespace=space-before-tab,trailing-space,tab-in-indent,-blank-at-eol diff=html
+*.x[ms]l	whitespace=space-before-tab,trailing-space,tab-in-indent diff=html
 
 # Avoid confusing ASCII underlines with leftover merge conflict markers
 README		conflict-marker-size=32
-- 
2.7.4


-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

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

Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Dagfinn Ilmari Mannsåker]

Robert Haas <robertmh...@gmail.com> writes:

> On Tue, Nov 8, 2016 at 8:53 AM, Dagfinn Ilmari Mannsåker
> <ilm...@ilmari.org> wrote:
>> Thank you very much.  I just looked at the patch again and realised the
>> completion of "TO" after RENAME VALUE  can be merged with the one
>> for RENAME ATTRIBUTE .  Attached is an updated and patch
>
> Committed.

Thanks!

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


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

Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Dagfinn Ilmari Mannsåker]

Artur Zakirov <a.zaki...@postgrespro.ru> writes:

> Hello,

Hi Artur,

> 2016-09-12 16:16 GMT+03:00 Dagfinn Ilmari Mannsåker <ilm...@ilmari.org>:
>>
>> I've added it to the 2016-11 commit fest:
>> https://commitfest.postgresql.org/11/795/
>>
>> - ilmari
>
> I've tested your patch.
[...]
> It seems that the patch can be commited without any fixes. So I marked
> it as "Ready for Committer".

Thank you very much.  I just looked at the patch again and realised the
completion of "TO" after RENAME VALUE  can be merged with the one
for RENAME ATTRIBUTE .  Attached is an updated and patch, which
only differs from the previous one as follows:

$ interdiff 0001-Add-psql-tab-completion-for-ALTER-TYPE-RENAME-VALUE{,-v2}.patch
diff -u b/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1918,11 +1918,8 @@ psql_completion(const char *text, int start, int end)
/* ALTER TYPE  RENAME  */
else if (Matches4("ALTER", "TYPE", MatchAny, "RENAME"))
COMPLETE_WITH_LIST3("ATTRIBUTE", "TO", "VALUE");
-   /* ALTER TYPE xxx RENAME ATTRIBUTE yyy */
-   else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "ATTRIBUTE", 
MatchAny))
-   COMPLETE_WITH_CONST("TO");
-   /* ALTER TYPE xxx RENAME VALUE yyy */
-   else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "VALUE", MatchAny))
+   /* ALTER TYPE xxx RENAME (ATTRIBUTE|VALUE) yyy */
+   else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "ATTRIBUTE|VALUE", 
MatchAny))
COMPLETE_WITH_CONST("TO");
/*
 * If we have ALTER TYPE  ALTER/DROP/RENAME ATTRIBUTE, provide list

>From d8454b66a544d91bf779ca3bfb38d18e4cd504f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Mon, 12 Sep 2016 12:17:37 +0100
Subject: [PATCH] =?UTF-8?q?Add=20psql=20tab=20completion=20for=20ALTER=20T?=
 =?UTF-8?q?YPE=20=E2=80=A6=20RENAME=20VALUE?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Modelled on the completion for attributes, tweaked to return string
literals intead of identifiers.
---
 src/bin/psql/tab-complete.c | 58 +
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a43bbc5..b556c00 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -202,6 +202,31 @@ do { \
 	matches = completion_matches(text, complete_from_query); \
 } while (0)
 
+#define COMPLETE_WITH_ENUM_VALUE(type) \
+do { \
+	char   *_completion_schema; \
+	char   *_completion_type; \
+\
+	_completion_schema = strtokx(type, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+	(void) strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+   false, false, pset.encoding); \
+	_completion_type = strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+			   false, false, pset.encoding);  \
+	if (_completion_type == NULL)\
+	{ \
+		completion_charp = Query_for_list_of_enum_values; \
+		completion_info_charp = type; \
+	} \
+	else \
+	{ \
+		completion_charp = Query_for_list_of_enum_values_with_schema; \
+		completion_info_charp = _completion_type; \
+		completion_info_charp2 = _completion_schema; \
+	} \
+	matches = completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
 	char   *_completion_schema; \
@@ -598,6 +623,26 @@ static const SchemaQuery Query_for_list_of_matviews = {
 "   AND (pg_catalog.quote_ident(nspname)='%s' "\
 "OR '\"' || nspname || '\"' ='%s') "
 
+#define Query_for_list_of_enum_values \
+"SELECT pg_catalog.quote_literal(enumlabel) "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t "\
+" WHERE t.oid = e.enumtypid "\
+"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"OR '\"' || typname || '\"'='%s') "\
+"   AND pg_catalog.pg_type_is_visible(t.oid)"
+
+#define Query_for_list_of_enum_values_with_schema \
+"SELECT pg_catalog.quote_literal(enumlabel) "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t, pg_catalog.pg_namespace n "\
+" WHERE t.oid = e.enumtypid "\
+"   AND n.oid = t.typnamespace "\
+"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"OR '\"' || typname || '\"'='%s') &qu

Re: [HACKERS] sequential scan result order vs performance

[Dagfinn Ilmari Mannsåker]

Jim Nasby  writes:

> BTW, I've sometimes wished for a mode where queries would silently have
> result ordering intentionally futzed, to eliminate any possibility of
> dependence on tuple ordering (as well as having sequences start at some
> random value).

FWIW, SQLite has this, in the form of 'PRAGMA reverse_unordered_selects'.

http://sqlite.org/pragma.html#pragma_reverse_unordered_selects

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


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

[HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> Hi hackers,
>
> Here's a patch to add psql tab completion for the recently-added ALTER
> TYPE … RENAME VALUE feature (thanks to Tom for fixing it up and
> committing it).

I've added it to the 2016-11 commit fest:
https://commitfest.postgresql.org/11/795/

- ilmari

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law



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

[HACKERS] [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Dagfinn Ilmari Mannsåker]

Hi hackers,

Here's a patch to add psql tab completion for the recently-added ALTER
TYPE … RENAME VALUE feature (thanks to Tom for fixing it up and
committing it).

It's modelled on the ALTER TYPE … RENAME ATTRIBUTE completion, but
tweaked to return string literals instead of identifiers.

- ilmari

>From f4fa474262e6e65f02095f9de09205bff7ea2a1d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 12 Sep 2016 12:17:37 +0100
Subject: [PATCH] =?UTF-8?q?Add=20psql=20tab=20completion=20for=20ALTER=20T?=
 =?UTF-8?q?YPE=20=E2=80=A6=20RENAME=20VALUE?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Modelled on the completion for attributes, tweaked to return string
literals intead of identifiers.
---
 src/bin/psql/tab-complete.c | 57 +++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 3e2f084..40790c9 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -202,6 +202,31 @@ do { \
 	matches = completion_matches(text, complete_from_query); \
 } while (0)
 
+#define COMPLETE_WITH_ENUM_VALUE(type) \
+do { \
+	char   *_completion_schema; \
+	char   *_completion_type; \
+\
+	_completion_schema = strtokx(type, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+	(void) strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+   false, false, pset.encoding); \
+	_completion_type = strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+			   false, false, pset.encoding);  \
+	if (_completion_type == NULL)\
+	{ \
+		completion_charp = Query_for_list_of_enum_values; \
+		completion_info_charp = type; \
+	} \
+	else \
+	{ \
+		completion_charp = Query_for_list_of_enum_values_with_schema; \
+		completion_info_charp = _completion_type; \
+		completion_info_charp2 = _completion_schema; \
+	} \
+	matches = completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
 	char   *_completion_schema; \
@@ -598,6 +623,26 @@ static const SchemaQuery Query_for_list_of_matviews = {
 "   AND (pg_catalog.quote_ident(nspname)='%s' "\
 "OR '\"' || nspname || '\"' ='%s') "
 
+#define Query_for_list_of_enum_values \
+"SELECT pg_catalog.quote_literal(enumlabel) "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t "\
+" WHERE t.oid = e.enumtypid "\
+"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"OR '\"' || typname || '\"'='%s') "\
+"   AND pg_catalog.pg_type_is_visible(t.oid)"
+
+#define Query_for_list_of_enum_values_with_schema \
+"SELECT pg_catalog.quote_literal(enumlabel) "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t, pg_catalog.pg_namespace n "\
+" WHERE t.oid = e.enumtypid "\
+"   AND n.oid = t.typnamespace "\
+"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"OR '\"' || typname || '\"'='%s') "\
+"   AND (pg_catalog.quote_ident(nspname)='%s' "\
+"OR '\"' || nspname || '\"' ='%s') "
+
 #define Query_for_list_of_template_databases \
 "SELECT pg_catalog.quote_ident(d.datname) "\
 "  FROM pg_catalog.pg_database d "\
@@ -1873,11 +1918,13 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH_LIST2("ATTRIBUTE", "VALUE");
 	/* ALTER TYPE  RENAME	*/
 	else if (Matches4("ALTER", "TYPE", MatchAny, "RENAME"))
-		COMPLETE_WITH_LIST2("ATTRIBUTE", "TO");
+		COMPLETE_WITH_LIST3("ATTRIBUTE", "TO", "VALUE");
 	/* ALTER TYPE xxx RENAME ATTRIBUTE yyy */
 	else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "ATTRIBUTE", MatchAny))
 		COMPLETE_WITH_CONST("TO");
-
+	/* ALTER TYPE xxx RENAME VALUE yyy */
+	else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "VALUE", MatchAny))
+		COMPLETE_WITH_CONST("TO");
 	/*
 	 * If we have ALTER TYPE  ALTER/DROP/RENAME ATTRIBUTE, provide list
 	 * of attributes
@@ -1897,6 +1944,12 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches5("ALTER", "GROUP", MatchAny, "ADD|DROP", "USER"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
 
+	/*
+	 * If we have ALTER TYPE  RENAME VALUE, provide list of enum values
+	 */
+	else if (Matches5("ALTER", "TYPE", MatchAny, "RENAME", "VALUE"))
+		COMPLETE_WITH_ENUM_VALUE(prev3_wd);
+
 /* BEGIN */
 	else if (Matches1("BEGIN"))
 		COMPLETE_WITH_LIST6("WORK", "TRANSACTION", "ISOLATION LEVEL", "READ", "DEFERRABLE", "NOT DEFERRABLE");
-- 
2.9.3


-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

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

Re: [HACKERS] [PATCH] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Emre Hasegeli  writes:

>> Bottom line here is that I'd rather commit ALTER TYPE RENAME VALUE with
>> no EXISTS features and then see it accrete those features together with
>> other types of RENAME, when and if there's a will to make that happen.
>
> This sounds like a good conclusion to me.

Works for me. I mainly added the IF [NOT] EXISTS support to be
consistent with ADD VALUE, and because I like idempotency, but I'm not
married to it.

Here is version 6 of the patch, which just adds RENAME VALUE with no IF
[NOT] EXISTS, rebased onto current master (particularly the
transactional ADD VALUE patch).

>From e4ee07d53bbab5ee434a12a2f30a9ac9bb5c89d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Tue, 6 Sep 2016 14:38:06 +0100
Subject: [PATCH 1/2] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20RENAME=20V?=
 =?UTF-8?q?ALUE=20=E2=80=A6=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unlike adding values to an enum, altering an existing value doesn't
affect the OID values or ordering, so can be done in a transaction
regardless of whether the enum was created in the same transaction.
---
 doc/src/sgml/ref/alter_type.sgml   | 18 +++-
 src/backend/catalog/pg_enum.c  | 91 ++
 src/backend/commands/typecmds.c| 12 +++--
 src/backend/parser/gram.y  | 14 ++
 src/include/catalog/pg_enum.h  |  2 +
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/enum.out | 58 
 src/test/regress/sql/enum.sql  | 27 +++
 8 files changed, 217 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index aec73f6..bdc2fa2 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -29,6 +29,7 @@ ALTER TYPE name RENAME ATTRIBUTE name RENAME TO new_name
 ALTER TYPE name SET SCHEMA new_schema
 ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ]
+ALTER TYPE name RENAME VALUE existing_enum_value TO new_enum_value
 
 where action is one of:
 
@@ -123,6 +124,17 @@ ALTER TYPE name ADD VALUE [ IF NOT
 

 
+   
+RENAME VALUE TO
+
+ 
+  This form renames a value in an enum type.
+  The value's place in the enum's ordering is not affected.
+  An error will occur if the old value is not already present or the new value is.
+ 
+
+   
+

 CASCADE
 
@@ -241,7 +253,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   new_enum_value
   

-The new value to be added to an enum type's list of values.
+The new value to be added to an enum type's list of values,
+or to rename an existing one to.
 Like all enum literals, it needs to be quoted.

   
@@ -252,7 +265,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   

 The existing enum value that the new value should be added immediately
-before or after in the enum type's sort ordering.
+before or after in the enum type's sort ordering,
+or renamed from.
 Like all enum literals, it needs to be quoted.

   
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index c66f963..2e48dfe 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -465,6 +465,97 @@ AddEnumLabel(Oid enumTypeOid,
 }
 
 
+/*
+ * RenameEnumLabel
+ *		Rename a label in an enum set.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal)
+{
+	Relation	pg_enum;
+	HeapTuple	old_tup = NULL;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+	boolfound_new = false;
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Without that, we couldn't be sure to get a consistent view
+	 * of the enum members via the syscache.  Note that this does not block
+	 * other backends from inspecting the type; see comments for
+	 * RenumberEnumType.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/* Locate the element to rename and check if the new label is already in
+	 * use.  The unique index on pg_enum would catch this anyway, but we
+	 * prefer 

Re: [HACKERS] [PATCH] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Emre Hasegeli  writes:

>> Here is v4, which changes the command from ALTER VALUE to RENAME VALUE,
>> for consistency with RENAME ATTRIBUTE.
>
> It looks like we always use "ALTER ... RENAME ... old_name TO
> new_name" syntax, so it is better that way.  I have noticed that all
> the other RENAMEs go through ExecRenameStmt().  We better do the same.

That would require changing it from an AlterEnumStmt to a RenameStmt
instead.  Those look to me like they're for renaming SQL objects,
i.e. things addressed by identifiers, whereas enum labels are just
strings.  Looking at the implementation of a few of the functions called
by ExecRenameStmt(), they have very little in common with
RenameEnumLabel() logic-wise.

>> +int nbr_index;
>> +Form_pg_enum nbr_en;
>
> What is "nbr"?  Why not calling them something like "i" and "val"?

This is the same naming as used in AddEnumLabel(), which I used as a
guide.

>> +   /* Locate the element to rename and check if the new label is already in
>
> The project style for multi-line commands is to leave the first line
> of the comment block empty.
>
>> +if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
>
> I found namestrcmp() for this.

Thanks, fixed.  This again came from using AddEnumLabel() as an example,
which should probably be fixed separately.

>
>> +}
>> +if (!old_tup)
>
> I would leave a space in between.
>
>> +ReleaseCatCacheList(list);
>> +heap_close(pg_enum, RowExclusiveLock);
>
> Maybe we better release them before reporting error, too.  I would
> release the list after the loop, close the heap before ereport().

As Tom said, this gets released automatically on error, and is again
similar to how AddEnumLabel() does it.

Here is an updated patch.

>From 542b20b3a0f82d07203035aa853bae2ddccb6af3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20RENAME=20VALUE?=
 =?UTF-8?q?=20=E2=80=A6=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unlike adding values to an enum, altering an existing value doesn't
affect the OID values or ordering, so can be done in a transaction.

IF EXISTS and IF NOT EXISTS are supported for ignoring the non-existence
of the old value or the existance of the new one, respectively.
---
 doc/src/sgml/ref/alter_type.sgml   |  24 +++-
 src/backend/catalog/pg_enum.c  | 117 +
 src/backend/commands/typecmds.c|  52 ++---
 src/backend/parser/gram.y  |  18 ++
 src/include/catalog/pg_enum.h  |   3 +
 src/include/nodes/parsenodes.h |   2 +
 src/test/regress/expected/enum.out |  74 +++
 src/test/regress/sql/enum.sql  |  35 +++
 8 files changed, 301 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..9f0ca8f 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -29,6 +29,7 @@ ALTER TYPE name RENAME ATTRIBUTE name RENAME TO new_name
 ALTER TYPE name SET SCHEMA new_schema
 ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ]
+ALTER TYPE name RENAME VALUE [ IF EXISTS ] existing_enum_value TO new_enum_value [ IF NOT EXISTS ]
 
 where action is one of:
 
@@ -124,6 +125,23 @@ ALTER TYPE name ADD VALUE [ IF NOT

 

+RENAME VALUE [ IF EXISTS ] TO [ IF NOT EXISTS ]
+
+ 
+  This form renames a value in an enum type.  The value's place in the
+  enum's ordering is not affected.
+ 
+ 
+  If IF EXISTS or IF NOT EXISTS is
+  specified, it is not an error if the type doesn't contain the old value
+  or already contains the new value, respectively: a notice is issued but
+  no other action is taken.  Otherwise, an error will occur if the old
+  value is not already present or the new value is.
+ 
+
+   
+
+   
 CASCADE
 
  
@@ -241,7 +259,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   new_enum_value
   

-The new value to be added to an enum type's list of values.
+The new value to be added to an enum type's list of values,
+or to rename an existing one to.
 Like all enum literals, it needs to be quoted.

   
@@ -252,7 +271,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   

 The existing enum value that the new value should be added immediately
-before or after in the enum type's sort ordering.
+before or after in the enum type's sort ordering,
+or renamed from.
 Like all enum literals, it needs to be quoted.

   
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..1920138 100644
--- a/src/backend/catalog/pg_enum.c

Re: [HACKERS] [PATCH] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>
>> I was bored and thought "how hard could it be?", and a few hours'
>> hacking later, I have something that seems to work.  It doesn't do IF
>> NOT EXISTS yet, and the error messaging could do with some improvement,
>> and there are no docs.  The patch is attached, as well as at
>> https://github.com/ilmari/postgres/commit/enum-alter-value
>
> Here's v3 of the patch of the patch, which I consider ready for proper
> review.  It now features:
>
> - IF (NOT) EXISTS support
> - Transaction support
> - Documentation
> - Improved error reporting (renaming a non-existent value to an existing
>   one complains about the former, not the latter)

Here is v4, which changes the command from ALTER VALUE to RENAME VALUE,
for consistency with RENAME ATTRIBUTE.

Emre, I noticed you modified the commitfest entry
(https://commitfest.postgresql.org/10/588/) to be for Andrew's
transactional enum addition patch instead, but didn't change the title.
I'll revert that as soon as it picks up this latest patch.  Do you wish
to remain a reviewer for this patch, or should I remove you?

>From 225e3819317aa82fee91afe4970a0596f316cf9f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20RENAME=20VALUE?=
 =?UTF-8?q?=20=E2=80=A6=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unlike adding values to an enum, altering an existing value doesn't
affect the OID values or ordering, so can be done in a transaction.

IF EXISTS and IF NOT EXISTS are supported for ignoring the non-existence
of the old value or the existance of the new one, respectively.
---
 doc/src/sgml/ref/alter_type.sgml   |  24 +++-
 src/backend/catalog/pg_enum.c  | 115 +
 src/backend/commands/typecmds.c|  52 ++---
 src/backend/parser/gram.y  |  18 ++
 src/include/catalog/pg_enum.h  |   3 +
 src/include/nodes/parsenodes.h |   2 +
 src/test/regress/expected/enum.out |  63 
 src/test/regress/sql/enum.sql  |  30 ++
 8 files changed, 283 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..9f0ca8f 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -29,6 +29,7 @@ ALTER TYPE name RENAME ATTRIBUTE name RENAME TO new_name
 ALTER TYPE name SET SCHEMA new_schema
 ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ]
+ALTER TYPE name RENAME VALUE [ IF EXISTS ] existing_enum_value TO new_enum_value [ IF NOT EXISTS ]
 
 where action is one of:
 
@@ -123,6 +124,23 @@ ALTER TYPE name ADD VALUE [ IF NOT
 

 
+   
+RENAME VALUE [ IF EXISTS ] TO [ IF NOT EXISTS ]
+
+ 
+  This form renames a value in an enum type.  The value's place in the
+  enum's ordering is not affected.
+ 
+ 
+  If IF EXISTS or IF NOT EXISTS is
+  specified, it is not an error if the type doesn't contain the old value
+  or already contains the new value, respectively: a notice is issued but
+  no other action is taken.  Otherwise, an error will occur if the old
+  value is not already present or the new value is.
+ 
+
+   
+

 CASCADE
 
@@ -241,7 +259,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   new_enum_value
   

-The new value to be added to an enum type's list of values.
+The new value to be added to an enum type's list of values,
+or to rename an existing one to.
 Like all enum literals, it needs to be quoted.

   
@@ -252,7 +271,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   

 The existing enum value that the new value should be added immediately
-before or after in the enum type's sort ordering.
+before or after in the enum type's sort ordering,
+or renamed from.
 Like all enum literals, it needs to be quoted.

   
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..5d4c665 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -463,6 +463,121 @@ restart:
 }
 
 
+/*
+ * RenameEnumLabel
+ *		Rename a label in an enum set.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal,
+bool skipIfNotExists,
+bool skipIfExists)
+{
+	Relation	pg_enum;
+	HeapTuple	old_tup = NULL;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+	boolfound_new = false;
+
+	/* check length of new label is ok */
+	if (strlen

Re: [HACKERS] Use pread and pwrite instead of lseek + write and read

[Dagfinn Ilmari Mannsåker]

Magnus Hagander  writes:

[pread/pwrite]

> Yeah, Windows does not have those API calls, but it shouldn't be rocket
> science to write a wrapper for it. The standard windows APIs can do the
> same thing -- but they'll need access to the HANDLE for the file and not
> the posix file descriptor.
>
> It also has things like ReadFileScatter() (
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365469(v=vs.85).aspx)
> which is not the same, but might also be interesting as a future
> improvement.

That looks a lot like POSIX readv()
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/readv.html),
and as far as I can tell it has the same issue as it in that it doesn't
take an offset argument, but requires you to seek first.

Linux and modern BSDs however have preadv()
(http://manpages.ubuntu.com/manpages/xenial/en/man2/preadv.2.html),
which takes an offset and an iovec array.  I don't know if Windows and
other platforms have anything similar.

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


-- 
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] Set sgml-basic-offset to 1 in .dir-locals.el

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:

> On 7/6/16 4:52 AM, Dagfinn Ilmari Mannsåker wrote:
>> This keeps the indentation consistent when editing the documentation
>> using Emacs.
>
> Unfortunately, sgml-basic-offset is not a "safe" variable, so if we put
> it into .dir-locals.el, then users will always be bothered with a
> warning about that.

Ah, I see I've added it to safe-local-variables in my .emacs.  I must
have done that a while back (as evidenced by the date in the patch) and
forgotten about it.  Oh well, never mind.

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


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

[HACKERS] [PATCH] Set sgml-basic-offset to 1 in .dir-locals.el

[Dagfinn Ilmari Mannsåker]

This keeps the indentation consistent when editing the documentation
using Emacs.

>From c345671ae4704df500dd17719c5e9973001663c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Sat, 26 Mar 2016 21:58:32 +
Subject: [PATCH] Set sgml-basic-offset to 1 in .dir-locals.el

---
 .dir-locals.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.dir-locals.el b/.dir-locals.el
index d8827a6..9574300 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -16,4 +16,5 @@
(indent-tabs-mode . t)
(tab-width . 4)))
  (sgml-mode . ((fill-column . 78)
-   (indent-tabs-mode . nil
+   (indent-tabs-mode . nil)
+   (sgml-basic-offset . 1
-- 
2.8.1


-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

-- 
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] To-Do item: skip table scan for adding column with provable check constraints

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> Robert Haas  writes:
>> Right.  If there were a DEFAULT on the new column that would of course
>> be different, and you can also do thinks like CHECK (a != b) here.
>> However, if the CHECK constraint does not reference any column other
>> than the newly-added one, and if the new column will have the same
>> value for every row either because there is no default or because the
>> default is a constant,
>
> ... and if the CHECK expression is immutable ...

Doesn't it have to be already?  Otherwise a value accepted at one point
in time could suddenly violate the constraint later, even though it
never changed.


ilmari

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
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] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Marko Tiikkaja <ma...@joh.to> writes:

> On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote:
>> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>>
>>> I was bored and thought "how hard could it be?", and a few hours'
>>> hacking later, I have something that seems to work.  It doesn't do IF
>>> NOT EXISTS yet, and the error messaging could do with some improvement,
>>> and there are no docs.  The patch is attached, as well as at
>>> https://github.com/ilmari/postgres/commit/enum-alter-value
>>
>> Here's v3 of the patch of the patch, which I consider ready for proper
>> review.
>
> A couple of trivial comments below.

Thanks, all fixed locally and will be in the next version of the patch.

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


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

[HACKERS] [PATCH] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> I was bored and thought "how hard could it be?", and a few hours'
> hacking later, I have something that seems to work.  It doesn't do IF
> NOT EXISTS yet, and the error messaging could do with some improvement,
> and there are no docs.  The patch is attached, as well as at
> https://github.com/ilmari/postgres/commit/enum-alter-value

Here's v3 of the patch of the patch, which I consider ready for proper
review.  It now features:

- IF (NOT) EXISTS support
- Transaction support
- Documentation
- Improved error reporting (renaming a non-existent value to an existing
  one complains about the former, not the latter)

>From 0fee3bdc2e1f4022344e050969b993c963889f55 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20ALTER=20VALUE?=
 =?UTF-8?q?=20=E2=80=A6=20TO=20=E2=80=A6=20for=20enums?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unlike adding values to an enum, altering an existing value doesn't
affect the OID values or ordering, so can be done in a transaction.

IF EXISTS and IF NOT EXISTS are supported for ignoring the non-existence
of the old value or the existance of the new one, respectively.
---
 doc/src/sgml/ref/alter_type.sgml   |  24 +++-
 src/backend/catalog/pg_enum.c  | 117 +
 src/backend/commands/typecmds.c|  51 +---
 src/backend/parser/gram.y  |  18 ++
 src/include/catalog/pg_enum.h  |   3 +
 src/include/nodes/parsenodes.h |   2 +
 src/test/regress/expected/enum.out |  63 
 src/test/regress/sql/enum.sql  |  30 ++
 8 files changed, 284 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..f6b4e66 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -29,6 +29,7 @@ ALTER TYPE name RENAME ATTRIBUTE name RENAME TO new_name
 ALTER TYPE name SET SCHEMA new_schema
 ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ]
+ALTER TYPE name ALTER VALUE [ IF EXISTS ] existing_enum_value TO new_enum_value [ IF NOT EXISTS ]
 
 where action is one of:
 
@@ -124,6 +125,23 @@ ALTER TYPE name ADD VALUE [ IF NOT

 

+ALTER VALUE [ IF EXISTST ] TO [ IF NOT EXISTS ]
+
+ 
+  This form renames a value in an enum type.  The value's place in the
+  enum's ordering is not affected.
+ 
+ 
+  If IF EXISTS or is IF NOT EXISTS
+  is specified, it is not an error if the type doesn't contain the old
+  value or already contains the new value, respectively: a notice is
+  issued but no other action is taken.  Otherwise, an error will occur
+  if the old value is not alreeady present or the new value is.
+ 
+
+   
+
+   
 CASCADE
 
  
@@ -241,7 +259,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   new_enum_value
   

-The new value to be added to an enum type's list of values.
+The new value to be added to an enum type's list of values,
+or to rename an existing one to.
 Like all enum literals, it needs to be quoted.

   
@@ -252,7 +271,8 @@ ALTER TYPE name ADD VALUE [ IF NOT
   

 The existing enum value that the new value should be added immediately
-before or after in the enum type's sort ordering.
+before or after in the enum type's sort ordering,
+or renamed from.
 Like all enum literals, it needs to be quoted.

   
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..2e78294 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,123 @@ restart:
 
 
 /*
+ * RenameEnumLabel
+ *		Add a new label to the enum set. By default it goes at
+ *		the end, but the user can choose to place it before or
+ *		after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal,
+bool skipIfNotExists,
+bool skipIfExists)
+{
+	Relation	pg_enum;
+	HeapTuple	old_tup = NULL;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+	boolfound_new = false;
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enu

Re: [HACKERS] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> On 03/25/2016 04:13 AM, Matthias Kurz wrote:
>>
>> Hopefully at the commitfest at least the transaction limitation
>> will/could be tackled - that would help us a lot already.
>
> I don't believe anyone knows how to do that safely. Enums pose special
> problems here exactly because unlike all other types the set of valid
> values is mutable.

However, this problem (and the one described in the comments of
AlterEnum()) doesn't apply to altering the name, since that doesn't
affect the OID or the ordering.  Attached is version 2 of the patch,
which allows ALTER TYPE ... ALTER VALUE inside a transaction.

It still needs documentation, and possibly support for IF (NOT) EXISTS,
if people think that's useful.

>From 0a482f6d7434964ce51f66c260bde4a74dac4da0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] Add ALTER TYPE ... ALTER VALUE '...' TO '...'

Unlike adding values, altering a value can be done in a transaction,
since it doesn't affect the OID values or ordering, so is safe to
rollback.

TODO: documentation, IF (NOT) EXISTS support(?)
---
 src/backend/catalog/pg_enum.c  | 88 ++
 src/backend/commands/typecmds.c| 50 --
 src/backend/parser/gram.y  | 14 ++
 src/include/catalog/pg_enum.h  |  2 +
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/enum.out | 58 +
 src/test/regress/sql/enum.sql  | 27 
 7 files changed, 218 insertions(+), 22 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..1079e6c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,94 @@ restart:
 
 
 /*
+ * RenameEnumLabel
+ *		Add a new label to the enum set. By default it goes at
+ *		the end, but the user can choose to place it before or
+ *		after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal)
+{
+	Relation	pg_enum;
+	HeapTuple	old_tup = NULL;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+	boolfound_new = false;
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Without that, we couldn't be sure to get a consistent view
+	 * of the enum members via the syscache.  Note that this does not block
+	 * other backends from inspecting the type; see comments for
+	 * RenumberEnumType.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/* Locate the element to rename and check if the new label is already in
+	 * use.  The unique index on pg_enum would catch this anyway, but we
+	 * prefer a friendlier error message.
+	 */
+	for (nbr_index = 0; nbr_index < nelems; nbr_index++)
+	{
+		enum_tup = &(list->members[nbr_index]->tuple);
+		nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+		if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
+			old_tup = enum_tup;
+
+		if (strcmp(NameStr(nbr_en->enumlabel), newVal) == 0)
+			found_new = true;
+	}
+	if (!old_tup)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+		oldVal)));
+	if (found_new)
+		ereport(ERROR,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("enum label \"%s\" already exists",
+		newVal)));
+
+	enum_tup = heap_copytuple(old_tup);
+	nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+	ReleaseCatCacheList(list);
+
+	/* Update new pg_enum entry */
+	namestrcpy(_en->enumlabel, newVal);
+	simple_heap_update(pg_enum, _tup->t_self, enum_tup);
+	CatalogUpdateIndexes(pg_enum, enum_tup);
+	heap_freetuple(enum_tup);
+
+	/* Make the updates visible */
+	CommandCounterIncrement();
+
+	heap_close(pg_enum, RowExclusiveLock);
+}
+
+
+/*
  * RenumberEnumType
  *		Renumber existing enum elements to have sort positions 1..n.
  *
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 227d382..63f030b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1236,32 +1236,38 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
 	if (!HeapTupleIsValid(tup))
 		elog(ERROR, "cache lookup failed for type %u", enum_type_oid);
 
-	

Re: [HACKERS] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

>
> I was bored and thought "how hard could it be?", and a few hours'
> hacking later, I have something that seems to work.  It doesn't do IF
> NOT EXISTS yet, and the error messaging could do with some improvement,
> and there are no docs.  The patch is attached, as well as at
> https://github.com/ilmari/postgres/commit/enum-alter-value

I've added it to the 2016-09 commitfest as well:
https://commitfest.postgresql.org/10/588/

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen



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

Re: [HACKERS] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Matthias Kurz  writes:

[altering and dropping enum values]

>>> Andrew Dunstan  writes:
>>> > On 03/09/2016 11:07 AM, Tom Lane wrote:
>>> >> I have a vague recollection that we discussed this at the time the enum
>>> >> stuff went in, and there are concurrency issues?  Don't recall details
>>> >> though.
>>>
>>> > Rings a vague bell, but should it be any worse than adding new labels?
>>>
>>> I think what I was recalling is the hazards discussed in the comments for
>>> RenumberEnumType.  However, the problem there is that a backend could make
>>> inconsistent ordering decisions due to seeing two different pg_enum rows
>>> under different snapshots.  Updating a single row to change its name
>>> doesn't seem to have a comparable hazard, and it wouldn't affect ordering
>>> anyway.  So it's probably no worse than any other object-rename situation.
>>>
>>> regards, tom lane
>>>
>>
>>
> Is there a way or a procedure we can go through to make the these ALTER
> TYPE enhancements a higher priority? How do you choose which
> features/enhancements to implement (next)?

I was bored and thought "how hard could it be?", and a few hours'
hacking later, I have something that seems to work.  It doesn't do IF
NOT EXISTS yet, and the error messaging could do with some improvement,
and there are no docs.  The patch is attached, as well as at
https://github.com/ilmari/postgres/commit/enum-alter-value

>From 1cb8feac0179eaa44720822ad84e146ec3ba67ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] Add ALTER TYPE ... ALTER VALUE '...' TO '...'

Needs docs, and when altering a non-existent value to one that already
exists, it should probably complain about the former fact, not the
latter.
---
 src/backend/catalog/pg_enum.c  | 93 ++
 src/backend/commands/typecmds.c| 17 +--
 src/backend/parser/gram.y  | 14 ++
 src/include/catalog/pg_enum.h  |  2 +
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/enum.out | 22 +
 src/test/regress/sql/enum.sql  | 11 +
 7 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..81e170c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,99 @@ restart:
 
 
 /*
+ * RenameEnumLabel
+ *		Add a new label to the enum set. By default it goes at
+ *		the end, but the user can choose to place it before or
+ *		after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal)
+{
+	Relation	pg_enum;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+
+
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Without that, we couldn't be sure to get a consistent view
+	 * of the enum members via the syscache.  Note that this does not block
+	 * other backends from inspecting the type; see comments for
+	 * RenumberEnumType.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	/*
+	 * Check if label is already in use.  The unique index on pg_enum would
+	 * catch this anyway, but we prefer a friendlier error message.
+	 */
+	enum_tup = SearchSysCache2(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid),
+			   CStringGetDatum(newVal));
+	if (HeapTupleIsValid(enum_tup))
+	{
+		ReleaseSysCache(enum_tup);
+		ereport(ERROR,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("enum label \"%s\" already exists",
+		newVal)));
+	}
+
+	pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/* Locate the element to rename */
+	for (nbr_index = 0; nbr_index < nelems; nbr_index++)
+	{
+		enum_tup = &(list->members[nbr_index]->tuple);
+		nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+		if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
+			break;
+	}
+	if (nbr_index >= nelems)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+		oldVal)));
+
+	enum_tup = heap_copytuple(enum_tup);
+	nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+	ReleaseCatCacheList(list);
+
+	/* Update new pg_enum entry */
+	namestrcpy(_en->enumlabel, newVal);
+	simple_heap_update(pg_enum, 

Re: [HACKERS] [PATCH] Use correct types and limits for PL/Perl SPI query results

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> 1) Perl's integers are at least pointer-sized and either signed or
>>unsigned, so can potentially hold up to 2⁶⁴-1. Floating point numbers
>>can also be larger than double (up to 128bit), allowing for exact
>>representation of integers beyond 2⁵³.  Hence, adjust the setting of
>>the "processed" hash item to use UV_MAX for the limit and (NV) or
>>(UV) for the casts.
>
> I thought about using UV where feasible, but it was not clear to me
> whether unsigned numbers behave semantically differently from signed ones
> in Perl.  If they do, the change you suggest would create a small semantic
> change in the behavior of code using spi_exec_query.  I'm not sure it's
> worth taking any risk of that, considering we already discourage people
> from using spi_exec_query for large results.

IVs and UVs are semantically equivalent, and Perl will automatically
convert between them (and NVs) as necessary, i.e. when crossing
IV_MAX/UV_MAX/IV_MIN.

>> 2) Perl 5.20 and later use SSize_t for array indices, so can cope with
>>up to SSize_t_max items in an array (if you have the memory).
>
> I don't much like having such hard-wired knowledge about Perl versions
> in our code.  Is there a way to identify this change other than
> #if PERL_BCDVERSION >= 0x5019004 ?

There isn't, unfortunately.  I could add e.g. AVidx_t and _MAX defines,
but that wouldn't be available until 5.26 (May 2017) at the earliest,
since full code freeze for May's 5.24 is next week.

>> 3) To be able to actually return such result sets from sp_exec_query(),
>>I had to change the repalloc() in spi_printtup() to repalloc_huge().
>
> Hmm, good point.  I wonder if there are any hazards to doing that.

One hazard would be that people not heeding the warning in
spi_exec_query will get a visit from the OOM killer (or death by swap)
instead of a friendly "invalid memory alloc request size".

>   regards, tom lane

ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


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

[HACKERS] [PATCH] Use correct types and limits for PL/Perl SPI query results

[Dagfinn Ilmari Mannsåker]

Hi hackers,

Commit 23a27b039d94ba359286694831eafe03cd970eef changed the type of
numbers-of-tuples-processed counters to uint64 and adjusted various PLs
to cope with this.  I noticed the PL/Perl changes did not take full
advantage of what Perl is capable of handling, so here's a patch that
improves that.

1) Perl's integers are at least pointer-sized and either signed or
   unsigned, so can potentially hold up to 2⁶⁴-1. Floating point numbers
   can also be larger than double (up to 128bit), allowing for exact
   representation of integers beyond 2⁵³.  Hence, adjust the setting of
   the "processed" hash item to use UV_MAX for the limit and (NV) or
   (UV) for the casts.

2) Perl 5.20 and later use SSize_t for array indices, so can cope with
   up to SSize_t_max items in an array (if you have the memory).

3) To be able to actually return such result sets from sp_exec_query(),
   I had to change the repalloc() in spi_printtup() to repalloc_huge().

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

>From f6d38f1a5c7d6135dd5f580aa8ed38bb74162a0f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Sun, 13 Mar 2016 01:21:43 +
Subject: [PATCH] Use correct types and limits for PL/Perl SPI query results

Perl's integers are pointer-sized, so can hold more than INT_MAX on LP64
platforms, and come in both signed (IV) and unsigned (UV).  Floating
point values (NV) may also be larger than double.

Since Perl 5.19.4 array indices are SSize_t instead of I32, so allow up
to SSize_t_max on those versions.  The limit is not imposed just by
av_extend's argument type, but all the array handling code, so remove
the speculative comment.

Finally, change spi_printtup() to use repalloc_huge() to be able to
return a tuple table of more than 1GiB.
---
 src/backend/executor/spi.c |  2 +-
 src/pl/plperl/plperl.c | 13 -
 src/pl/plperl/plperl.h |  7 +++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 3d04c23..fd94179 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1800,7 +1800,7 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *self)
 		/* Double the size of the pointer array */
 		tuptable->free = tuptable->alloced;
 		tuptable->alloced += tuptable->free;
-		tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,
+		tuptable->vals = (HeapTuple *) repalloc_huge(tuptable->vals,
 	  tuptable->alloced * sizeof(HeapTuple));
 	}
 
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 269f7f3..d405179 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3104,9 +3104,9 @@ plperl_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 processed,
 	hv_store_string(result, "status",
 	cstr2sv(SPI_result_code_string(status)));
 	hv_store_string(result, "processed",
-	(processed > (uint64) INT_MAX) ?
-	newSVnv((double) processed) :
-	newSViv((int) processed));
+	(processed > (uint64) UV_MAX) ?
+	newSVnv((NV) processed) :
+	newSVuv((UV) processed));
 
 	if (status > 0 && tuptable)
 	{
@@ -3114,12 +3114,7 @@ plperl_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 processed,
 		SV		   *row;
 		uint64		i;
 
-		/*
-		 * av_extend's 2nd argument is declared I32.  It's possible we could
-		 * nonetheless push more than INT_MAX elements into a Perl array, but
-		 * let's just fail instead of trying.
-		 */
-		if (processed > (uint64) INT_MAX)
+		if (processed > AV_SIZE_MAX)
 			ereport(ERROR,
 	(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 			errmsg("query result has too many rows to fit in a Perl array")));
diff --git a/src/pl/plperl/plperl.h b/src/pl/plperl/plperl.h
index 9179bbd..2f376ee 100644
--- a/src/pl/plperl/plperl.h
+++ b/src/pl/plperl/plperl.h
@@ -100,4 +100,11 @@ void		plperl_spi_freeplan(char *);
 void		plperl_spi_cursor_close(char *);
 char	   *plperl_sv_to_literal(SV *, char *);
 
+/* Perl 5.19.4 changed array indices from I32 to SSize_t */
+#if PERL_BCDVERSION >= 0x5019004
+#define AV_SIZE_MAX ((uint64) SSize_t_MAX)
+#else
+#define AV_SIZE_MAX ((uint64) I32_MAX)
+#endif
+
 #endif   /* PL_PERL_H */
-- 
2.7.0


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

[HACKERS] [PATCH] Obsolete wording in PL/Perl comment

[Dagfinn Ilmari Mannsåker]

The comment in hv_store_string() says that negative key length to
hv_store() for UTF-8 is not documented, and mentions that 5.6 doesn't
track UTF-8-ness of keys.  However, the negative length convention has
been documented since 5.16¹, and 5.6 is no longer supported.  The
attached patch updates the comment to reflect this.

[1]: http://perldoc.perl.org/perlapi.html#hv_store

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law

>From 9dfe4c076b5040557dec694dee91701438d658fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Sun, 13 Mar 2016 02:46:45 +
Subject: [PATCH] Fix obsolete wording in PL/Perl hv_store_string comment

Negative klen is documented since Perl 5.16, and 5.6 is no longer
supported.
---
 src/pl/plperl/plperl.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 269f7f3..a659695 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3911,10 +3911,8 @@ hv_store_string(HV *hv, const char *key, SV *val)
 	hkey = pg_server_to_any(key, strlen(key), PG_UTF8);
 
 	/*
-	 * This seems nowhere documented, but under Perl 5.8.0 and up, hv_store()
-	 * recognizes a negative klen parameter as meaning a UTF-8 encoded key. It
-	 * does not appear that hashes track UTF-8-ness of keys at all in Perl
-	 * 5.6.
+	 * hv_store() recognizes a negative klen parameter as meaning a UTF-8
+	 * encoded key
 	 */
 	hlen = -(int) strlen(hkey);
 	ret = hv_store(hv, hkey, hlen, val, 0);
-- 
2.7.0


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