Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Sun, Jan 08, 2017 at 06:50:12PM -0600, Jim Nasby wrote:
> On 1/5/17 12:04 AM, David Fetter wrote:
> > +errmsg("UPDATE requires a WHERE clause 
> > when require_where.delete is set to on"),
> 
> ISTM that message is no longer true.
> 
> The second if could also be an else if too.

I refactored this into one case and removed some fluff, some of which
was never needed, some of which is no longer.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
commit 123f2c48e67da6a3693decfae2722fa1a686f48d
Author: David Fetter 
Date:   Thu Jul 21 23:34:21 2016 -0700

require_where: a contrib hook

This adds a process utility hook which makes simple UPDATE and DELETE
statements require a WHERE clause when loaded.

It is not intended to provide a general capability.  Instead, its job is to
prevent common human errors made by people who only rarely use SQL.  The 
hook
is small enough to be usable as part of a short lesson on hooks.

diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..4bd456f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -40,6 +40,7 @@ SUBDIRS = \
pgstattuple \
pg_visibility   \
postgres_fdw\
+   require_where   \
seg \
spi \
tablefunc   \
diff --git a/contrib/require_where/Makefile b/contrib/require_where/Makefile
new file mode 100644
index 000..933eb00
--- /dev/null
+++ b/contrib/require_where/Makefile
@@ -0,0 +1,19 @@
+# contrib/require_where/Makefile
+
+MODULE_big = require_where
+OBJS = require_where.o
+
+PGFILEDESC = 'require_where - require simple DELETEs and UPDATEs to have a 
WHERE clause'
+
+REGRESS = require_where
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS = $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/require_where
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_builddir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/require_where/expected/require_where.out 
b/contrib/require_where/expected/require_where.out
new file mode 100644
index 000..1884722
--- /dev/null
+++ b/contrib/require_where/expected/require_where.out
@@ -0,0 +1,16 @@
+--
+--   Test require_where
+--
+\set echo all
+CREATE TABLE test_require_where(t TEXT);
+UPDATE test_require_where SET t=t; -- succeeds
+DELETE FROM test_require_where; -- succeeds
+LOAD 'require_where';
+UPDATE test_require_where SET t=t; -- fails
+ERROR:  UPDATE requires a WHERE clause when the require_where hook is enabled.
+HINT:  To update all rows, use "WHERE true" or similar.
+UPDATE test_require_where SET t=t WHERE true; -- succeeds
+DELETE FROM test_require_where; -- fails
+ERROR:  DELETE requires a WHERE clause when the require_where hook is enabled.
+HINT:  To delete all rows, use "WHERE true" or similar.
+DELETE FROM test_require_where WHERE true; -- succeeds
diff --git a/contrib/require_where/require_where.c 
b/contrib/require_where/require_where.c
new file mode 100644
index 000..9eae929
--- /dev/null
+++ b/contrib/require_where/require_where.c
@@ -0,0 +1,62 @@
+/*
+ * --
+ *
+ * require_where.c
+ *
+ * Copyright (C) 2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/require_where/require_where.c
+ *
+ * --
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+
+#include "parser/analyze.h"
+
+#include "utils/elog.h"
+
+PG_MODULE_MAGIC;
+
+void   _PG_init(void);
+void   _PG_fini(void);
+
+static post_parse_analyze_hook_type original_post_parse_analyze_hook = 
NULL;
+
+/*
+ * This module makes simple UPDATE and DELETE statements require a WHERE clause
+ * and complains when this is not present.
+ */
+static void
+require_where_check(ParseState *pstate, Query *query)
+{
+
+   if (query->commandType == CMD_UPDATE || query->commandType == 
CMD_DELETE)
+   {
+   /* Make sure there's something to look at. */
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("%s requires a WHERE clause 
when the require_where hook is enabled.",
+query->commandType == 
CMD_UPDATE ? "UPDATE" : "DELETE"),
+errhint("To %s all rows, use \"WHERE 
true\" or similar.",
+  

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Mon, Jan 09, 2017 at 07:52:11PM -0300, Alvaro Herrera wrote:
> David Fetter wrote:
> 
> > +   if (query->commandType == CMD_UPDATE || query->commandType == 
> > CMD_DELETE)
> > +   {
> > +   /* Make sure there's something to look at. */
> > +   Assert(query->jointree != NULL);
> > +   if (query->jointree->quals == NULL)
> > +   ereport(ERROR,
> > +   (errcode(ERRCODE_SYNTAX_ERROR),
> > +errmsg("%s requires a WHERE clause 
> > when the require_where hook is enabled.",
> > +query->commandType == 
> > CMD_UPDATE ? "UPDATE" : "DELETE"),
> > +errhint("To %s all rows, use \"WHERE 
> > true\" or similar.",
> > +query->commandType == 
> > CMD_UPDATE ? "update" : "delete")));
> > +   }
> 
> Per my earlier comment, I think this should use
> ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED instead.
> 
> I think this should say "the \"require_hook\" extension" rather than
> use the term "hook".
> 
> (There are two or three translatability rules violations in this
> snippet, but since this is an extension and those are not translatable,
> I won't say elaborate further.)

I'm happy to fix it.  Are the rules all in one spot I can refer to?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] merging some features from plpgsql2 project

[Marko Tiikkaja]

On Tue, Jan 10, 2017 at 1:03 AM, Jim Nasby  wrote:

> On 1/9/17 5:53 PM, Marko Tiikkaja wrote:
>
>> My idea was that the currently unsupported combination of NOT
>> NULL and
>> no DEFAULT would mean "has to be assigned to a non-NULL value
>> before it
>> can be read from, or an exception is thrown".  Solves the most
>> common
>> use case and is backwards compatible.
>>
>>
>> That won't allow you to use a variable in multiple places though...
>> is there a reason we couldn't support something like IS DEFINED and
>> UNSET?
>>
>>
>> I don't understand what your use case is.  Could you demonstrate that
>> with some code you'd write if these features were in?
>>
>
> One use case is NEW and OLD in triggers. Checking to see if one or the
> other is set is easier than checking TG_OP. It's also going to be faster
> (probably MUCH faster; IIRC the comparison currently happens via SPI).
>

This sounds useless.


> Another case is selecting into a record:
>
> EXECUTE ... INTO rec;
> IF rec IS DEFINED THEN
> ELSE
>   EXECUTE  INTO rec;
>   IF rec IS DEFINED THEN
>

And this a workaround for non-functional FOUND.

I can't get excited about this idea based on these examples.


.m

Re: [HACKERS] ICU integration

[Peter Geoghegan]

On Mon, Jan 9, 2017 at 12:25 PM, Peter Eisentraut
 wrote:
> On 1/7/17 10:01 PM, Peter Geoghegan wrote:
>> It occurs to me that the comparison caching stuff added by commit
>> 0e57b4d8b needs to be considered here, too.

> That might be worth looking into, but it seems a bit daunting to
> construct a benchmark specifically for this, unless we have the one that
> was originally used lying around somewhere.

The benchmark used when 0e57b4d8b went in only had to prove that there
was no measurable overhead when the optimization didn't help (It was
quite clear that it was worthwhile in good cases). I think that
comparison caching will continue to be about as effective as before in
good cases, but you don't do comparison caching anymore. That might be
fine, but let's be sure that that's the right trade-off.

To demonstrate the effectiveness of the patch, I used this cities sample data:

http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/data/cities.dump

Test query: select country, province, count(*) from cities group by
rollup (country, province);

This was shown to be about 25% faster, although that was with
abbreviated keys (plus caching of abbreviated keys), and not just the
comparison caching optimization.

-- 
Peter Geoghegan


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

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

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

I wrote:

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


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

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


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


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


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

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Thomas Munro]

On Sat, Jan 7, 2017 at 9:01 AM, Thomas Munro
 wrote:
> On Tue, Jan 3, 2017 at 10:53 PM, Thomas Munro
>  wrote:
>> I will post a new rebased version soon with that and
>> some other nearby problems fixed.
>
> Here is a new WIP patch.

To make this easier to understand and harmonise the logic used in a
few places, I'm now planning to chop it up into a patch series,
probably something like this:

1.  Change existing hash join code to use chunk-based accounting
2.  Change existing hash join code to use a new interface for dealing
with batches
3.  Add shared hash join support, single batch only
4.  Add components for doing shared batch reading (unused)
5.  Add multi-batch shared hash join support

-- 
Thomas Munro
http://www.enterprisedb.com


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

Re: [HACKERS] merging some features from plpgsql2 project

[Marko Tiikkaja]

On Tue, Jan 10, 2017 at 12:47 AM, Jim Nasby 
wrote:

> On 1/9/17 5:30 PM, Marko Tiikkaja wrote:
>
My idea was that the currently unsupported combination of NOT NULL and
>> no DEFAULT would mean "has to be assigned to a non-NULL value before it
>> can be read from, or an exception is thrown".  Solves the most common
>> use case and is backwards compatible.
>>
>
> That won't allow you to use a variable in multiple places though... is
> there a reason we couldn't support something like IS DEFINED and UNSET?
>

I don't understand what your use case is.  Could you demonstrate that with
some code you'd write if these features were in?


.m

Re: [HACKERS] merging some features from plpgsql2 project

[Jim Nasby]

On 1/9/17 5:12 PM, Merlin Moncure wrote:

Agreed: If you want to break compatibility, pushing a new language is
the better way than GUC.   If you got consensus on this, having both
languages side by side supported for a while (maybe 4-5 releases) is
they way to go, and finally the only language is frozen and moved to
extension.  But this is a lot of work and aggravation, are you *sure*
you can only get what you want with a full compatibility break?


FWIW, that work and aggravation part is what I hoped to avoid with GUCs.

I do think that whichever route we go, we're going to be stuck 
supporting the old version for a LONG time. A big part of why 
standard_conforming_strings was so ugly is users didn't have enough time 
to adjust. If we'd had that enabled by default for 4-5 releases it 
wouldn't have been nearly as much of an issue.

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


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

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

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

Jim Nasby  writes:

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


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


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


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


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



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


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

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

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


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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Alvaro Herrera]

David Fetter wrote:

> + if (query->commandType == CMD_UPDATE || query->commandType == 
> CMD_DELETE)
> + {
> + /* Make sure there's something to look at. */
> + Assert(query->jointree != NULL);
> + if (query->jointree->quals == NULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +  errmsg("%s requires a WHERE clause 
> when the require_where hook is enabled.",
> +  query->commandType == 
> CMD_UPDATE ? "UPDATE" : "DELETE"),
> +  errhint("To %s all rows, use \"WHERE 
> true\" or similar.",
> +  query->commandType == 
> CMD_UPDATE ? "update" : "delete")));
> + }

Per my earlier comment, I think this should use
ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED instead.

I think this should say "the \"require_hook\" extension" rather than
use the term "hook".

(There are two or three translatability rules violations in this
snippet, but since this is an extension and those are not translatable,
I won't say elaborate further.)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] merging some features from plpgsql2 project

[Merlin Moncure]

On Sun, Jan 8, 2017 at 2:52 AM, Joel Jacobson  wrote:
> On Sat, Jan 7, 2017 at 8:56 PM, Pavel Stehule  wrote:
>>
>> Jim, Marko, Joel - is there a place, features where we can find a partial 
>> agreement? If it is, then we can move our view there.
>
> I have decided I definitively want a new language, and I'm willing to
> pay for it.

well, as they say, "money talks" :-D.

> Hopefully the community will join forces and contribute with ideas and
> code, but with or without you or the rest of the community, plpgsql2
> is going to happen.
> Call it pltrustly or plpgsql2, I don't care. I just care about ending
> my suffering from being forced writing plpgsql every day. It sucks,
> and I'm going to end it.

Curious, are you mainly troubled by the 'INTO STRICT' family of
problems? Or something else?  Pavel has scored some points with PRAGMA
syntax and ISTM that does not require compatibility break.

> And please kill all these GUCs ideas. The best thing with PostgreSQL
> is the natural expected behaviour of the default configuration.
> Contrary to MySQL where you have to enable lots and lots of
> configuration options just to get a behaviour you expect as a novice
> user.

I think there is a lot of support for this point of view.  Jim is
notable outlier here, but for the most part we don't do language
behavior changes with GUC.

> It's much better to just come together and agree on whatever we have
> learned during the last 15 years of PL/pgSQL1, and sample all ideas
> during a year maybe, and decide what to put into PL/pgSQL2. To make it
> useful, we should aim to not break compatibility for _most_ code, but
> accept some necessary rewrites of functions with deprecated
> anti-patterns.

Agreed: If you want to break compatibility, pushing a new language is
the better way than GUC.   If you got consensus on this, having both
languages side by side supported for a while (maybe 4-5 releases) is
they way to go, and finally the only language is frozen and moved to
extension.  But this is a lot of work and aggravation, are you *sure*
you can only get what you want with a full compatibility break?

With respect to your company developers specifically?  I'm genuinely
curious if you've taken a good look at pl/v8 and why you've determined
it's not suitable to move forward with.  It's got a different set of
headaches, but is really fast, and sometimes wonder if with some
alternative preprocessing (like coffeescript but geared towards SQL)
could have some long term promise.

merlin


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

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

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

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

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

regards, tom lane


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

Re: [HACKERS] merging some features from plpgsql2 project

[Jim Nasby]

On 1/9/17 6:07 PM, Marko Tiikkaja wrote:

One use case is NEW and OLD in triggers. Checking to see if one or
the other is set is easier than checking TG_OP. It's also going to
be faster (probably MUCH faster; IIRC the comparison currently
happens via SPI).


This sounds useless.


I guess you've not written much non-trivial trigger code then... the 
amount of code duplication you end up with is quite ridiculous. It's 
also a good example of why treating this as an exception and trapping 
isn't a good solution either: you can already do that with triggers today.


Being able to check the existence of a variable is a very common idiom 
in other languages, so I'm don't see why plpgsql shouldn't have it.

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


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

Re: [HACKERS] ALTER TABLE .. ALTER COLUMN .. ERROR: attribute .. has wrong type

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Tom Lane wrote:
> >> [ pokes around... ]  The code I was thinking of is convert_tuples_by_name
> >> in access/common/tupconvert.c.  There's a bit of an API mismatch in that
> >> it wants to wrap the mapping array in a TupleConversionMap struct; but
> >> maybe we could refactor tupconvert.c to offer a way to get just the map
> >> array.
> 
> > Ah, nice gadget.  I think the attached patch should do.
> 
> Looks reasonable to me.

Thanks for looking!  Pushed.

> > Hm, I was thinking in unreasonable numbers of columns, keeping in mind
> > that they can appear in arbitrary order in child tables.  Then again,
> > that probably seldom occurs in real databases.  I suppose this could
> > become an issue with table partitioning becoming more common, but I'm
> > okay with deferring the optimization work.
> 
> It occurred to me that it'd be really easy to improve
> convert_tuples_by_name so that, rather than having the inner loop
> start from j = 0 every time, it starts from the attribute after the
> last match (and loops around if needed, so that it still examines
> every child attribute).  I think this would keep it at more-or-less
> linear time for all but very contrived child tables.
> 
> Since your patch is touching that code I won't do anything about it
> right now, but maybe later.

Yeah, I had the same idea.  Looks fiddly but not terribly difficult, and
well localized.  Didn't we have a list of tasks for eager contributors?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] merging some features from plpgsql2 project

[Marko Tiikkaja]

On Mon, Jan 9, 2017 at 12:37 AM, Jim Nasby  wrote:

> If we're going to create a brand new language then I think it would be
> extremely foolish to keep *any* of the current pain points around. Off the
> top of my head:
>
> - variables must have an identifier (what $ in most languages does). The
> steps you have to go through to avoid simple naming collisions are insane.
>

This is exactly what we did not want to do with this project.  The idea is
to create a language which is really close to PL/PgSQL, but removes some of
the brain diarrhoea currently present.

Now, this *is* a problem, and the solution we had (well I, mostly, at this
point) in mind is to use the underscore prefix for all input variables and
make OUT parameters invisible to queries inside function bodies unless
explicitly prefixed with   OUT.  As far as I can tell this eliminates most
if not all collisions while staying almost completely compatible with
arguably well-written PL/PgSQL 1.

- Support for the notion of a variable being unset (which is NOT the same
> thing as NULL).
>

My idea was that the currently unsupported combination of NOT NULL and no
DEFAULT would mean "has to be assigned to a non-NULL value before it can be
read from, or an exception is thrown".  Solves the most common use case and
is backwards compatible.


.m

Re: [HACKERS] merging some features from plpgsql2 project

[Jim Nasby]

On 1/9/17 3:01 AM, Pavel Stehule wrote:

You are forgot on function paramaters  - somebody can use a function
argument  like FOUND, .. So auto variables should to be declared in most
top namespace.


Right, that's why I said it was an alternative. I agree it would be 
better to just have 2 explicit namespaces: the top one being auto 
variables and the one below that being function arguments. The namespace 
below that would be the top-most *user* block.


Both of the pre-defined namespaces need the ability to change their 
name; I don't see any issue with using PRAGMA for that.

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


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

Re: [HACKERS] merging some features from plpgsql2 project

[Jim Nasby]

On 1/9/17 5:53 PM, Marko Tiikkaja wrote:

My idea was that the currently unsupported combination of NOT
NULL and
no DEFAULT would mean "has to be assigned to a non-NULL value
before it
can be read from, or an exception is thrown".  Solves the most
common
use case and is backwards compatible.


That won't allow you to use a variable in multiple places though...
is there a reason we couldn't support something like IS DEFINED and
UNSET?


I don't understand what your use case is.  Could you demonstrate that
with some code you'd write if these features were in?


One use case is NEW and OLD in triggers. Checking to see if one or the 
other is set is easier than checking TG_OP. It's also going to be faster 
(probably MUCH faster; IIRC the comparison currently happens via SPI).


Another case is selecting into a record:

EXECUTE ... INTO rec;
IF rec IS DEFINED THEN
ELSE
  EXECUTE  INTO rec;
  IF rec IS DEFINED THEN
...

Perhaps DEFINED is not the best keyword. Ultimately I want to know if a 
variable has been assigned a value, as well as being able to mark a 
variable as unassigned (though arguably you might not need to be able to 
un-assign...).

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


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

Re: [HACKERS] proposal: session server side variables

[Robert Haas]

On Thu, Jan 5, 2017 at 5:39 AM, Fabien COELHO  wrote:
> Half-persistence (in definition, not in value) is not a key feature needed
> by the use-case.

Well, you don't get to decide that.  You've been told by at least
three or four people that they don't want variables to be
transactional, you've been pointed to documentation links showing that
in other database systems including Oracle variables are not
transactional, and you still insist that this proposal is senseless
unless variables are transactional.  You have every right to decide
what you think is useful, but you don't have a right to decide what
other people think is useful.  You don't get veto power over what
Pavel wants to implement, even if you personally would not choose to
implement it that way, and especially not when multiple people are
agreeing with Pavel and disagreeing with you.

On the substance of this issue, I would note that Pavel's idea of
entering variables in pg_class has a number of advantages:

1. The code can control permissions using the same system that we use
to control permissions on all other objects, instead of having to
create a completely new one.  Similarly for dependencies.  This
wouldn't matter if it were possible to get by with no system for
privileges on variables or with a very simple system such as you
proposed upthread, but I think that's somewhat unrealistic in the face
of security-definer functions and row-level security.  Execution in
multiple privilege contexts within the same session is a fact of life,
and whatever design gets chosen has to somehow cope with that; using
the existing infrastructure is one reasonable choice.  Trying to do
something excessively simple here will result in security bugs.

2. The user doesn't need to re-declare the variables you want to use
at the beginning of every session.  This is also the reason why many
people want global temporary tables.  They don't do anything that
can't be done with local temporary tables; they're just more
convenient to use.

3. If somebody goes to drop a type, they'll have to use CASCADE to get
rid of the dependent variable.  That might tip them off to conduct an
investigation into whether that variables is being used.  If the
variables are established entirely on a per-session basis, there will
be no such warning at DROP time.  You'll only find out about the
problem when the application starts failing.

4. As Pavel already said, it makes things easier for a static checker.
I'm not certain that anyone, including you, has correctly understood
Pavel's point here, which is perhaps because Pavel's first language is
not English.  But I'm pretty sure I understand it, so let me try to
restate it more straightforwardly.  Suppose a static checker
encounters the statement SET VARIABLE flumpf = 1 (assume for purposes
of this bullet point that this is the syntax for setting a variable).
If variables have to be catalogued, and flumpf is not in the catalog,
this is a probably a bug.  If flumpf is in the catalog but is of a
type that cannot contain the value 1, this is also probably a bug.
But if variables do not have to be catalogued, then the static checker
will have a hard time inferring anything about the correctness of this
statement.  It is not impossible; for example, the static checker
could have a requirement that the user running it set up the session
with all required variables before running the checker.  But that is
not particularly convenient.  IIUC, Pavel's point is that a user who
creates a function which uses a certain variable would probably also
put code in that same function to create the variable if it is not yet
present.  And a static checker probably can't see through that.
You've argued that there is no problem here, but it seems absolutely
unarguable to me that static checkers benefit from having more things
statically configured and less stuff figured out at runtime.  That's
why it's easier to statically check programming languages with
strongly typed variables than it is to statically check languages with
run-time typing.

Now, there are certainly advantages to NOT entering variables in
pg_class, too.  For example, it makes the operation much more
light-weight.  If the code uses basically the same variables over and
over again, entering them in the catalog doesn't cost anything
meaningful.  If it's constantly creating new variables, that's going
to create catalog bloat if those variables have to be added to the
catalogs, and that's going to stink.  I don't mean to pass judgement
here in favor of Pavel's proposal and against other proposals.  But it
seems clear to me that Pavel's proposal does have certain fairly
compelling advantages and a good deal of community support; and your
replies don't seem to be acknowledging any of that.  I think they
should.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To 

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

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

Jim Nasby  writes:

Got a stack trace. The abort happens in TclObjLookupVar:


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


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


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


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

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


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

Re: [HACKERS] merging some features from plpgsql2 project

[Andrew Dunstan]

On 01/09/2017 06:12 PM, Merlin Moncure wrote:

With respect to your company developers specifically?  I'm genuinely
curious if you've taken a good look at pl/v8 and why you've determined
it's not suitable to move forward with.  It's got a different set of
headaches, but is really fast, and sometimes wonder if with some
alternative preprocessing (like coffeescript but geared towards SQL)
could have some long term promise.




Yeah, especially if built against a modern V8, with all or most of the 
ES6 stuff. Without template strings and lexically scoped variables it's 
very unpleasant for large functions, but with them it's usable.  It's 
also something a very large number of people are familiar with. As you 
say it's damn fast.


cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



--
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 pg_basebackup -x stream the default

[Michael Paquier]

On Tue, Jan 10, 2017 at 12:05 AM, Magnus Hagander  wrote:
> OK. Pushed. I agree it made it more readable, if nothing else.

Thanks.
-- 
Michael


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

Re: [HACKERS] merging some features from plpgsql2 project

[Jim Nasby]

On 1/9/17 5:30 PM, Marko Tiikkaja wrote:


This is exactly what we did not want to do with this project.  The idea
is to create a language which is really close to PL/PgSQL, but removes
some of the brain diarrhoea currently present.


As a general comment, ISTM it'd be much better to do as much as we can 
in the current language then. It's going to take a LOT to get people to 
switch to a different language, so there needs to be a LOT of added value.



Now, this *is* a problem, and the solution we had (well I, mostly, at
this point) in mind is to use the underscore prefix for all input
variables and make OUT parameters invisible to queries inside function
bodies unless explicitly prefixed with   OUT.  As far as I can tell this
eliminates most if not all collisions while staying almost completely
compatible with arguably well-written PL/PgSQL 1.


That might be workable... it's still rather ugly though.

I don't see prefixing everything with _ as being useful though; people 
can already do that if they want to uglify the function's argument names.


I do think there's stuff that could be done along these lines with 
namespaces though. Allowing users to rename the namespace that arguments 
went into would be a huge step forward. I think having a separate 
namespace for all the automatic variables would be a big help too.


Amusingly, that would allow users to set the namespace to '$', which 
would (almost) give you $variable.



- Support for the notion of a variable being unset (which is NOT the
same thing as NULL).


My idea was that the currently unsupported combination of NOT NULL and
no DEFAULT would mean "has to be assigned to a non-NULL value before it
can be read from, or an exception is thrown".  Solves the most common
use case and is backwards compatible.


That won't allow you to use a variable in multiple places though... is 
there a reason we couldn't support something like IS DEFINED and UNSET?

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


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

Re: [HACKERS] RustgreSQL

[Jim Nasby]

On 1/9/17 3:15 AM, Pavel Stehule wrote:

The running unsafe PL in own managed processes is good idea - Years, I
have a one diploma theme "better management of unsafe PL in Postgres" -
but still without any interest from students :(.  I had two
possibilities to see catastrophic errors related to wrong usage of
PLPerlu. If we can locks interpret/environment in some safe sandbox,
then it should be great.


Incidentally, Tom just observed in another thread that Tcl treats out of 
memory as a panic situation, so anyone using pltcl opens the risk of 
arbitrary database-wide panics. Fenced pltcl is one possible solution 
for that problem.

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


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Bruce Momjian]

On Mon, Jan  9, 2017 at 01:34:03PM -0500, Robert Haas wrote:
> On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
>  wrote:
> > On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> >> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> >> this idea more than COMMENT ON CURRENT DATABASE.
> >
> > We already have the reserved key word CURRENT_CATALOG, which is the
> > standard spelling.  But I wouldn't be bothered if we made
> > CURRENT_DATABASE somewhat reserved as well.
> 
> Maybe I'm just lacking in imagination, but what's the argument against
> spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
> anything new at all, and it also looks more SQL-ish to me.  SQL
> generally tries to emulate English, and I don't normally
> speak_hyphenated_words.

I assume it is to match our use of CURRENT_USER as having special
meaning.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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 for changes to recovery.conf API

[Peter Eisentraut]

On 1/1/17 4:14 PM, Simon Riggs wrote:
> OK, so here's the patch, plus doc cleanup patch.

I don't think this patch is likely to succeed if we throw in more ideas
in every round.

I think we have or are approaching agreement on moving recovery.conf
into postgresql.conf, making the settings reloadable, and adding signal
(formerly trigger) files to trigger recovery or standby.  I think that
is a useful change, but it's already big enough and needs extensive
reviewing and testing.

All the other stuff, such as regrouping the recovery parameters,
removing the hot_standby setting, renaming the primary_* parameters,
making the directory of the signal files configurable, should be
separate patches that should be discussed separately.  I think the
arguments for these latter changes are weaker, and tactically I would
focus on getting the recovery.conf move in before thinking about all the
other ones.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] Incorrect XLogRegisterBuffer flag for revmapbuf in brin

[Alvaro Herrera]

Alvaro Herrera wrote:

> If you examine the revmap in a replica after running the script below,
> you'd observe it's different than the one in the master.  I confirmed
> that the proposed patch fixes the problem.

Pushed.  Thanks for the report!

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] increasing the default WAL segment size

[David Rowley]

On 10 January 2017 at 07:40, Robert Haas  wrote:
> On Sat, Jan 7, 2017 at 7:45 PM, Jim Nasby  wrote:
>> Well, now that there's 3 places that need to do almost the same thing, I
>> think it'd be best to just centralize this somewhere. I realize that's not
>> going to save any significant amount of code, but it would make it crystal
>> clear what's going on (assuming the excellent comment above RIGHTMOST_ONE
>> was kept).
>
> Hmm.  This sounds a lot like what fls() and my_log2() also do.  I've
> been quietly advocating for fls() because we only provide an
> implementation in src/port if the operating system doesn't have it,
> and the operating system may have an implementation that optimizes to
> a single machine-language instruction (bsrl on x86, I think, see
> 4f658dc851a73fc309a61be2503c29ed78a1592e).  But the fact that our
> src/port implementation uses a loop instead of the RIGHTMOST_ONE()
> trick seems non-optimal.

It does really sound like we need a bitutils.c as mentioned in [1].
It would be good to make use of GCC's __builtin_popcount [2] instead
of the number_of_ones[] array in bitmapset.c. It should be a bit
faster and less cache polluting.

[1] https://www.postgresql.org/message-id/14578.1462595...@sss.pgh.pa.us
[2] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html


-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] sequence data type

[Peter Eisentraut]

On 1/8/17 2:39 PM, Steve Singer wrote:
> The only concern I have with the functionality is that there isn't a way 
> to change the type of a sequence.

If we implement the NEXT VALUE FOR expression (or anything similar that
returns a value from the sequence), then the return type of that
expression would be the type of the sequence.  Then, changing the type
of the sequence would require reparsing all expressions involving the
sequence.  This could probably be sorted out somehow, but I don't want
to be too lax now and cause problems for later features.  There is a
similar case, namely changing the return type of a function, which we
also prohibit.

> @@ -1236,7 +1239,15 @@ init_params(ParseState *pstate, List *options, 
> bool isInit,
>  {
>  DefElem*defel = (DefElem *) lfirst(option);
> 
> -   if (strcmp(defel->defname, "increment") == 0)
> +   if (strcmp(defel->defname, "as") == 0)
> +   {
> +   if (as_type)
> +   ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("conflicting or 
> redundant options")));
> +   as_type = defel;
> +   }
> +   else if (strcmp(defel->defname, "increment") == 0)
> 
> Should we including parser_errposition(pstate, defel->location)));  like 
> for the other errors below this?

Yes, good catch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] guc-ify the formerly hard-coded MAX_SEND_SIZE to max_wal_send

[Jonathon Nelson]

On Sun, Jan 8, 2017 at 11:36 AM, Greg Stark  wrote:

> On 8 January 2017 at 17:26, Greg Stark  wrote:
> > On 5 January 2017 at 19:01, Andres Freund  wrote:
> >> That's a bit odd - shouldn't the OS network stack take care of this in
> >> both cases?  I mean either is too big for TCP packets (including jumbo
> >> frames).  What type of OS and network is involved here?
> >
> > 2x may be plausible. The first 128k goes out, then the rest queues up
> > until the first ack comes back. Then the next 128kB goes out again
> > without waiting... I think this is what Nagle is supposed to actually
> > address but either it may be off by default these days or our usage
> > pattern may be defeating it in some way.
>
> Hm. That wasn't very clear.  And the more I think about it, it's not right.
>
> The first block of data -- one byte in the worst case, 128kB in our
> case -- gets put in the output buffers and since there's nothing
> stopping it it immediately gets sent out. Then all the subsequent data
> gets put in output buffers but buffers up due to Nagle. Until there's
> a full packet of data buffered, the ack arrives, or the timeout
> expires, at which point the buffered data drains efficiently in full
> packets. Eventually it all drains away and the next 128kB arrives and
> is sent out immediately.
>
> So most packets are full size with the occasional 128kB packet thrown
> in whenever the buffer empties. And I think even when the 128kB packet
> is pending Nagle only stops small packets, not full packets, and the
> window should allow more than one packet of data to be pending.
>
> So, uh, forget what I said. Nagle should be our friend here.
>

[I have not done a rigid analysis, here, but...]

I *think* libpq is the culprit here.

walsender says "Hey, libpq - please send (up to) 128KB of data!" and
doesn't "return" until it's "sent". Then it sends more.  Regardless of the
underlying cause (nagle, tcp congestion control algorithms, umpteen
different combos of hardware and settings, etc..) in almost every test I
saw improvement (usually quite a bit). This was most easily observable with
high bandwidth-delay product links, but my time in the lab is somewhat
limited.

I calculated "performance" the most simple measurement possible: how long
did it take for Y volume of data to get transferred, performed over a
long-enough interval (typically 1800 seconds) for TCP windows to open up,
etc...

-- 
Jon Nelson
Dyn / Principal Software Engineer

Re: [HACKERS] Incorrect XLogRegisterBuffer flag for revmapbuf in brin

[Alvaro Herrera]

Alvaro Herrera wrote:
> Kuntal Ghosh wrote:
> > Hi all,
> > 
> > In brin_doupdate(line 290), REGBUF_STANDARD is used to register
> > revmap buffer reference in WAL record. But, revmap buffer page doesn't
> > have a standard page layout and it doesn't update pd_upper and
> > pd_lower as well.
> 
> Hmm.  This bug should be causing WAL replay to zero out the revmap page
> contents, since essentially the whole page is covered by the "hole" in
> standard pages.  I can't see what is causing that not to happen, but
> evidently it isn't, since the index works in a replica.  What am I
> missing?

Ah, I figured it out, and I can reproduce that this bug loses the BRIN
data, effectively corrupting the index -- but AFAICS user queries would
not return corrupted results, because since all the revmap entries
become zeroes, BRIN interprets this as "the range is not summarized" and
so all ranges become lossy for the bitmap scan.

Also, the bug is very low probability.  You need to cause an UPDATE xlog
record, which only occurs when a range gets a new index tuple that
doesn't fit in the existing index page (so the index tuple is wider than
the original -- something that doesn't happen with fixed-width
datatypes).  And you need to have a backup block for the revmap page --
IOW that revmap page update needs to be the first after a checkpoint
(and not be running full_page_writes=off).

If you examine the revmap in a replica after running the script below,
you'd observe it's different than the one in the master.  I confirmed
that the proposed patch fixes the problem.

DROP TABLE IF EXISTS brin_iso;
CREATE TABLE brin_iso (value text) WITH (fillfactor = 10);
CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1);
DO $$
  DECLARE curtid tid;
  BEGIN
LOOP
  INSERT INTO brin_iso VALUES ('a');
  PERFORM brin_summarize_new_values('brinidx');
  SELECT max(pages) INTO curtid FROM 
brin_revmap_data(get_raw_page('brinidx', 1))
   WHERE pages <> '(0,0)';
  EXIT WHEN curtid > tid '(3, 0)';
END LOOP;
  END;
  $$ ;
DELETE FROM brin_iso WHERE ctid < '(0,99)';
VACUUM brin_iso ;
CHECKPOINT;
INSERT INTO brin_iso VALUES (repeat('xyzxxz', 24));


(I'm not sure if it's possible that revmap page 1 can be filled before
the first regular page is full; if that happens, this will loop
forever.)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] Incorrect XLogRegisterBuffer flag for revmapbuf in brin

[Alvaro Herrera]

Kuntal Ghosh wrote:
> Hi all,
> 
> In brin_doupdate(line 290), REGBUF_STANDARD is used to register
> revmap buffer reference in WAL record. But, revmap buffer page doesn't
> have a standard page layout and it doesn't update pd_upper and
> pd_lower as well.

Hmm.  This bug should be causing WAL replay to zero out the revmap page
contents, since essentially the whole page is covered by the "hole" in
standard pages.  I can't see what is causing that not to happen, but
evidently it isn't, since the index works in a replica.  What am I
missing?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Peter Eisentraut]

On 1/9/17 1:34 PM, Robert Haas wrote:
> On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
>  wrote:
>> On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
>>> We will need to make CURRENT_DATABASE a reserved keyword. But I like
>>> this idea more than COMMENT ON CURRENT DATABASE.
>>
>> We already have the reserved key word CURRENT_CATALOG, which is the
>> standard spelling.  But I wouldn't be bothered if we made
>> CURRENT_DATABASE somewhat reserved as well.
> 
> Maybe I'm just lacking in imagination, but what's the argument against
> spelling it CURRENT DATABASE?

To achieve consistent support for specifying the current database, we
would need to change the grammar for every command involving databases.
And it would also set a precedent for similar commands, such as current
user/role.  Plus support in psql, pg_dump, etc. would get more complicated.

Instead, it would be simpler to define a grammar symbol like

database_name: ColId | CURRENT_DATABASE

and make a small analogous change in objectaddress.c and you're done.

Compare rolespec in gram.y.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Bruce Momjian]

On Mon, Jan  9, 2017 at 04:52:46PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Mon, Jan  9, 2017 at 01:34:03PM -0500, Robert Haas wrote:
> > > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> > >  wrote:
> > > > On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> > > >> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> > > >> this idea more than COMMENT ON CURRENT DATABASE.
> > > >
> > > > We already have the reserved key word CURRENT_CATALOG, which is the
> > > > standard spelling.  But I wouldn't be bothered if we made
> > > > CURRENT_DATABASE somewhat reserved as well.
> > > 
> > > Maybe I'm just lacking in imagination, but what's the argument against
> > > spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
> > > anything new at all, and it also looks more SQL-ish to me.  SQL
> > > generally tries to emulate English, and I don't normally
> > > speak_hyphenated_words.
> > 
> > I assume it is to match our use of CURRENT_USER as having special
> > meaning.
> 
> CURRENT_USER is a standards-mandated keyword, but CURRENT_DATABASE is
> not.  The closest thing SQL has is CURRENT_CATALOG, which is the string
> that identifies the "current default catalog".  This would lead us to
> COMMENT ON DATABASE CURRENT_CATALOG
> 
> Do we want that spelling?  It looks ugly to me.

Agreed.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Alvaro Herrera]

Bruce Momjian wrote:
> On Mon, Jan  9, 2017 at 01:34:03PM -0500, Robert Haas wrote:
> > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> >  wrote:
> > > On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> > >> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> > >> this idea more than COMMENT ON CURRENT DATABASE.
> > >
> > > We already have the reserved key word CURRENT_CATALOG, which is the
> > > standard spelling.  But I wouldn't be bothered if we made
> > > CURRENT_DATABASE somewhat reserved as well.
> > 
> > Maybe I'm just lacking in imagination, but what's the argument against
> > spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
> > anything new at all, and it also looks more SQL-ish to me.  SQL
> > generally tries to emulate English, and I don't normally
> > speak_hyphenated_words.
> 
> I assume it is to match our use of CURRENT_USER as having special
> meaning.

CURRENT_USER is a standards-mandated keyword, but CURRENT_DATABASE is
not.  The closest thing SQL has is CURRENT_CATALOG, which is the string
that identifies the "current default catalog".  This would lead us to
COMMENT ON DATABASE CURRENT_CATALOG

Do we want that spelling?  It looks ugly to me.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Peter Eisentraut]

On 1/9/17 2:52 PM, Alvaro Herrera wrote:
> CURRENT_USER is a standards-mandated keyword, but CURRENT_DATABASE is
> not.  The closest thing SQL has is CURRENT_CATALOG, which is the string
> that identifies the "current default catalog".  This would lead us to
> COMMENT ON DATABASE CURRENT_CATALOG
> 
> Do we want that spelling?  It looks ugly to me.

Hence my suggestion earlier to make CURRENT_DATABASE reserved.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Increase pltcl test coverage

[Jim Nasby]

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

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


Got a stack trace. The abort happens in TclObjLookupVar:

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

nsPtr itself is NULL.


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



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


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

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

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

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

regards, tom lane


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

Re: [HACKERS] RustgreSQL

[Jim Nasby]

On 1/9/17 11:51 AM, Robert Haas wrote:

Anyway, with regards to either Rust (which I know very little about)
or C++ (which I know more about) I think it would be more promising to
think about enabling extensions to be written in such languages than
to think about converting the entire source base.  A system like


Yeah, converting the entire codebase is probably doomed to failure from 
the start.



PostgreSQL is almost a language of its own; we don't really code for
PostgreSQL in C, but in "PG-C".  Learning the PG-specific idioms is
arguably more work than learning C itself, and that would still be
true, I think, if we had a "PG-C++" or a "PG-Rust" or a "PG-D"
variant.  Still, if having such variants drew more programmers to work
on extending PostgreSQL, I think that would be worth some work on our
part to enable it.  However, maintaining multiple copies of our
1.2-million-line source base just for easier reference by people more
familiar with one of those languages than with C sounds to me like it
would create more problems than it would solve.


I do wonder if there are parts of the codebase that would be much better 
suited to a language other than C, and could reasonably be ported. 
Especially if that could be done in such a way that the net result is 
still C code so we're not adding dependencies to non developers (similar 
to bison).


Extensions are a step in that direction, but they're ultimately not core 
Postgres (which is a different issue).

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


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

Re: [HACKERS] RustgreSQL

[Craig Ringer]

On 10 January 2017 at 09:54, Joel Jacobson  wrote:

> One idea of an area that would be most useful from a user-perspective
> is probably all pg_catalog functions that are immutable.
> They should be able to be written without expertise of the PostgreSQL 
> internals,
> since they only depend on the input parameters to produce the output.

Wait, what?

No, that doesn't follow at all.

Immutable functions can and do use functionality from all over the
server. They just don't depend on user-visible mutable _state_
elsewhere in the server.

As with all the rest, you'd need something that integrates well enough
with C that you can use C functions ... and macros. In which case
you're basically writing C.

That's why I mentioned upthread that C++ is probably the only
reasonably likely direction to go in, if we ever change, and probably
only a progressive change based on a restricted subset of C++ if we
start needing C++-only features, start having platform support issues,
etc. The common subset of C and C++ is a large portion of the C
language.

> And that also means is should be easier to write them in a different
> language than C,
> because they don't need access to any PostgreSQL internal data structures,
> or make use of existing C functions.

Not the case at all. You'd need a large adapter layer for pretty much
any language to handle interaction with Pg's data types, memory
management, etc.

We don't currently define a formal public API or a "safe" subset of
PostgreSQL's C interfaces. Anyone who wanted to make a serious attempt
at writing "safe" or "fenced" C extensions in another language that
supports restricted execution would need to start there. Whether they
want to use NaCL, .NET Core or Mono and C#, Java with SecurityManager,
or whatever.

That's what the existing PL/Perl does, it just has a _very_ small part
of PostgreSQL's interfaces exposed, so it's very limited in what it
can actually do.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] guc-ify the formerly hard-coded MAX_SEND_SIZE to max_wal_send

[Jon Nelson]

On Sat, Jan 7, 2017 at 7:48 PM, Jim Nasby  wrote:

> On 1/5/17 12:55 PM, Jonathon Nelson wrote:
>
>> Attached please find a patch for PostgreSQL 9.4 which changes the
>> maximum amount of data that the wal sender will send at any point in
>> time from the hard-coded value of 128KiB to a user-controllable value up
>> to 16MiB. It has been primarily tested under 9.4 but there has been some
>> testing with 9.5.
>>
>
> To make sure this doesn't get lost, please add it to
> https://commitfest.postgresql.org. Please verify the patch will apply
> against current HEAD and pass make check-world.


Attached please find a revision of the patch, changed in the following ways:

1. removed a call to debug2.
2. applies cleanly against master (as of 8c5722948e831c1862a39da2bb5d79
3a6f2aabab)
3. one small indentation fix, one small verbiage fix.
4. switched to calculating the upper bound using XLOG_SEG_SIZE rather than
hard-coding 16384.
5. the git author is - obviously - different.

make check-world passes.
I have added it to the commitfest.
I have verified with strace that up to 16MB sends are being used.
I have verified that the GUC properly grumps about values greater than
XLOG_SEG_SIZE / 1024 or smaller than 4.

-- 
Jon
From 7286e290daec32e12260e9e1e44a490c13ed2245 Mon Sep 17 00:00:00 2001
From: Jon Nelson 
Date: Mon, 9 Jan 2017 20:00:41 -0600
Subject: [PATCH] guc-ify the formerly hard-coded MAX_SEND_SIZE to max_wal_send

---
 src/backend/replication/walsender.c | 13 +++--
 src/backend/utils/misc/guc.c| 12 
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index f3082c3..4a9eb16 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -91,7 +91,7 @@
  * because signals are checked only between messages.  128kB (with
  * default 8k blocks) seems like a reasonable guess for now.
  */
-#define MAX_SEND_SIZE (XLOG_BLCKSZ * 16)
+int	max_wal_send_guc = 0;
 
 /* Array of WalSnds in shared memory */
 WalSndCtlData *WalSndCtl = NULL;
@@ -2194,7 +2194,7 @@ retry:
 /*
  * Send out the WAL in its normal physical/stored form.
  *
- * Read up to MAX_SEND_SIZE bytes of WAL that's been flushed to disk,
+ * Read up to max_wal_send kilobytes of WAL that's been flushed to disk,
  * but not yet sent to the client, and buffer it in the libpq output
  * buffer.
  *
@@ -2208,6 +2208,7 @@ XLogSendPhysical(void)
 	XLogRecPtr	startptr;
 	XLogRecPtr	endptr;
 	Size		nbytes;
+	int max_wal_send = max_wal_send_guc * 1024;
 
 	if (streamingDoneSending)
 	{
@@ -2346,8 +2347,8 @@ XLogSendPhysical(void)
 
 	/*
 	 * Figure out how much to send in one message. If there's no more than
-	 * MAX_SEND_SIZE bytes to send, send everything. Otherwise send
-	 * MAX_SEND_SIZE bytes, but round back to logfile or page boundary.
+	 * max_wal_send bytes to send, send everything. Otherwise send
+	 * max_wal_send bytes, but round back to logfile or page boundary.
 	 *
 	 * The rounding is not only for performance reasons. Walreceiver relies on
 	 * the fact that we never split a WAL record across two messages. Since a
@@ -2357,7 +2358,7 @@ XLogSendPhysical(void)
 	 */
 	startptr = sentPtr;
 	endptr = startptr;
-	endptr += MAX_SEND_SIZE;
+	endptr += max_wal_send;
 
 	/* if we went beyond SendRqstPtr, back off */
 	if (SendRqstPtr <= endptr)
@@ -2376,7 +2377,7 @@ XLogSendPhysical(void)
 	}
 
 	nbytes = endptr - startptr;
-	Assert(nbytes <= MAX_SEND_SIZE);
+	Assert(nbytes <= max_wal_send);
 
 	/*
 	 * OK to read and send the slice.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5b23dbf..b9a0c47 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -112,6 +112,7 @@ extern char *default_tablespace;
 extern char *temp_tablespaces;
 extern bool ignore_checksum_failure;
 extern bool synchronize_seqscans;
+extern int  max_wal_send_guc;
 
 #ifdef TRACE_SYNCSCAN
 extern bool trace_syncscan;
@@ -2342,6 +2343,17 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
+		{"max_wal_send", PGC_SIGHUP, REPLICATION_SENDING,
+			gettext_noop("Sets the maximum WAL payload size for WAL replication."),
+			NULL,
+			GUC_UNIT_KB
+		},
+		_wal_send_guc,
+		128, 4, XLOG_SEG_SIZE / 1024,
+		NULL, NULL, NULL
+	},
+
+	{
 		{"commit_delay", PGC_SUSET, WAL_SETTINGS,
 			gettext_noop("Sets the delay in microseconds between transaction commit and "
 		 "flushing WAL to disk."),
-- 
2.10.2


-- 
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] Rename pg_switch_xlog to pg_switch_wal

[Michael Paquier]

On Tue, Jan 10, 2017 at 1:53 AM, Vladimir Rusinov  wrote:
>
> On Mon, Jan 9, 2017 at 4:14 PM, Peter Eisentraut
>  wrote:
>>
>> > pg_is_xlog_replay_paused| pg_is_recovery_paused
>>
>> All the other xlog_replay names have been changed to wal_replay, so
>> making this one different is probably not so good.
>
> Oops, forgot about that one. Attached patch with this addressed as well.

-errhint("pg_xlogfile_name_offset() cannot be executed
during recovery.")));
+errhint(
+"pg_wal_file_name_offset() cannot be executed
during recovery.")));
I am not sure that there is any need to reformat this error hint.

Your patch includes useless diff noise in pg_proc.h. By that I mean
the lines of pg_start_backup, pg_stop_backup, etc.

+/*  Next OID: 6016 */
+
I don't think you need that.

src/test/perl/PostgresNode.pm has added recently a new method called
lsn() that uses some of the WAL position functions. Their update is
necessary as well for this patch.

As there are two school of thoughts on this thread, keeping your patch
with the compatibility table is the best move for now. Even if we end
up by having a version without aliases, that will be just code to
remove in the final version.
-- 
Michael


-- 
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] move collation import to backend

[Euler Taveira]

On 18-12-2016 18:30, Peter Eisentraut wrote:
> Updated patch with that fix.
> 
Peter, I reviewed and improved your patch.

* I document the new function. Since collation is a database object, I
chose "Database Object Management Functions" section.
* I've added a check to any-encoding database because I got 'FATAL:
collation "C" already exists' on Debian 8, although, I didn't get on
CentOS 7. The problem is that Debian has two locales for C (C and
C.UTF-8) and CentOS has just one (C).
* I've added OidIsValid to test the new collation row.
* I've changed the parameter order. Schema seems more important than
if_not_exists. Also, we generally leave those boolean parameters for the
end of list. I don't turn if_not_exists optional but IMO it would be a
good idea (default = true).
* You removed some #if and #ifdef while moving things around. I put it back.
* You didn't pgident some lines of code but I'm sure you didn't for a
small patch footprint.
* I didn't test on Windows.
* As a last comment, you set cost = 100 and it seems reasonable because
it lasts 411 ms to scan/load 923 collations in my slow VM. However,
successive executions takes ~1200 ms.

I'm attaching the complete and also a patch at the top of your last patch.


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..1e52a48 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19190,6 +19190,38 @@ postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
 in the database's default tablespace, the tablespace can be specified as 0.

 
+   
+   Operating system collations are loaded with the
+   pg_import_system_collations function, shown in .
+   
+
+   
+Collation Functions
+
+ 
+  Name Return Type Description
+ 
+
+ 
+  
+   
+pg_import_system_collations
+pg_import_system_collations(schema regnamespace, if_not_exists boolean)
+   
+   void
+   Import operating system collations
+  
+ 
+
+   
+
+   
+   pg_import_system_collations loads collations that it finds on
+   the operating system into system catalog pg_collation,
+   skipping those that are already present.
+   
+
   
 
   
diff --git a/src/backend/catalog/pg_collation.c b/src/backend/catalog/pg_collation.c
index 63c2eb9..694c0f6 100644
--- a/src/backend/catalog/pg_collation.c
+++ b/src/backend/catalog/pg_collation.c
@@ -98,10 +98,21 @@ CollationCreate(const char *collname, Oid collnamespace,
 			  PointerGetDatum(collname),
 			  Int32GetDatum(-1),
 			  ObjectIdGetDatum(collnamespace)))
-		ereport(ERROR,
+	{
+		if (if_not_exists)
+		{
+			ereport(NOTICE,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("collation \"%s\" already exists, skipping",
+		collname)));
+			return InvalidOid;
+		}
+		else
+			ereport(ERROR,
 (errcode(ERRCODE_DUPLICATE_OBJECT),
  errmsg("collation \"%s\" already exists",
 		collname)));
+	}
 
 	/* open pg_collation */
 	rel = heap_open(CollationRelationId, RowExclusiveLock);
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index e108b50..cf3acea 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -139,7 +139,7 @@ DefineCollation(ParseState *pstate, List *names, List *parameters)
 			 collctype,
 			 false);
 
-	if (!newoid)
+	if (!OidIsValid(newoid))
 		return InvalidObjectAddress;
 
 	ObjectAddressSet(address, CollationRelationId, newoid);
@@ -183,6 +183,7 @@ IsThereCollationInNamespace(const char *collname, Oid nspOid)
 }
 
 
+#ifdef HAVE_LOCALE_T
 /*
  * "Normalize" a locale name, stripping off encoding tags such as
  * ".utf8" (e.g., "en_US.utf8" -> "en_US", but "br_FR.iso885915@euro"
@@ -216,13 +217,15 @@ normalize_locale_name(char *new, const char *old)
 
 	return changed;
 }
+#endif	/* HAVE_LOCALE_T */
 
 
 Datum
 pg_import_system_collations(PG_FUNCTION_ARGS)
 {
-	bool		if_not_exists = PG_GETARG_BOOL(0);
-	Oid nspid = PG_GETARG_OID(1);
+#if defined(HAVE_LOCALE_T) && !defined(WIN32)
+	Oid nspid = PG_GETARG_OID(0);
+	bool		if_not_exists = PG_GETARG_BOOL(1);
 
 	FILE	   *locale_a_handle;
 	char		localebuf[NAMEDATALEN]; /* we assume ASCII so this is fine */
@@ -321,6 +324,7 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
 	if (count == 0)
 		ereport(ERROR,
 (errmsg("no usable system locales were found")));
+#endif	/* not HAVE_LOCALE_T && not WIN32 */
 
 	PG_RETURN_VOID();
 }
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index b0126a9..bb8637e 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5345,7 +5345,7 @@ DESCR("pg_controldata recovery state information as a function");
 DATA(insert OID = 3444 ( pg_control_init PGNSP PGUID 12 1 0 0 0 f f f f t f v s 0 0 2249 "" 

Re: [HACKERS] _hash_addovflpage has a bug

[Amit Kapila]

On Mon, Jan 9, 2017 at 11:59 PM, Robert Haas  wrote:
> On Fri, Jan 6, 2017 at 11:32 PM, Amit Kapila  wrote:
>> On Sat, Jan 7, 2017 at 2:33 AM, Robert Haas  wrote:
>>> It looks to to me like the recent hash index changes have left
>>> _hash_addovflpage slightly broken.  I think that if that function
>>> reaches the point where it calls _hash_getbuf() to fetch the next page
>>> in the bucket chain, we also need to clear retain_pin.  Otherwise,
>>> we'll erroneously think that we're supposed to retain a pin even when
>>> the current page is an overflow page rather than the primary bucket
>>> page, which is wrong.
>>>
>>
>> How?  I think we are ensuring that we retain pin only if it is a
>> bucket page.  The check is as below:
>> if ((pageopaque->hasho_flag & LH_BUCKET_PAGE) && retain_pin)
>
> Oh, right.  So I guess there's no bug, but I still think that's pretty
> ugly.  How about:
>
> if (retain_pin)
> LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> else
> _hash_relbuf(rel, buf);
> retain_pin = false;
>
> instead?
>

Yeah, we can write code that way, but then it is better to rely just
on retain_pin variable in the function and add an Assert for bucket
page whenever we are retaining pin.  How about doing something like
attached patch?

>>> A larger question, I suppose, is why this function wants to be certain
>>> of adding a new page even if someone else has already done so.  It
>>> seems like it might be smarter for it to just return the newly added
>>> page to the caller and let the caller try to use it.  _hash_doinsert
>>> has an Assert() that the tuple fits on the returned page, but that
>>> could be deleted.  As it stands, if two backends try to insert a tuple
>>> into the same full page at the same time, both of them will add an
>>> overflow page and we'll end up with 2 overflow pages each containing 1
>>> tuple.
>>
>> I think it is because in the current algorithm we first get an
>> overflow page and then add it to the end.  Now we can change it such
>> that first, we acquire the lock on the tail page, check if we still
>> need an overflow page, if so, then proceed, else return the already
>> added overflow page.
>
> For the WAL patch, this is all going to need to be atomic anyway,
> right?  Like, you have to allocate the overflow page and add it to the
> bucket chain as a single logged operation?

Yes.

>  Not sure exactly how that
> works out in terms of locking.
>

We have to change the locking order as mentioned above by me.  This
change is already present in that patch, so maybe we add the check as
suggested by you along with that patch.  Now, another thing we could
do is to extract those changes from WAL patch, but I am not sure if it
is worth the effort.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


improve_code_hash_add_ovfl_page_v1.patch
Description: Binary data

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

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

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

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

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

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

regards, tom lane


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

Re: [HACKERS] Support for pg_receivexlog --format=plain|tar

[Michael Paquier]

On Sat, Jan 7, 2017 at 8:19 PM, Magnus Hagander  wrote:
> On Sat, Jan 7, 2017 at 12:31 AM, Michael Paquier 
> wrote:
>> There is something I forgot. With this patch,
>> FindStreamingStart()@pg_receivexlog.c is actually broken. In short it
>> forgets to consider files that have been compressed at the last run of
>> pg_receivexlog and will try to stream changes from the beginning. I
>> can see that gzip -l provides this information... But I have yet to
>> find something in zlib that allows a cheap lookup as startup of
>> streaming should be fast. Looking at how gzip -l does it may be faster
>> than looking at the docs.
>
> Do we really care though? As in, does startup of streaming have to be *that*
> fast? Even gunziping 16Mb (worst case) doesn't exactly take a long time. If
> your pg_receivexlog is restarting so often that it becomes a problem, I
> think you already have another and much bigger problem on your hands.

Based on some analysis, it is enough to look at the last 4 bytes of
the compressed file to get the size output data with a single call to
lseek() and then read(). So as there is a simple way to do things and
that's far cheaper than decompressing perhaps hundred of segments I'd
rather do it this way. Attached is the implementation. This code is
using 2 booleans for 4 states of the file names: full non-compressed,
partial non-compressed, full compressed and partial compressed. This
keeps the last check of FindStreamingStart() more simple, but that's
quite fancy lately to have an enum for such things :D

> I found another problem with it -- it is completely broken in sync mode. You
> need to either forbid sync mode together with compression, or teach
> dir_sync() about it. The later would sound better, but I wonder how much
> that's going to kill compression due to the small blocks? Is it a reasonable
> use-case?

Hm. Looking at the docs I think that --compress defined with
--synchronous maps to the use of Z_SYNC_FLUSH with gzflush(). FWIW I
don't have a direct use case for it, but it is not a major effort to
add it, so done. There is no actual reason to forbid this combinations
of options either.
-- 
Michael
diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index bfa055b58b..8c1ea9a2e2 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -180,6 +180,19 @@ PostgreSQL documentation

   
  
+
+ 
+  -Z level
+  --compress=level
+  
+   
+Enables gzip compression of transaction logs, and specifies the
+compression level (0 through 9, 0 being no compression and 9 being best
+compression).  The suffix .gz will
+automatically be added to all filenames.
+   
+  
+ 
 
 

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8ebf24e771..b9c0bb5fff 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -481,7 +481,7 @@ LogStreamerMain(logstreamer_param *param)
 	stream.partial_suffix = NULL;
 
 	if (format == 'p')
-		stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
+		stream.walmethod = CreateWalDirectoryMethod(param->xlog, 0, do_sync);
 	else
 		stream.walmethod = CreateWalTarMethod(param->xlog, compresslevel, do_sync);
 
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index b6f57a878c..74fa5c68c0 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -34,6 +34,7 @@
 /* Global options */
 static char *basedir = NULL;
 static int	verbose = 0;
+static int	compresslevel = 0;
 static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static volatile bool time_to_abort = false;
@@ -57,6 +58,15 @@ static bool stop_streaming(XLogRecPtr segendpos, uint32 timeline,
 	exit(code);	\
 	}
 
+/* Routines to evaluate segment file format */
+#define IsCompressXLogFileName(fname)\
+	(strlen(fname) == XLOG_FNAME_LEN + strlen(".gz") &&	\
+	 strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&		\
+	 strcmp((fname) + XLOG_FNAME_LEN, ".gz") == 0)
+#define IsPartialCompressXLogFileName(fname)\
+	(strlen(fname) == XLOG_FNAME_LEN + strlen(".gz.partial") &&	\
+	 strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&		\
+	 strcmp((fname) + XLOG_FNAME_LEN, ".gz.partial") == 0)
 
 static void
 usage(void)
@@ -75,6 +85,7 @@ usage(void)
 	printf(_("  --synchronous  flush transaction log immediately after writing\n"));
 	printf(_("  -v, --verbose  output verbose messages\n"));
 	printf(_("  -V, --version  output version information, then exit\n"));
+	printf(_("  -Z, --compress=0-9 compress logs with given compression level\n"));
 	printf(_("  -?, --help show this help, then exit\n"));
 	printf(_("\nConnection options:\n"));
 	printf(_("  

Re: [HACKERS] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

Hello, Amit, Magnus,

I'm sorry for my late reply.  Yesterday was a national holiday in Japan.


From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Kapila
> PGSharedMemoryReAttach is called after the startup of new process whereas
> pgwin32_ReserveSharedMemoryRegion is called before the new process could
> actually start.  Basically, pgwin32_ReserveSharedMemoryRegion is used to
> reserve shared memory for each backend, so calling VirtualAlloc there should
> follow spec for huge pages.  If you have some reason for not using, then
> it is not clear from your reply, can you try to explain in detail.

OK.  The processing takes place in the following order:

1. postmaster calls CreateProcess() to start a child postgres in a suspended 
state.
2. postmaster calls VirtualAlloc(MEM_RESERVE) in 
pgwin32_ReserveSharedMemoryRegion() to reserve the virtual address space in the 
child to secure space for the shared memory.  This call just affects the 
virtual address space and does not allocate physical memory.  So the large page 
is still irrelevant.
3. postmaster resumes execution of the child postgres.
4. The child postgres calls VirtualFree(MEM_RESERVE)  in 
PGSharedMemoryReAttach() to release the reserved virtual address space.  Here, 
the effect of VirtualAlloc() is invalidated anyway.
5. The child process calls MapViewOfFile() in PGSharedMemoryReAttach() to map 
the shared memory at the same address.  Hereafter, the large page option 
specified in CreateFileMapping() call is relevant.


> + if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, )) {
> + CloseHandle(hToken); ereport(elevel, (errmsg("could not enable Lock
> + pages in memory user right: error
> code %lu", GetLastError()),
> + errdetail("Failed system call was LookupPrivilegeValue."))); return
> + FALSE; }
> 
> The order of closing handle and ereport is different here than other places
> in the same function.  If there is no specific reason for doing so, then
> keep the order consistent.

You are right, I modified the patch to call CloseHandle() after ereport() so 
that ereport() CloseHandle() wouldn't change the error code for ereport().  
That's the order used in postmaster.c.


> I have tried to test v4 version of the patch and it is always failing in
> below error after call to AdjustTokenPrivileges:
> 
> + if (GetLastError() != ERROR_SUCCESS)
> + {
> + if (GetLastError() == ERROR_NOT_ALL_ASSIGNED) ereport(elevel,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("could not enable Lock pages in memory user right"),
> + errhint("Assign Lock pages in memory user right to the Windows user
> account which runs PostgreSQL.")));
> 
> I have ensured that user used to run PostgreSQL has "Lock pages in memory"
> privilege/rights.  I have followed msdn tips [1] to do that (restarted the
> m/c after assigning privilege).  I am using Win7. Can you share the steps
> you have followed to test and your windows m/c details?
> 
> [1] -
> https://msdn.microsoft.com/en-us/library/windows/desktop/ff961911(v=vs
> .85).aspx

I succeeded by following the same procedure using secpol.msc on Win10, running 
64-bit PostgreSQL.  I started PostgreSQL as a Windows service because it's the 
normal way, with the service account being a regular Windows user account(i.e. 
not LocalSystem).

But... I failed to start PostgreSQL by running "pg_ctl start" from a command 
prompt, receiving the same error message as you.  On the other hand, I could 
start PostgreSQL on a command prompt with administrative privileges 
(right-click on "Command prompt" from the Start menu and select "Run as an 
administrator" in the menu.  It seems that Windows removes many privileges, 
including "Lock Pages in Memory", when starting the normal command prompt.  As 
its evidence, you can use the attached priv.c to see what privileges are 
assigned and and enabled/disabled.  Build it like "cl priv.c" and just run 
priv.exe on each command prompt.  Those runs show different privileges.

Should I need to do something, e.g. explain in the document that the user 
should use the command prompt with administrative privileges when he uses 
huge_pages?

Regards
Takayuki Tsunakawa



win_large_pages_v6.patch
Description: win_large_pages_v6.patch
#include 
#include 

int main()
{
HANDLE hToken;
PTOKEN_PRIVILEGES pTokenPrivileges;
char szPrivilegeName[256];
char szDisplayName[256];
DWORD dwLength;
DWORD dwLanguageId;
DWORD i;

if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, )) {
return 1;
}

GetTokenInformation(hToken, TokenPrivileges, NULL, 0, );

pTokenPrivileges = (PTOKEN_PRIVILEGES)LocalAlloc(LPTR, dwLength);
if (pTokenPrivileges == NULL) {
CloseHandle(hToken);
return 1;
}

GetTokenInformation(hToken, TokenPrivileges, pTokenPrivileges, dwLength, 
);


for (i = 0; i < pTokenPrivileges->PrivilegeCount; i++) {

dwLength = 

Re: [HACKERS] merging some features from plpgsql2 project

[Pavel Stehule]

2017-01-10 5:59 GMT+01:00 Peter Eisentraut :

> On 1/7/17 6:39 AM, Pavel Stehule wrote:
> > ROW_COUNT .. shortcut for GET DIAGNOSTICS row_count = ROW_COUNT.
>
> I don't see the point.
>

A check how much rows was impacted by query is relative often task. So we
can do this task more user friendly.

Second motivation - ROW_COUNT is working for static and for dynamic SQL -
it can be partial replace of FOUND variable.

But now, when I am thinking about it - it can be strange for some users
too. Pretty often we use implicit LIMIT for query execution. So ROW_COUNT
can be probably different than users expecting.

Regards

Pavel


>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] RustgreSQL

[Joel Jacobson]

On Mon, Jan 9, 2017 at 3:22 PM, Jim Nasby  wrote:
> I do wonder if there are parts of the codebase that would be much better
> suited to a language other than C, and could reasonably be ported.
> Especially if that could be done in such a way that the net result is still
> C code so we're not adding dependencies to non developers (similar to
> bison).
>
> Extensions are a step in that direction, but they're ultimately not core
> Postgres (which is a different issue).

I think this is a great idea!

That way the amount of C code could be reduced over time,
while safely extending the official version with new functionality on
the surface,
without increasing the amount of C code.

One idea of an area that would be most useful from a user-perspective
is probably all pg_catalog functions that are immutable.
They should be able to be written without expertise of the PostgreSQL internals,
since they only depend on the input parameters to produce the output.

And that also means is should be easier to write them in a different
language than C,
because they don't need access to any PostgreSQL internal data structures,
or make use of existing C functions.


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

Re: [HACKERS] WIP: Covering + unique indexes.

[Erik Rijkers]

On 2017-01-09 16:02, Anastasia Lubennikova wrote:

 include_columns_10.0_v1.patch


The patch applies, compiles, and make check is OK.

It yields nice perfomance gains and I haven't been able to break 
anything (yet).


Some edits of the sgml-changes are attached.

Thank you for this very useful improvement.

Erik Rijkers





--- doc/src/sgml/catalogs.sgml.orig	2017-01-10 03:40:52.649761572 +0100
+++ doc/src/sgml/catalogs.sgml	2017-01-10 03:53:13.408298695 +0100
@@ -3598,7 +3598,7 @@
   int2
   
   The number of key columns in the index. "Key columns" are ordinary
-  index columns in contrast with "included" columns.
+  index columns (as opposed to "included" columns).
  
 
  
--- doc/src/sgml/ref/create_index.sgml.orig	2017-01-10 03:14:25.603940872 +0100
+++ doc/src/sgml/ref/create_index.sgml	2017-01-10 03:22:20.013526245 +0100
@@ -153,16 +153,15 @@
 the table's heap. Having these columns in the INCLUDE clause
 in some cases allows PostgreSQL to skip the heap read
 completely. This also allows UNIQUE indexes to be defined on
-one set of columns, which can include another set of column in the
-INCLUDE clause, on which the uniqueness is not enforced upon.
+one set of columns, which can include another set of columns in the
+INCLUDE clause, on which the uniqueness is not enforced.
 It's the same with other constraints (PRIMARY KEY and EXCLUDE). This can
 also can be used for non-unique indexes as any columns which are not required
-for the searching or ordering of records can be included in the
-INCLUDE clause, which can slightly reduce the size of the index,
-due to storing included attributes only in leaf index pages.
+for the searching or ordering of records can be used in the
+INCLUDE clause, which can slightly reduce the size of the index.
 Currently, only the B-tree access method supports this feature.
 Expressions as included columns are not supported since they cannot be used
-in index-only scan.
+in index-only scans.

   
  
--- doc/src/sgml/ref/create_table.sgml.orig	2017-01-10 03:15:17.033377433 +0100
+++ doc/src/sgml/ref/create_table.sgml	2017-01-10 03:34:57.541537576 +0100
@@ -615,9 +615,9 @@
  
   Adding a unique constraint will automatically create a unique btree
   index on the column or group of columns used in the constraint.
-  Optional clause INCLUDE allows to add into the index
-  a portion of columns on which the uniqueness is not enforced upon.
-  Note, that althogh constraint is not enforced upon included columns, it still
+  The optional clause INCLUDE adds to that index
+  one or more columns on which the uniqueness is not enforced.
+  Note that although the constraint is not enforced on the included columns, it still
   depends on them. Consequently, some operations on these columns (e.g. DROP COLUMN)
   can cause cascade constraint and index deletion.
   See paragraph about INCLUDE in
@@ -659,7 +659,7 @@
   index on the column or group of columns used in the constraint.
   An optional INCLUDE clause allows a list of columns
   to be specified which will be included in the non-key portion of the index.
-  Althogh uniqueness is not enforced on the included columns, the constraint
+  Although uniqueness is not enforced on the included columns, the constraint
   still depends on them. Consequently, some operations on the included columns
   (e.g. DROP COLUMN) can cause cascade constraint and index deletion.
   See paragraph about INCLUDE in

-- 
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_background contrib module proposal

[Jim Nasby]

On 1/9/17 7:22 AM, amul sul wrote:

On Sun, Jan 8, 2017 at 8:50 AM, Jim Nasby  wrote:



[skipped...]


Oh, hmm. So I guess if you do that when the background process is idle it's
the same as a close?

I think we need some way to safeguard against accidental forkbombs for cases
where users aren't intending to leave something running in the background.
There's other reasons to use this besides spawning long running processes,
and I'd certainly want to be able to ensure the calling function wasn't
accidentally leaving things running that it didn't mean to. (Maybe the patch
already does this...)



Current pg_background patch built to the top of BackgroundSession code
take care of that;
user need to call pg_background_close() to gracefully close previously
forked background
worker.  Even though if user session who forked this worker exited
without calling
pg_background_close(), this background worked force to exit with following log:

ERROR:  could not read from message queue: Other process has detached queue
LOG:  could not send on message queue: Other process has detached queue
LOG:  worker process: background session by PID 61242 (PID 61358)
exited with exit code 1

Does this make sense to you?


I'm specifically thinking of autonomous transactions, where you do NOT 
want to run the command in the background, but you do want to run it in 
another connection.


The other example is running a command in another database. Not the same 
as the autonomous transaction use case, but once again you likely don't 
want that command running in the background.


Put another way, when you launch a new process in a shell script, you 
have to do something specific for that process to run in the background.


My concern here is that AIUI the only way to launch a bgworker is with 
it already backgrounded. That makes it much more likely that a bug in 
your code produces an unintended fork-bomb (connection-bomb?). That 
would be especially true if the function was re-entrant.


Since one of the major points of bgworkers is exactly to run stuff in 
the background, while the parent connection is doing something else, I 
can certainly understand the default case being to background something, 
but I think it'd be good to have a simple way to start a bgworker, have 
it do something, and wait until it's done.


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


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

Re: [HACKERS] proposal: session server side variables

[Craig Ringer]

On 10 January 2017 at 05:14, Robert Haas  wrote:

> 2. The user doesn't need to re-declare the variables you want to use
> at the beginning of every session.  This is also the reason why many
> people want global temporary tables.  They don't do anything that
> can't be done with local temporary tables; they're just more
> convenient to use.

They'll also help a lot with pg_attribute and pg_class bloat.

I don't feel strongly either way about catalog use, but definitely
think declare-before-use is crucial. Pretty much every language that
implicitly declares variables has landed up adding a way to require
them to be declared for a reason.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] pg_restore accepts -j -1

[Stephen Frost]

Greetings,

For reasons which seem likely to be entirely unintentional, pg_restore
will accept a '-1' for -j:

pg_restore -j -1

This seems to result in the parallel state being NULL and so things
don't outright break, but it hardly seems likely to be what the user was
asking for- my guess is that they actually wanted "parallel, single
transaction", which we don't actually support:

-> pg_restore -j 2 -1
pg_restore: cannot specify both --single-transaction and multiple jobs

We also don't accept -1 for pg_dump:

-> pg_dump -j -1
pg_dump: invalid number of parallel jobs

If I'm missing something, please let me know, otherwise I'll plan to put
the same check into pg_restore which exists in pg_dump.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] merging some features from plpgsql2 project

[Pavel Stehule]

2017-01-10 2:02 GMT+01:00 Jim Nasby :

> On 1/9/17 6:07 PM, Marko Tiikkaja wrote:
>
>> One use case is NEW and OLD in triggers. Checking to see if one or
>> the other is set is easier than checking TG_OP. It's also going to
>> be faster (probably MUCH faster; IIRC the comparison currently
>> happens via SPI).
>>
>>
>> This sounds useless.
>>
>
> I guess you've not written much non-trivial trigger code then... the
> amount of code duplication you end up with is quite ridiculous. It's also a
> good example of why treating this as an exception and trapping isn't a good
> solution either: you can already do that with triggers today.
>
> Being able to check the existence of a variable is a very common idiom in
> other languages, so I'm don't see why plpgsql shouldn't have it.


In strongly typed language like PLpgSQL is DEFINE little bit strange. On
second hand there are some elements of dynamic languages - record types and
polymorphics parameters.

Some languages has reflection API - some other like Oberon some special
statements - variable guards that allows safe casting and safe usage - it
is not far what we do with Node API.





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

Re: [HACKERS] merging some features from plpgsql2 project

[Peter Eisentraut]

On 1/7/17 6:39 AM, Pavel Stehule wrote:
> ROW_COUNT .. shortcut for GET DIAGNOSTICS row_count = ROW_COUNT. 

I don't see the point.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

[HACKERS] remove floats from bootstrap scanner/parser

[Alvaro Herrera]

I happened to notice that we have dead code to parse floating point
numbers in the boostrap scanner/parser.  This seems unused since forever
-- or, more precisely, I couldn't find any floats in 8.2's postgres.bki
(the oldest I have around).  I would just rip it out, per the attached.

-- 
Álvaro Herrera
diff --git a/src/backend/bootstrap/bootparse.y 
b/src/backend/bootstrap/bootparse.y
index d28af23..33d9f0b 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -105,11 +105,11 @@ static int num_columns_read = 0;
 
 %type   boot_index_params
 %type  boot_index_param
-%typeboot_const boot_ident
+%typeboot_ident
 %type   optbootstrap optsharedrelation optwithoutoids 
boot_column_nullness
 %type  oidspec optoideq optrowtypeoid
 
-%token  CONST_P ID
+%token  ID
 %token OPEN XCLOSE XCREATE INSERT_TUPLE
 %token XDECLARE INDEX ON USING XBUILD INDICES UNIQUE XTOAST
 %token COMMA EQUALS LPAREN RPAREN
@@ -464,16 +464,10 @@ boot_column_val_list:
 boot_column_val:
  boot_ident
{ InsertOneValue($1, num_columns_read++); }
-   | boot_const
-   { InsertOneValue($1, num_columns_read++); }
| NULLVAL
{ InsertOneNull(num_columns_read++); }
;
 
-boot_const :
- CONST_P { $$ = yylval.str; }
-   ;
-
 boot_ident :
  ID{ $$ = yylval.str; }
;
diff --git a/src/backend/bootstrap/bootscanner.l 
b/src/backend/bootstrap/bootscanner.l
index 9cfd19c..6467882 100644
--- a/src/backend/bootstrap/bootscanner.l
+++ b/src/backend/bootstrap/bootscanner.l
@@ -66,7 +66,6 @@ static intyyline = 1; /* line number 
for error reporting */
 
 D  [0-9]
 oct\\{D}{D}{D}
-Exp[Ee][-+]?{D}+
 id ([A-Za-z0-9_]|{oct}|\-)+
 sid\"([^\"])*\"
 arrayid [A-Za-z0-9_]+\[{D}*\]
@@ -127,13 +126,6 @@ insert { return(INSERT_TUPLE); }
return(ID);
}
 
-(-)?{D}+"."{D}*({Exp})? |
-(-)?{D}*"."{D}+({Exp})? |
-(-)?{D}+{Exp}  {
-   yylval.str = 
pstrdup(yytext);
-   return(CONST_P);
-   }
-
 .  {
elog(ERROR, "syntax error at line %d: 
unexpected character \"%s\"", yyline, yytext);
}

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

Re: [HACKERS] Parallel bitmap heap scan

[tushar]

On 01/09/2017 07:22 PM, Dilip Kumar wrote:

Thanks, Tushar. I have fixed it. The defect was in 0002. I have also
observed another issue related to code refactoring, Actually, there
was some code present in 0001 which supposed to be in 0003.

Thanks, I have checked at my end and it is fixed now.

--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company



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

Re: [HACKERS] pg_hba_file_settings view patch

[Michael Paquier]

On Thu, Jan 5, 2017 at 1:58 PM, Michael Paquier
 wrote:
> Could you hold on a bit to commit that? I'd like to look at it in more
> details. At quick glance, there is for example no need to use
> CreateTemplateTupleDesc and list the columns both in pg_proc.h and the
> C routine itself. And memset() can be used in fill_hba_line for the
> error code path.

And here we go.

+
+postgres=# select * from pg_hba_rules;
[... large example ...]
+
+
It would be nice to reduce the width of this example. That's not going
to be friendly with the generated html.

+   switch (hba->conntype)
+   {
+   case ctLocal:
+   values[index] = CStringGetTextDatum("local");
+   break;
+   case ctHost:
+   values[index] = CStringGetTextDatum("host");
+   break;
+   case ctHostSSL:
+   values[index] = CStringGetTextDatum("hostssl");
+   break;
+   case ctHostNoSSL:
+   values[index] = CStringGetTextDatum("hostnossl");
+   break;
+   default:
+   elog(ERROR, "unexpected connection type in parsed HBA entry");
+   break;
+   }
Here let's remove the break clause and let compilers catch problem
when they show up.

+   if (hba->pamservice)
+   {
+   initStringInfo();
+   appendStringInfoString(, "pamservice=");
+   appendStringInfoString(, hba->pamservice);
+   options[noptions++] = CStringGetTextDatum(str.data);
+   }
There is a bunch of code duplication here. I think that it would make
more sense to encapsulate that into a routine, at least let's use
appendStringInfo and let's group the two calls together.

+/* LDAP supports 10 currently, keep this well above the most any
method needs */
+#define MAX_OPTIONS 12
Er, why? There is an assert already, that should be enough.

=# \d pg_hba_rules
   View "pg_catalog.pg_hba_rules"
  Column  |  Type   | Collation | Nullable | Default
--+-+---+--+-
 line_number  | integer |   |  |
 type | text|   |  |
 keyword_database | text[]  |   |  |
 database | text[]  |   |  |
 keyword_user | text[]  |   |  |
 user_name| text[]  |   |  |
 keyword_address  | text|   |  |
 address  | inet|   |  |
 netmask  | inet|   |  |
 hostname | text|   |  |
 method   | text|   |  |
 options  | text[]  |   |  |
 error| text|   |  |
keyword_database and database map actually to the same thing if you
refer to a raw pg_hba.conf file because they have the same meaning for
user. You could simplify the view and just remove keyword_database,
keyword_user and keyword_address. This would simplify your patch as
well with all hte mumbo-jumbo to see if a string is a dedicated
keyword or not. In its most simple shape pg_hba_rules should show to
the user as an exact map of the entries of the raw file.

I have copied the example file of pg_hba.conf, reloaded it:
https://www.postgresql.org/docs/devel/static/auth-pg-hba-conf.html
And then the output result gets corrupted by showing up free()'d results:
null   | null| \x7F\x7F\x7F\x7F\x7F

+   if (err_msg)
+   {
+   /* type */
+   index++;
+   nulls[index] = true;
[... long sequence ...]
Please let's use MemSet here and remove this large chunk of code...

+   if (!superuser())
+   ereport(ERROR,
+   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+(errmsg("must be superuser to view pg_hba.conf settings";
Access to the function is already revoked, so what's the point of this
superuser check?

+   tupdesc = CreateTemplateTupleDesc(NUM_PG_HBA_LOOKUP_ATTS, false);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "line_number",
+  INT4OID, -1, 0);
There is no need to list all the columns here. You can just use
get_call_result_type() and be done with it as the types and columns
names are already listed in the pg_proc entry of the new function.
-- 
Michael


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

[Joel Jacobson]

On Mon, Jan 9, 2017 at 6:03 PM, Craig Ringer  wrote:
> Immutable functions can and do use functionality from all over the
> server. They just don't depend on user-visible mutable _state_
> elsewhere in the server.

OK, fair point, but at least the functionality *could* be written without using
existing C functions, since its only the input that determine what
output will be returned. The dependencies used by the immutable
functions can also be ported, function by function, until there are
no dependencies.


-- 
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] Incorrect XLogRegisterBuffer flag for revmapbuf in brin

[Kuntal Ghosh]

On Tue, Jan 10, 2017 at 2:52 AM, Alvaro Herrera
 wrote:
>
> Alvaro Herrera wrote:
>
> > If you examine the revmap in a replica after running the script below,
> > you'd observe it's different than the one in the master.  I confirmed
> > that the proposed patch fixes the problem.
>
> Pushed.  Thanks for the report!
>
Thanks. It's good to see the wal-consistency checking tool is
able to track some real bugs.

> (I'm not sure if it's possible that revmap page 1 can be filled before
> the first regular page is full; if that happens, this will loop
> forever.)
It is unlikely that revmap page will be filled before first regular
page for the above
test case.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
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] DROP FUNCTION of multiple functions

[Michael Paquier]

On Sun, Jan 1, 2017 at 1:17 AM, Peter Eisentraut
 wrote:
> On 12/1/16 9:32 PM, Peter Eisentraut wrote:
>> I think it would be better to get rid of objargs and have objname be a
>> general Node that can contain more specific node types so that there is
>> some amount of type tracking.  FuncWithArgs would be one such type,
>> Typename would be another, Value would be used for simple strings, and
>> we could create some other ones, or stick with lcons for some simple
>> cases.  But then we don't have to make stuff into one-item lists to just
>> to satisfy the currently required List.
>>
>> That's the general idea.  But that's a rather big change that I would
>> rather break down into smaller pieces.
>
> People wanted to the whole thing at once, so here it is.
>
> Patches 0001 and 0002 are some prep work.  Patch 0003 is the main patch
> that removes the objargs fields and changes the objname to a generic
> Node*.  0004 is an API simplification that could be committed together
> with 0003 but I kept it separate for easier review.  0005 accomplishes
> the DROP FUNCTION changes.  0006 is some other cleanup that fell out of
> this.

I don't see any problems with 0001. In 0002, the comment of
class_args/CreateOpClassItem in parsenodes.h needs to be updated,
because this becomes used by operators as well.

Regarding 0003, which is large, but actually quite simple... The
approach makes sense. All the checks on the object formats are moved
out of the static routines in objectaddress.c and moved into a single
place.

+   if (list_length(name) != 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("name list length must be exactly %d", 1)));
+   objnode = linitial(name);
Those are the sanity checks for each object are moved to
pg_get_object_address(). This has as result some loss of verbosity in
the error messages. As the object type is passed by the user when
calling the SQL-callable interface, this new way looks fine to me.

One comment though: there are still many list_make2() or even
list_make3 calls for some object types. Would it make sense to replace
those lists with a decided number of items by a Node and simplify the
interface? Using a Node instead of a list would save some cycles. OK
that's really few though as that's basically comparing a list lookup
with a direct pointer lookup.

+   | DROP drop_type1 any_name_list opt_drop_behavior
+   {
+   DropStmt *n = makeNode(DropStmt);
+   n->removeType = $2;
+   n->missing_ok = FALSE;
+   n->objects = $3;
+   n->behavior = $4;
+   n->concurrent = false;
+   $$ = (Node *)n;
+   }
+   | DROP drop_type2 IF_P EXISTS name_list opt_drop_behavior
+   {
drop_type1 and drop_type2 are not really explicit, especially after
reading that one allows schema-qualified names, not the other. Could
you use something like drop_type_qualified? The same applies to
comment_type and security_label.

0004 could be merged with 0002, no? That looks good to me.

In 0005, a nit:
+DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int),
functest_IS_3(int);
 -- Cleanups
The DROP query could be moved below the cleanup comment.

It may be a good idea to add an example of query dropping two
functions at once in the docs.

Could you add some regression tests for the drop of aggregates and
operators to stress the parsing code path?

While looking at 0006... DROP POLICY and DROP RULE could be unified. I
just noticed that while reading the code.

This patch series look in rather good shape to me at the end.
-- 
Michael


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

Re: [HACKERS] Block level parallel vacuum WIP

[Masahiko Sawada]

On Tue, Jan 10, 2017 at 3:46 PM, Amit Kapila  wrote:
> On Mon, Jan 9, 2017 at 2:18 PM, Masahiko Sawada  wrote:
>> On Sat, Jan 7, 2017 at 2:47 PM, Amit Kapila  wrote:
>>> On Fri, Jan 6, 2017 at 11:08 PM, Masahiko Sawada  
>>> wrote:
 On Mon, Oct 3, 2016 at 11:00 AM, Michael Paquier
  wrote:
> On Fri, Sep 16, 2016 at 6:56 PM, Masahiko Sawada  
> wrote:
>> Yeah, I don't have a good solution for this problem so far.
>> We might need to improve group locking mechanism for the updating
>> operation or came up with another approach to resolve this problem.
>> For example, one possible idea is that the launcher process allocates
>> vm and fsm enough in advance in order to avoid extending fork relation
>> by parallel workers, but it's not resolve fundamental problem.
>

 I got some advices at PGConf.ASIA 2016 and started to work on this again.

 The most big problem so far is the group locking. As I mentioned
 before, parallel vacuum worker could try to extend the same visibility
 map page at the same time. So we need to make group locking conflict
 in some cases, or need to eliminate the necessity of acquiring
 extension lock. Attached 000 patch uses former idea, which makes the
 group locking conflict between parallel workers when parallel worker
 tries to acquire extension lock on same page.

>>>
>>> How are planning to ensure the same in deadlock detector?  Currently,
>>> deadlock detector considers members from same lock group as
>>> non-blocking.  If you think we don't need to make any changes in
>>> deadlock detector, then explain why so?
>>>
>>
>> Thank you for comment.
>> I had not considered necessity of dead lock detection support. But
>> because lazy_scan_heap actquires the relation extension lock and
>> release it before acquiring another extension lock, I guess we don't
>> need that changes for parallel lazy vacuum. Thought?
>>
>
> Okay, but it is quite possible that lazy_scan_heap is not able to
> acquire the required lock as that is already acquired by another
> process (which is not part of group performing Vacuum), then all the
> processes in a group might need to run deadlock detector code wherein
> multiple places, it has been assumed that group members won't
> conflict.  As an example, refer code in TopoSort where it is trying to
> emit all groupmates together and IIRC, the basis of that part of the
> code is groupmates won't conflict with each other and this patch will
> break that assumption.  I have not looked into the parallel vacuum
> patch, but changes in 000_make_group_locking_conflict_extend_lock_v2
> doesn't appear to be safe. Even if your parallel vacuum patch doesn't
> need any change in deadlock detector, then also as proposed it appears
> that changes in locking will behave same for any of the operations
> performing relation extension.  So in future any parallel operation
> (say parallel copy/insert) which involves relation extension lock will
> behave similary.  Is that okay or are you assuming that the next
> person developing any such feature should rethink about this problem
> and extends your solution to match his requirement.

Thank you for expatiation. I agree that we should support dead lock
detection as well in this patch even if this feature doesn't need that
actually. I'm going to extend 000 patch to support dead lock
detection.

>
>
>> What do we actually gain from having the other parts of VACUUM execute
>> in parallel? Does truncation happen faster in parallel?
>>
>
> I think all CPU intensive operations for heap (like checking of
> dead/live rows, processing of dead tuples, etc.) can be faster.

Vacuum on table with no index can be faster as well.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] Declarative partitioning - another take

[Keith Fiske]

Is there any reason for the exclusion of parent tables from the pg_tables
system catalog view? They do not show up in information_schema.tables as
well. I believe I found where to make the changes and I tested to make sure
it works for my simple case. Attached is my first attempt at patching
anything in core. Not sure if there's anywhere else this would need to be
fixed.

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com
diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 4df390a..c31d0d8 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -1901,6 +1901,7 @@ CREATE VIEW tables AS
   WHEN c.relkind = 'r' THEN 'BASE TABLE'
   WHEN c.relkind = 'v' THEN 'VIEW'
   WHEN c.relkind = 'f' THEN 'FOREIGN TABLE'
+  WHEN c.relkind = 'P' THEN 'PARTITIONED TABLE'
   ELSE null END
  AS character_data) AS table_type,
 
@@ -1912,7 +1913,7 @@ CREATE VIEW tables AS
CAST(t.typname AS sql_identifier) AS user_defined_type_name,
 
CAST(CASE WHEN c.relkind = 'r' OR
-  (c.relkind IN ('v', 'f') AND
+  (c.relkind IN ('v', 'f', 'P') AND
-- 1 << CMD_INSERT
pg_relation_is_updatable(c.oid, false) & 8 = 8)
 THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_insertable_into,
@@ -1923,7 +1924,7 @@ CREATE VIEW tables AS
 FROM pg_namespace nc JOIN pg_class c ON (nc.oid = c.relnamespace)
LEFT JOIN (pg_type t JOIN pg_namespace nt ON (t.typnamespace = nt.oid)) ON (c.reloftype = t.oid)
 
-WHERE c.relkind IN ('r', 'v', 'f')
+WHERE c.relkind IN ('r', 'v', 'f', 'P')
   AND (NOT pg_is_other_temp_schema(nc.oid))
   AND (pg_has_role(c.relowner, 'USAGE')
OR has_table_privilege(c.oid, 'SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER')
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 31aade1..f4dc460 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -136,7 +136,7 @@ CREATE VIEW pg_tables AS
 C.relrowsecurity AS rowsecurity
 FROM pg_class C LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
  LEFT JOIN pg_tablespace T ON (T.oid = C.reltablespace)
-WHERE C.relkind = 'r';
+WHERE C.relkind = 'r' OR C.relkind = 'P';
 
 CREATE VIEW pg_matviews AS
 SELECT


-- 
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: session server side variables

[Fabien COELHO]

Hello Robert,

Half-persistence (in definition, not in value) is not a key feature 
needed by the use-case.


Well, you don't get to decide that.


I do not think that your reprimand is deserved about this point: I did not 
decide a subjective opinion, I noted an objective fact.


You've been told by at least three or four people that they don't want 
variables to be transactional, you've been pointed to documentation 
links showing that in other database systems including Oracle variables 
are not transactional, and you still insist that this proposal is 
senseless unless variables are transactional.


Indeed.

I have submitted a proof of this fact in the form of a counter example 
which (1) (pseudo) implements the use-case by logging into an audit table 
the fact a user accesses the secure level (2) shows that the status of a 
non transactional session variable used for keeping this status is wrong 
for the use case in some cases (it says that all is well while appending 
to the audit table failed).


I have also recognized that the use-case could be implemented safely, 
although not correctly, if pg provides nested/autonomous transactions like 
Oracle, DB2 or MS SQL does, but I think that having such a useful feature 
is quite far away...


You have every right to decide what you think is useful, but you don't 
have a right to decide what other people think is useful.


Hmmm.

I feel entitled to point out to other people that their belief that a 
feature as described provides a correct solution to a particular use case 
is wrong, if it is factually the case. If they persist in this belief 
despite the submitted proof, I can only be sad about it, because if pg 
provides a feature for a security-relared use case which does not work 
correctly it is just shooting one's foot.


I do not like Pavel's feature, this is a subjective opinion. This feature 
does not provide a correct solution for the use case, this is an objective 
fact. The presented feature does not have a real use case, this is too 
bad.


Finally, I did not "veto" this feature, I reviewed it in depth and 
concluded negatively. You are a committer and I'm just a "silly academic", 
you do not have to listen to anything I say and can take majority votes 
against proofs if you want.


--
Fabien.


--
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_dump / copy bugs with "big lines" ?

[Alvaro Herrera]

Daniel Verite wrote:

> My tests are OK too but I see an issue with the code in
> enlargeStringInfo(), regarding integer overflow.
> The bit of comment that says:
> 
>   Note we are assuming here that limit <= INT_MAX/2, else the above
>   loop could overflow.
> 
> is obsolete, it's now INT_MAX instead of INT_MAX/2.

I would keep this comment but use UINT_MAX/2 instead.

> There's a related problem here:
>   newlen = 2 * str->maxlen;
>   while (needed > newlen)
>   newlen = 2 * newlen;
> str->maxlen is an int going up to INT_MAX so [2 * str->maxlen] now
> *will* overflow when [str->maxlen > INT_MAX/2].
> Eventually it somehow works because of this:
>   if (newlen > limit)
>   newlen = limit;
> but newlen is wonky (when resulting from int overflow)
> before being brought back to limit.

Yeah, you're right.  We also need to cast "needed" to Size in the while
test; and the repalloc_huge() call no longer needs a cast.

I propose the attached.

Not sure if we also need to cast the assignment to str->maxlen in the
last line.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 1327c2eea9c7ca074d88bb167bd4c35338d2de0b
Author: Alvaro Herrera 
AuthorDate: Tue Jan 10 02:46:42 2017 -0300
CommitDate: Tue Jan 10 02:46:42 2017 -0300

Fix overflow check in StringInfo

A thinko I introduced in fa2fa9955280.  Also, amend a similarly broken
comment.

Report and patch by Daniel Vérité.
Discussion: 
https://postgr.es/m/1706e85e-60d2-494e-8a64-9af1e1b21...@manitou-mail.org

diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index bdc204e..11d751a 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -313,19 +313,19 @@ enlargeStringInfo(StringInfo str, int needed)
 * for efficiency, double the buffer size each time it overflows.
 * Actually, we might need to more than double it if 'needed' is big...
 */
-   newlen = 2 * str->maxlen;
-   while (needed > newlen)
+   newlen = 2 * (Size) str->maxlen;
+   while ((Size) needed > newlen)
newlen = 2 * newlen;
 
/*
 * Clamp to the limit in case we went past it.  Note we are assuming 
here
-* that limit <= INT_MAX/2, else the above loop could overflow.  We will
+* that limit <= UINT_MAX/2, else the above loop could overflow.  We 
will
 * still have newlen >= needed.
 */
if (newlen > limit)
newlen = limit;
 
-   str->data = (char *) repalloc_huge(str->data, (Size) newlen);
+   str->data = (char *) repalloc_huge(str->data, newlen);
 
str->maxlen = newlen;
 }

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

Re: [HACKERS] Block level parallel vacuum WIP

[Masahiko Sawada]

On Sat, Jan 7, 2017 at 7:18 AM, Claudio Freire  wrote:
> On Fri, Jan 6, 2017 at 2:38 PM, Masahiko Sawada  wrote:
>>  table_size | indexes | parallel_degree |   time
>> +-+-+--
>>  6.5GB  |   0 |   1 | 00:00:14
>>  6.5GB  |   0 |   2 | 00:00:02
>>  6.5GB  |   0 |   4 | 00:00:02
>
> Those numbers look highly suspect.
>
> Are you sure you're not experiencing caching effects? (ie: maybe you
> ran the second and third vacuums after the first, and didn't flush the
> page cache, so the table was cached)
>
>>  6.5GB  |   2 |   1 | 00:02:18
>>  6.5GB  |   2 |   2 | 00:00:38
>>  6.5GB  |   2 |   4 | 00:00:46
> ...
>>  13GB   |   0 |   1 | 00:03:52
>>  13GB   |   0 |   2 | 00:00:49
>>  13GB   |   0 |   4 | 00:00:50
> ..
>>  13GB   |   2 |   1 | 00:12:42
>>  13GB   |   2 |   2 | 00:01:17
>>  13GB   |   2 |   4 | 00:02:12
>
> These would also be consistent with caching effects

Since I ran vacuum after updated all pages on table, I thought that
all data are in either shared buffer or OS cache. But anyway, I
measured it at only one time so this result is not accurate. I'll test
again and measure it at some times.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] parallelize queries containing subplans

[Dilip Kumar]

On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila  wrote:
> Attached patch implements the above idea.  This will enable
> parallelism for queries containing un-correlated subplans, an example
> of which is as follows:

I have reviewed the patch (pq_pushdown_subplan_v1.patch), Mostly patch
looks clean to me.
I have also done some basic functional testing and it's working fine.

I have one comment.

+ else if (IsA(node, AlternativeSubPlan))
+ {
+ AlternativeSubPlan *asplan = (AlternativeSubPlan *) node;
+ ListCell   *lc;
+
+ Assert(context->root);
+
+ foreach(lc, asplan->subplans)
+ {
+ SubPlan*splan = (SubPlan *) lfirst(lc);
+ Plan   *plan;
+
+ Assert(IsA(splan, SubPlan));
+
+ plan = planner_subplan_get_plan(context->root, splan);
+ if (!plan->parallel_safe)
+ return true;
+ }
  }

I see no reason why we need to process the subplan list of
AlternativeSubPlan here, Anyway expression_tree_walker will take care
of processing each subplan of AlternativeSubPlan. Now in the case
where all the subplans in AlternativeSubPlan are parallel_safe we will
process this list twice. But, more than that it will be cleaner to not
handle AlternativeSubPlan here unless there is some strong reason?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Block level parallel vacuum WIP

[Amit Kapila]

On Mon, Jan 9, 2017 at 2:18 PM, Masahiko Sawada  wrote:
> On Sat, Jan 7, 2017 at 2:47 PM, Amit Kapila  wrote:
>> On Fri, Jan 6, 2017 at 11:08 PM, Masahiko Sawada  
>> wrote:
>>> On Mon, Oct 3, 2016 at 11:00 AM, Michael Paquier
>>>  wrote:
 On Fri, Sep 16, 2016 at 6:56 PM, Masahiko Sawada  
 wrote:
> Yeah, I don't have a good solution for this problem so far.
> We might need to improve group locking mechanism for the updating
> operation or came up with another approach to resolve this problem.
> For example, one possible idea is that the launcher process allocates
> vm and fsm enough in advance in order to avoid extending fork relation
> by parallel workers, but it's not resolve fundamental problem.

>>>
>>> I got some advices at PGConf.ASIA 2016 and started to work on this again.
>>>
>>> The most big problem so far is the group locking. As I mentioned
>>> before, parallel vacuum worker could try to extend the same visibility
>>> map page at the same time. So we need to make group locking conflict
>>> in some cases, or need to eliminate the necessity of acquiring
>>> extension lock. Attached 000 patch uses former idea, which makes the
>>> group locking conflict between parallel workers when parallel worker
>>> tries to acquire extension lock on same page.
>>>
>>
>> How are planning to ensure the same in deadlock detector?  Currently,
>> deadlock detector considers members from same lock group as
>> non-blocking.  If you think we don't need to make any changes in
>> deadlock detector, then explain why so?
>>
>
> Thank you for comment.
> I had not considered necessity of dead lock detection support. But
> because lazy_scan_heap actquires the relation extension lock and
> release it before acquiring another extension lock, I guess we don't
> need that changes for parallel lazy vacuum. Thought?
>

Okay, but it is quite possible that lazy_scan_heap is not able to
acquire the required lock as that is already acquired by another
process (which is not part of group performing Vacuum), then all the
processes in a group might need to run deadlock detector code wherein
multiple places, it has been assumed that group members won't
conflict.  As an example, refer code in TopoSort where it is trying to
emit all groupmates together and IIRC, the basis of that part of the
code is groupmates won't conflict with each other and this patch will
break that assumption.  I have not looked into the parallel vacuum
patch, but changes in 000_make_group_locking_conflict_extend_lock_v2
doesn't appear to be safe. Even if your parallel vacuum patch doesn't
need any change in deadlock detector, then also as proposed it appears
that changes in locking will behave same for any of the operations
performing relation extension.  So in future any parallel operation
(say parallel copy/insert) which involves relation extension lock will
behave similary.  Is that okay or are you assuming that the next
person developing any such feature should rethink about this problem
and extends your solution to match his requirement.


> What do we actually gain from having the other parts of VACUUM execute
> in parallel? Does truncation happen faster in parallel?
>

I think all CPU intensive operations for heap (like checking of
dead/live rows, processing of dead tuples, etc.) can be faster.


> Can you give us some timings for performance of the different phases,
> with varying levels of parallelism?

I feel timings depend on the kind of test we perform, for example if
there are many dead rows in heap and there are few indexes on a table,
we might see that the gain for doing parallel heap scan is
substantial.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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] postgres_fdw bug in 9.6

[Etsuro Fujita]

On 2017/01/09 22:36, Ashutosh Bapat wrote:

I wrote:

Done.  Attached is the new version of the patch.



Why is this so?
 * If the outer cheapest-total path is parameterized by the inner
 * rel, we can't generate a nestloop path.


That parameterization means that for each row scanned from the inner 
rel, the nestloop has to pass down the param values of the row into the 
scan of the outer rel; but it's not possible for the nestloop to work 
like that.  You can find the same tests in match_unsorted_outer.



 * If either cheapest-total path is parameterized by the other
 * rel, we can't generate a hashjoin/mergejoin path.


The above applies to hashjoins and mergejoins.  In addition, those joins 
cannot pass down the param values of each outer-rel row into the 
inner-rel scan, like nestloop joins, so the inner path mustn't be 
parameterized by the outer rel in those joins.  See the same tests in 
sort_inner_and_outer and hash_inner_and_outer.


While re-reading the patch, I noticed this could be simplified:

!   case JOIN_FULL:
!   /*
!* If either cheapest-total path is parameterized by 
the other
!* rel, we can't generate a hashjoin/mergejoin path.  
(There's no
!* use looking for alternative input paths, since these 
should
!* already be the least-parameterized available paths.)
!*/
!   if (PATH_PARAM_BY_REL(outer_path, innerrel) ||
!   PATH_PARAM_BY_REL(inner_path, outerrel))
!   return NULL;
!   /* If proposed path is still parameterized, we can't 
use it. */
!   required_outer = 
calc_non_nestloop_required_outer(outer_path,
!   
  inner_path);

This would mean that both the outer and inner paths must be 
unparameterized.  Will simplify if it's ok.



I don't think we need to provide details of what kind of path the function
builds.
+ join plan.  CreateLocalJoinPath builds a nested loop join
+ path for the specified join relation, except when the join type is
+ FULL, in which case a merge or hash join path is built.


Ok, will remove.


I am not able to understand the code or the comment below
+
+/* Save first mergejoin information for possible use by the FDW */
+if (outerkeys == all_pathkeys)
+{
+extra->mergeclauses = cur_mergeclauses;
+extra->merge_pathkeys = merge_pathkeys;
+extra->merge_outerkeys = outerkeys;
+extra->merge_innerkeys = innerkeys;
+}


In the case when mergejoin is allowed and we have merge clauses but no 
hash clauses, CreateLocalJoinPath creates a mergejoin from the 
cheapest-total paths for the outer and inner rels, by explicitly sorting 
both the outer and inner rels.  The above is for creating the mergejoin 
in CreateLocalJoinPath; since sort_inner_and_outer also creates 
mergejoins from the cheapest-total paths, the above code saves data 
about one of mergejoins created there so that CreateLocalJoinPath uses 
that data to create the mergejoin.


On second thought, I noticed that this could be simplified a bit as 
well; since the output sort order (merge_pathkeys) of the mergejoin 
implementing a FULL join is NIL (see build_join_pathkeys), we wouldn't 
need to save that info in merge_pathkeys.  Will simplify if it's ok.



If the inner and/or outer paths are not ordered as required, we will need to
order them. Code below doesn't seem to handle that case.
/*
 * If the paths are already well enough ordered, we can
 * skip doing an explicit sort.
 */
if (outerkeys &&
pathkeys_contained_in(outerkeys, outer_path->pathkeys))
outerkeys = NIL;
if (innerkeys &&
pathkeys_contained_in(innerkeys, inner_path->pathkeys))
innerkeys = NIL;


I might be missing something, but if the outer/inner paths are already 
well sorted, we wouldn't need an explicit sort, so we can set the 
outerkeys/innerkeys to NIL safely.  (You can find the same optimization 
in try_mergejoin_path.)


Thanks for the review!

Best regards,
Etsuro Fujita




--
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] Rename pg_switch_xlog to pg_switch_wal

[Vladimir Rusinov]

On Fri, Jan 6, 2017 at 12:44 AM, Michael Paquier 
wrote:

> > - OIDs - where do I get numbers from? I was kinda choosing them at
> random,
> > unaware if there is some process for keeping track of them. Please point
> me
> > if such thing exists and I'll change them.
>
> You can use src/include/catalog/unused_oids to look at the OIDs not
> yet assigned. Assigning them logically increasing may be a good idea.
> But OID ordering is not that mandatory for the internal functions and
> operators.
>

Thanks, that's useful tip.
Looks like I am fine, although I could have reused some lower numbers.


> > - New function names. I've used 'recovery' instead of 'xlog_replay'
>
> I would not have touched this one. But if we go this way, let's
> bike-shed and use pg_recovery_ as prefix for consistency.
>

Makes sense, renamed those to wal_replay.


> > - Release notes. I was unable to find a draft for 10.0. How do I make
> sure
>
> these renames are not forgotten?
>
> That's part of the job of the release note writer, very likely Tom or
> Bruce. The commit message should be explicit enough that the writer
> does not have to dig into the code itself for the changes.
>

I see. Attached patch is just a output of 'git diff master' and does not
contain message since I've messed up squashing, but here's the commit
message I intended to use (feel free to reuse fully or partially, beware
that I have probably messed up grammar, so somebody who actually knows
English should review):

Remove 'xlog' references from admin functions.

After 'pg_xlog' has been renamed to 'pg_wal' 'xlog' reference in
function names is confusing.
This change renames 'xlog' in function names to 'wal', keeping old
function names as aliases.

Following functions have been renamed:

Name|   Replaced by
|---
pg_current_xlog_flush_location  | pg_current_wal_flush_location
pg_current_xlog_insert_location | pg_current_wal_insert_location
pg_current_xlog_location| pg_current_wal_location
pg_is_xlog_replay_paused| pg_is_recovery_paused
pg_last_xlog_receive_location   | pg_last_wal_receive_location
pg_last_xlog_replay_location| pg_last_wal_replay_location
pg_switch_xlog  | pg_switch_wal
pg_xlog_location_diff   | pg_wal_location_diff
pg_xlog_replay_pause| pg_wal_replay_pause
pg_xlog_replay_resume   | pg_wal_replay_resume
pg_xlogfile_name| pg_wal_file_name
pg_xlogfile_name_offset | pg_wal_file_name_offset


-- 
Vladimir Rusinov
Storage SRE, Google Ireland

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047
diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
index 56c6618d3d..dc907baf87 100644
--- a/contrib/bloom/t/001_wal.pl
+++ b/contrib/bloom/t/001_wal.pl
@@ -16,7 +16,7 @@ sub test_index_replay
 	# Wait for standby to catch up
 	my $applname = $node_standby->name;
 	my $caughtup_query =
-"SELECT pg_current_xlog_location() <= write_location FROM pg_stat_replication WHERE application_name = '$applname';";
+"SELECT pg_current_wal_location() <= write_location FROM pg_stat_replication WHERE application_name = '$applname';";
 	$node_master->poll_query_until('postgres', $caughtup_query)
 	  or die "Timed out while waiting for standby 1 to catch up";
 
diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out
index c104c4802d..275c84a450 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -58,7 +58,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
 SELECT slot_name, plugin, slot_type, active,
 NOT catalog_xmin IS NULL AS catalog_xmin_set,
 xmin IS NULl  AS data_xmin_not_set,
-pg_xlog_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
+pg_wal_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
 FROM pg_replication_slots;
 slot_name|plugin | slot_type | active | catalog_xmin_set | data_xmin_not_set | some_wal 
 -+---+---++--+---+--
diff --git a/contrib/test_decoding/sql/ddl.sql b/contrib/test_decoding/sql/ddl.sql
index 89b8b8f780..49dad39b50 100644
--- a/contrib/test_decoding/sql/ddl.sql
+++ b/contrib/test_decoding/sql/ddl.sql
@@ -29,7 +29,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
 SELECT slot_name, plugin, slot_type, active,
 NOT catalog_xmin IS NULL AS catalog_xmin_set,
 xmin IS NULl  AS data_xmin_not_set,
-pg_xlog_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
+pg_wal_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
 FROM pg_replication_slots;
 
 /*
diff --git a/doc/src/sgml/backup.sgml 

Re: [HACKERS] Replication/backup defaults

[Peter Eisentraut]

On 1/9/17 7:44 AM, Magnus Hagander wrote:
> So based on that, I suggest we go ahead and make the change to make both
> the values 10 by default. And that we do that now, because that lets us
> get it out through more testing on different platforms, so that we catch
> issues earlier on if they do arise.

Sounds good.

> It would be interesting to find out why it's limited as well, of course,
> but I don't think we need to wait for that. 

After some testing and searching for documentation, it seems that at
least the BSD platforms have a very low default semmns setting
(apparently 60, which leads to max_connections=30).

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Replication/backup defaults

[Magnus Hagander]

On Sat, Jan 7, 2017 at 7:57 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 1/7/17 6:23 AM, Magnus Hagander wrote:
> > In the build farm, I have found 6 critters that do not end up with
> the
> > 100/128MB setting: sidewinder, curculio, coypu, brolga, lorikeet,
> > opossum.  I wonder what limitations initdb is bumping against.
> >
> >
> > Since you lookeda t the data -- they did not end up with 100, but what's
> > the lowest they did end up with? Did they go all the way down to 10?
>
> They all ended up on 30 or 40.
>
> The only documented way this could happen is if the semaphore
> configuration does not allow enough room, but this would have to be a
> very particular setting on all these quite different boxes.
>
>
So based on that, I suggest we go ahead and make the change to make both
the values 10 by default. And that we do that now, because that lets us get
it out through more testing on different platforms, so that we catch issues
earlier on if they do arise.

It would be interesting to find out why it's limited as well, of course,
but I don't think we need to wait for that.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] UNDO and in-place update

[Amit Kapila]

On Fri, Jan 6, 2017 at 7:41 PM, Robert Haas  wrote:
> On Fri, Jan 6, 2017 at 6:28 AM, Amit Kapila  wrote:
>>> Also, I'm thinking the bit could be stored in the line pointer rather
>>> than the tuple, because with this design we don't need
>>> LP_UNUSED/LP_NORMAL/LP_REDIRECT/LP_DEAD any more.  We could use one
>>> bit to indicate dead or not-dead and the second bit to indicate
>>> recently-modified or not-recently-modified.  With that approach,
>>> clearing the bits only requires iterating over the line pointer array,
>>> not the tuples themselves.
>>
>> I think this can help in mitigating the overhead.  However, now
>> another question is if we just unset it when there is no other active
>> transaction operating on the current page except for current
>> transaction, then will that tuple be considered all-visible?  I think
>> no transaction operating on a page can't be taken as a guarantee for
>> tuple to be marked as all-visible. If that is true, then what is
>> advantage of clearing the bit?
>
> That's kind of a strange question.  The mission of this proposed bit
> is to tell you whether the transaction(s) referenced in the page's
> UNDO pointer have modified that tuple.  If you didn't clear it when
> you reset the UNDO pointer to a new transaction, then the bit would
> always be set for every tuple on the page and it would be completely
> useless.  (I mean, the tuples had to be inserted originally, right?
> So the bit was set then.  And if you never clear it, it will just stay
> set.)
>
> As to your other questions: If the page's UNDO pointer isn't valid,
> then the whole page is all-visible.  If the page's UNDO pointer is
> valid, then any tuple for which the bit is not set is all-visible.
>

Okay, I see the point.  I think here UNDO pointer can be marked
invalid either during page-pruning or vacuum.

Another point which I think is not discussed till now is, how the
locking will work.  As of now, we first lock the tuple and then xid on
which we have to wait. In the new design, we don't know the xid which
has updated the tuple and I am not sure there is any easy way to find
that information.  One idea could be that we have some fixed number of
slots (i think we can make it variable as well, but for simplicity,
lets consider it as fixed) in the page header which will store the
offset to the transaction id inside a TPD entry of the page.  Consider
a TPD entry of page contains four transactions, so we will just store
enough information in heap page header to reach the transaction id for
these four transactions. I think each such page header slot could be
three or four bits long depending upon how many concurrent
transactions we want to support on a page after which a new
transaction has to wait (I think in most workloads supporting
simultaneous eight transactions on a page should be sufficient).
Then we can have an additional byte (or less than byte) in the tuple
header to store lock info which is nothing but an offset to the slot
in the page header.   We might find some other locking technique as
well, but I think keeping it same as current has benefit.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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_background contrib module proposal

[amul sul]

On Sun, Jan 8, 2017 at 8:50 AM, Jim Nasby  wrote:
>
[skipped...]
>
> Oh, hmm. So I guess if you do that when the background process is idle it's
> the same as a close?
>
> I think we need some way to safeguard against accidental forkbombs for cases
> where users aren't intending to leave something running in the background.
> There's other reasons to use this besides spawning long running processes,
> and I'd certainly want to be able to ensure the calling function wasn't
> accidentally leaving things running that it didn't mean to. (Maybe the patch
> already does this...)
>

Current pg_background patch built to the top of BackgroundSession code
take care of that;
user need to call pg_background_close() to gracefully close previously
forked background
worker.  Even though if user session who forked this worker exited
without calling
pg_background_close(), this background worked force to exit with following log:

ERROR:  could not read from message queue: Other process has detached queue
LOG:  could not send on message queue: Other process has detached queue
LOG:  worker process: background session by PID 61242 (PID 61358)
exited with exit code 1

Does this make sense to you?


Regards,
Amul


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

Re: [HACKERS] Parallel bitmap heap scan

[Dilip Kumar]

On Mon, Jan 9, 2017 at 5:01 PM, tushar  wrote:
> Thanks Dilip. issue is reproducible  if  we  uses '--enable-cassert' switch
> in the configure. We are able to reproduce it only with --enable-cassert' .

Thanks, Tushar. I have fixed it. The defect was in 0002. I have also
observed another issue related to code refactoring, Actually, there
was some code present in 0001 which supposed to be in 0003.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0001-opt-parallelcost-refactoring-v8.patch
Description: Binary data


0002-hash-support-alloc-free-v8.patch
Description: Binary data


0003-parallel-bitmap-heap-scan-v8.patch
Description: Binary data

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

Re: [HACKERS] merging some features from plpgsql2 project

[Pavel Stehule]

Hi


>
>  Real support for using variables as identifiers / nothing restricted to
> only accepting a Const.


>
This point is problematic not only from performance perspective.

if you don't use some special syntax and you allow variables as identifier,
then you will got a ambiguous situation quickly - although variables can
have special symbol prefix

SELECT * FROM tab WHERE $var1 = $var3

What is $var1, what is $var2? identifier or value?

Regards

Pavel

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Vladimir Rusinov]

On Mon, Jan 9, 2017 at 4:14 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> > pg_is_xlog_replay_paused| pg_is_recovery_paused
>
> All the other xlog_replay names have been changed to wal_replay, so
> making this one different is probably not so good.
>

Oops, forgot about that one. Attached patch with this addressed as well.

Remove 'xlog' references from admin functions.

After 'pg_xlog' has been renamed to 'pg_wal' 'xlog' reference in
function names is confusing.
This change renames 'xlog' in function names to 'wal', keeping old
function names as aliases.

Following functions have been renamed:

Name|   Replaced by
|---
pg_current_xlog_flush_location  | pg_current_wal_flush_location
pg_current_xlog_insert_location | pg_current_wal_insert_location
pg_current_xlog_location| pg_current_wal_location
pg_is_xlog_replay_paused| pg_is_wal_replay_paused
pg_last_xlog_receive_location   | pg_last_wal_receive_location
pg_last_xlog_replay_location| pg_last_wal_replay_location
pg_switch_xlog  | pg_switch_wal
pg_xlog_location_diff   | pg_wal_location_diff
pg_xlog_replay_pause| pg_wal_replay_pause
pg_xlog_replay_resume   | pg_wal_replay_resume
pg_xlogfile_name| pg_wal_file_name
pg_xlogfile_name_offset | pg_wal_file_name_offset

-- 
Vladimir Rusinov
Storage SRE, Google Ireland

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047
diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
index 56c6618d3d..dc907baf87 100644
--- a/contrib/bloom/t/001_wal.pl
+++ b/contrib/bloom/t/001_wal.pl
@@ -16,7 +16,7 @@ sub test_index_replay
 	# Wait for standby to catch up
 	my $applname = $node_standby->name;
 	my $caughtup_query =
-"SELECT pg_current_xlog_location() <= write_location FROM pg_stat_replication WHERE application_name = '$applname';";
+"SELECT pg_current_wal_location() <= write_location FROM pg_stat_replication WHERE application_name = '$applname';";
 	$node_master->poll_query_until('postgres', $caughtup_query)
 	  or die "Timed out while waiting for standby 1 to catch up";
 
diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out
index c104c4802d..275c84a450 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -58,7 +58,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
 SELECT slot_name, plugin, slot_type, active,
 NOT catalog_xmin IS NULL AS catalog_xmin_set,
 xmin IS NULl  AS data_xmin_not_set,
-pg_xlog_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
+pg_wal_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
 FROM pg_replication_slots;
 slot_name|plugin | slot_type | active | catalog_xmin_set | data_xmin_not_set | some_wal 
 -+---+---++--+---+--
diff --git a/contrib/test_decoding/sql/ddl.sql b/contrib/test_decoding/sql/ddl.sql
index 89b8b8f780..49dad39b50 100644
--- a/contrib/test_decoding/sql/ddl.sql
+++ b/contrib/test_decoding/sql/ddl.sql
@@ -29,7 +29,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
 SELECT slot_name, plugin, slot_type, active,
 NOT catalog_xmin IS NULL AS catalog_xmin_set,
 xmin IS NULl  AS data_xmin_not_set,
-pg_xlog_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
+pg_wal_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
 FROM pg_replication_slots;
 
 /*
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 6eaed1efbe..e37f101277 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -726,9 +726,9 @@ test ! -f /mnt/server/archivedir/000100A90065  cp pg_wal/0
 

 Also, you can force a segment switch manually with
-pg_switch_xlog if you want to ensure that a
-just-finished transaction is archived as soon as possible.  Other utility
-functions related to WAL management are listed in pg_switch_wal if you want to ensure that a just-finished
+transaction is archived as soon as possible.  Other utility functions
+related to WAL management are listed in .

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e31868ba..8021afb369 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17925,13 +17925,13 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_create_restore_point


-pg_current_xlog_flush_location
+pg_current_wal_flush_location


-pg_current_xlog_insert_location
+pg_current_wal_insert_location


-pg_current_xlog_location
+pg_current_wal_location


  

Re: [HACKERS] Parallel bitmap heap scan

[Dilip Kumar]

On Mon, Jan 9, 2017 at 3:07 PM, tushar  wrote:
> creating directory data ... ok
> creating subdirectories ... ok
> selecting default max_connections ... 100
> selecting default shared_buffers ... 128MB
> selecting dynamic shared memory implementation ... posix
> creating configuration files ... ok
> running bootstrap script ... ok
> performing post-bootstrap initialization ... sh: line 1: 30709 Segmentation
> fault "/home/centos/PG10_9ja/postgresql/edbpsql/bin/postgres" --single -F -O
> -j -c search_path=pg_catalog -c exit_on_error=true template1 > /dev/null
> child process exited with exit code 139
> initdb: removing data directory "data"

I have taken the latest code, applied all 3 patches and compiled.
Initdb is working fine for me.

Can you please verify, do you have any extra patch along with my patch?
Did you properly clean the code?

I have asked one of my colleague to verify this and the result is
same, No crash.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

[Pavel Stehule]

2017-01-09 1:21 GMT+01:00 Jim Nasby :

> On 1/8/17 5:56 PM, Greg Stark wrote:
>
>> On 8 January 2017 at 21:50, Jim Nasby  wrote:
>>
>>> Somewhat related to that... it would be useful if Postgres had "fenced"
>>> functions; functions that ran in a separate process and only talked to a
>>> backend via a well defined API (such as libpq). There's two major
>>> advantages
>>> that would give us:
>>>
>>
>> The problem with this is that any of the "interesting" extensions need
>> to use the server API. That is, they need to be able to do things like
>> throw errors, expand toast data, etc.
>>
>
> There's plenty of interesting things you can do in python or R, even
> without that ability.
>
> IMHO just about anything you could do in an external process would be
>> something you could much more easily and conveniently do in the
>> client. And it would be more flexible and scalable as well as it's a
>> lot easier to add more clients than it is to scale up the database.
>>
>
> Well, then you're suffering from serious network latency, and you're
> forced into worrying about endian issues and what-not. Those problems don't
> exist when you're running on the same server. There's also things that
> might make sense on a local-only protocol but would make no sense with an
> external one. My guess is that you'd ultimately want a protocol that's
> something "in between" SPI and libpq.


The running unsafe PL in own managed processes is good idea - Years, I have
a one diploma theme "better management of unsafe PL in Postgres" - but
still without any interest from students :(.  I had two possibilities to
see catastrophic errors related to wrong usage of PLPerlu. If we can locks
interpret/environment in some safe sandbox, then it should be great.

Regards

Pavel


>
>
> That said, there were several pl language implementations that worked
>> this way. IIRC one of the Java pl languages ran in a separate Java
>> process.
>>
>> I think the solution to the problem you're describing is the project
>> formerly known as NaCl
>> https://en.wikipedia.org/wiki/Google_Native_Client
>>
>
> Possibly; depends on if it would allow running things like R or python.
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] merging some features from plpgsql2 project

[Pavel Stehule]

2017-01-09 1:10 GMT+01:00 Jim Nasby :

> On 1/8/17 12:03 AM, Pavel Stehule wrote:
>
>> BTW, I do wish you could change the label of the scope that
>> arguments went into, so that you could use that label to refer
>> to function parameters. If we allowed that it'd perhaps be the
>> best of both worlds: you'd be guaranteed access to all auto
>> variables and parameters, and that access wouldn't need to be
>> tied to the function name (which can be both painful and error
>> prone).
>>
>>
>> We can talk about compiler directive.
>>
>> PRAGMA auto_variables_label() -- require function scope only
>>
>>
>> If we know a list of all auto variables, then it can be on function or
>> block level - it can create aliases.
>>
>
> Oh, the problem is that if you have an argument with the same name as an
> auto variable you're in trouble.
>

I didn't well explained my idea

It is similar to your plpgsql scope. You are introducing the convention. I
proposed a explicit specification. The result is similar.


>
> Probably the easiest thing is to have a scope that sits above the scope
> containing the arguments, and then allow the user to rename both scopes if
> so desired. So in effect you'd end up with
>
> <> -- new scope
> DECLARE
> FOUND;
> etc
> BEGIN
>   <>
>   DECLARE
>   argument_1;
>   argument_2;
>   BEGIN
> -- User supplied block goes here, with optional label
>   END;
> END;
>

It is similar to

PRAGMA auto_variables_namespace(plpgsql);
BEGIN
  ...
END;

Using PRAGMA is more verbose - it is useful for code audit, review - it is
speaking  "I will overwrite some auto variables here, and I need special
namespace"

plpgsql_check, maybe plpgsql self can raise warning if these variables are
shadowed and some option/pragma is not used. Maybe current extra check does
it already.


> Alternatively, we could do...
>
> <>
> DECLARE
> FOUND;
> etc
> BEGIN
>   DECLARE-- User's DECLARE
>   argument_1;
>   argomuent_2;
>   -- User supplied declare code
>   BEGIN -- User's BEGIN
>   
> END
>
> That removes one level of nesting. It's probably better to go with the
> first option though, since it's simpler.
>

You are forgot on function paramaters  - somebody can use a function
argument  like FOUND, .. So auto variables should to be declared in most
top namespace.

Usually it is invisible for users - one, two more namespaces has zero cost
for compilation and absolute zero impact for evaluation.


>
> In both cases, I'd really like the ability to rename those blocks. #pragma
> would be fine for that.
>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>

Re: [HACKERS] merging some features from plpgsql2 project

[Pavel Stehule]

2017-01-09 0:39 GMT+01:00 Jim Nasby :

> On 1/7/17 11:44 PM, Pavel Stehule wrote:
>
>> This is not overloading of SQL command - it is like annotations. It is
>> smart idea, so I was not surprised if ANSI/SQL reuses it.
>>
>
> SHas ANSI declared that they will NEVER support := in a SELECT that's not
> running in a stored function? Because if they haven't done that, there's
> nothing preventing them from doing just that. If that happens we're going
> to have some very difficult choices to make.


No, there is nothing declared in ANSI. But currently in ANSI is not using
one operator for two different thing.

Regards

Pavel


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

Re: [HACKERS] Block level parallel vacuum WIP

[Simon Riggs]

On 9 January 2017 at 08:48, Masahiko Sawada  wrote:

> I had not considered necessity of dead lock detection support.

It seems like a big potential win to scan multiple indexes in parallel.

What do we actually gain from having the other parts of VACUUM execute
in parallel? Does truncation happen faster in parallel? ISTM we might
reduce the complexity of this if there is no substantial gain.

Can you give us some timings for performance of the different phases,
with varying levels of parallelism?

Does the design for collecting dead TIDs use a variable amount of
memory? Does this work negate the other work to allow VACUUM to use >
1GB memory?

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Parallel bitmap heap scan

[tushar]

On 01/09/2017 01:05 PM, Dilip Kumar wrote:

This patch can be used by 0003-parallel-bitmap-heap-scan-v7.patch
attached in the mail and also parallel-index-scan[2] can be rebased on
this, if this get committed,
After applying your patches against the fresh sources of PG v10 , not 
able to perform initdb


centos@tusharcentos7 bin]$ ./initdb -D data
The files belonging to this database system will be owned by user "centos".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.utf8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... sh: line 1: 30709 
Segmentation fault 
"/home/centos/PG10_9ja/postgresql/edbpsql/bin/postgres" --single -F -O 
-j -c search_path=pg_catalog -c exit_on_error=true template1 > /dev/null

child process exited with exit code 139
initdb: removing data directory "data"
[centos@tusharcentos7 bin]$

--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company



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

Re: [HACKERS] Replication/backup defaults

[Magnus Hagander]

On Sun, Jan 8, 2017 at 2:19 AM, Jim Nasby  wrote:

> On 1/5/17 2:50 PM, Tomas Vondra wrote:
>
>> Ultimately, the question is whether the number of people running into
>> "Hey, I can't take pg_basebackup or setup a standby with the default
>> config!" is higher or lower than number of people running into "Hey,
>> CREATE TABLE + COPY is slower now!"
>>
>
> I'm betting it's way higher. Loads of folks use Postgres and never do any
> kind of ETL.


I'm willing to say "the majority".



> That is not to say there are no other cases benefiting from those
>> optimizations, but we're talking about the default value - we're not
>> removing the wal_level=minimal.
>>
>
> This would be a non-issue if we provided example configs for a few
> different workloads. Obviously those would never be optimal either, but
> they *would* show users what settings they should immediately look at
> changing in their environment.


It might also be worthwhile to provide a section in the docs just saying
"these are the parameters you probably want to look at for workload "
rather than an actual example configuration. Including a short sentence or
two about why.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS

[Ashutosh Sharma]

Hi,

> The patch itself is really straight forward (I'm new to sending patches, so
> I've chosen a simple one), and there is only one thing that concerns me (as
> in, if I did it right/good). The difference in syntax for SCHEMAS and the
> other objects is that IN SCHEMA option makes no sense here (as we don't have
> nested schemas), and to solve that I simple added the error "cannot use IN
> SCHEMA clause when using GRANT/REVOKE ON SCHEMAS".
>
> Does that look good to you?

To me, It looks fine.

>
> Also, should I add translations for that error message in other languages (I
> can do that without help of tools for pt_BR) or is that a latter process in
> the releasing?
>

I think you should add it but i am not sure when it is done.

> Other than that, I added a few regression tests (similar to others used for
> ADP), and patched the documentation (my English is not that good, so I'm
> open to suggestions). Anything else I forgot?

You have forgot to change the description section of "ADP". In the
description section you need to mention that privileges for schemas
too can be altered along with other database objects. Other than that,
I feel the patch looks good and has no bug.

--
With Regards,
Ashutosh Sharma.
EnterpriseDB: http://www.enterprisedb.com


-- 
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] postgres_fdw bug in 9.6

[Ashutosh Bapat]

>
> Done.  Attached is the new version of the patch.
>
> * I'm still not sure the search approach is the right way to go, so I
> modified CreateLocalJoinPath so that it creates a mergejoin path that
> explicitly sorts both the outer and inner relations as in
> sort_inner_and_outer, by using the information saved in that function. I
> think we could try to create a sort-free mergejoin as in
> match_unsorted_outer, but I'm not sure it's worth complicating the code.

Why is this so?
 * If the outer cheapest-total path is parameterized by the inner
 * rel, we can't generate a nestloop path.
and
 * If either cheapest-total path is parameterized by the other
 * rel, we can't generate a hashjoin/mergejoin path.  (There's no

If the inner and/or outer paths are not ordered as required, we will need to
order them. Code below doesn't seem to handle that case.
/*
 * If the paths are already well enough ordered, we can
 * skip doing an explicit sort.
 */
if (outerkeys &&
pathkeys_contained_in(outerkeys, outer_path->pathkeys))
outerkeys = NIL;
if (innerkeys &&
pathkeys_contained_in(innerkeys, inner_path->pathkeys))
innerkeys = NIL;

> * I modified CreateLocalJoinPath so that it handles the cheapest-total paths
> for the outer/inner relations that are parameterized if possible.

I don't think we need to provide details of what kind of path the function
builds.
+ join plan.  CreateLocalJoinPath builds a nested loop join
+ path for the specified join relation, except when the join type is
+ FULL, in which case a merge or hash join path is built.


I am not able to understand the code or the comment below
+
+/* Save first mergejoin information for possible use by the FDW */
+if (outerkeys == all_pathkeys)
+{
+extra->mergeclauses = cur_mergeclauses;
+extra->merge_pathkeys = merge_pathkeys;
+extra->merge_outerkeys = outerkeys;
+extra->merge_innerkeys = innerkeys;
+}

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Parallel bitmap heap scan

[tushar]

On 01/09/2017 04:36 PM, Dilip Kumar wrote:

I have taken the latest code, applied all 3 patches and compiled.
Initdb is working fine for me.

Can you please verify, do you have any extra patch along with my patch?
Did you properly clean the code?
Thanks Dilip. issue is reproducible  if  we  uses '--enable-cassert' 
switch in the configure. We are able to reproduce it only with 
--enable-cassert' .


--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company



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

Re: [HACKERS] Make pg_basebackup -x stream the default

[Magnus Hagander]

On Sat, Jan 7, 2017 at 1:32 PM, Michael Paquier 
wrote:

> On Sat, Jan 7, 2017 at 12:04 AM, Magnus Hagander 
> wrote:
> > On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander 
> > wrote:
> >> Meh, just as I was going to respond "committed" I noticed this second
> >> round of review comments. Apologies, pushed without that.
> >>
> >> I agree on the change with includewal/streamwal. That was already the
> case
> >> in the existing code of course, but that doesn't mean it couldn't be
> made
> >> better. I'll take a look at doing that as a separate patch.
> >>
> >
> > Here's a patch that does this. Does this match what you were thinking?
>
> Yes, that's it. I have looked at the patch in details and that looks
> correct to me.
>

OK. Pushed. I agree it made it more readable, if nothing else.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Microvacuum support for Hash Index

[Jesper Pedersen]

Hi Ashutosh,

On 01/06/2017 12:54 AM, Ashutosh Sharma wrote:

using pgbench -M prepared -c 10 -j 10 -T 600 -f test.sql test

crashes after a few minutes with

TRAP: FailedAssertion("!(LWLockHeldByMeInMode(((LWLock*)
(&(bufHdr)->content_lock)), LW_EXCLUSIVE))", File: "bufmgr.c", Line: 3781)


Attached v4 patch fixes this assertion failure.



Yes, that fixes the problem.

However (master / WAL v7 / MV v4),

--- ddl.sql ---
CREATE TABLE test AS SELECT generate_series(1, 10) AS id, 0 AS val;
CREATE INDEX IF NOT EXISTS idx_id ON test USING hash (id);
CREATE INDEX IF NOT EXISTS idx_val ON test USING hash (val);
ANALYZE;
--- ddl.sql ---

--- test.sql ---
\set id random(1,10)
\set val random(0,10)
BEGIN;
DELETE FROM test WHERE id = :id;
INSERT INTO test VALUES (:id, :val);
COMMIT;
--- test.sql ---

gives

#9  0x0098a83e in elog_finish (elevel=20, fmt=0xb6ea92 
"incorrect local pin count: %d") at elog.c:1378
#10 0x007f0b33 in LockBufferForCleanup (buffer=1677) at 
bufmgr.c:3605
#11 0x00549390 in XLogReadBufferForRedoExtended 
(record=0x2afced8, block_id=1 '\001', mode=RBM_NORMAL, 
get_cleanup_lock=1 '\001', buf=0x7ffe3ee27c8c) at xlogutils.c:394
#12 0x004c5026 in hash_xlog_vacuum_one_page (record=0x2afced8) 
at hash_xlog.c:1109

#13 0x004c5547 in hash_redo (record=0x2afced8) at hash_xlog.c:1214
#14 0x0053a361 in StartupXLOG () at xlog.c:6975
#15 0x007a4ca0 in StartupProcessMain () at startup.c:216

on the slave instance in a master-slave setup.

Also, the src/backend/access/README file should be updated with a 
description of the changes which this patch provides.


Best regards,
 Jesper



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

[Robert Haas]

On Sun, Jan 8, 2017 at 4:59 AM, Gavin Flower
 wrote:
>> Is this completely unrealistic or is it carved in stone PostgreSQL will
>> always be a C project forever and ever?
>>
> From my very limited understanding, PostgreSQL is more likely to be
> converted to C++!

I'm tempted to snarkily reply that we should start by finishing the
conversion of PostgreSQL from LISP to C before we worry about
converting it to anything else.  There are various code comments that
imply that it actually was LISP at one time and I can certainly
believe that given our incredibly wasteful use of linked lists in so
many places.  gram.y asserts that this problem was fixed as far as the
grammar is concerned...

 *AUTHORDATEMAJOR EVENT
 *Andrew Yu Sept, 1994
POSTQUEL to SQL conversion
 *Andrew Yu Oct, 1994   lispy
code conversion

...but I think it'd be fair to say that even there it was fixed only in part.

Anyway, with regards to either Rust (which I know very little about)
or C++ (which I know more about) I think it would be more promising to
think about enabling extensions to be written in such languages than
to think about converting the entire source base.  A system like
PostgreSQL is almost a language of its own; we don't really code for
PostgreSQL in C, but in "PG-C".  Learning the PG-specific idioms is
arguably more work than learning C itself, and that would still be
true, I think, if we had a "PG-C++" or a "PG-Rust" or a "PG-D"
variant.  Still, if having such variants drew more programmers to work
on extending PostgreSQL, I think that would be worth some work on our
part to enable it.  However, maintaining multiple copies of our
1.2-million-line source base just for easier reference by people more
familiar with one of those languages than with C sounds to me like it
would create more problems than it would solve.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Increase pltcl test coverage

[Tom Lane]

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

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

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

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

regards, tom lane


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Peter Eisentraut]

On 1/9/17 10:50 AM, Vladimir Rusinov wrote:
> pg_is_xlog_replay_paused| pg_is_recovery_paused

All the other xlog_replay names have been changed to wal_replay, so
making this one different is probably not so good.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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: new set of psql patches for loading (saving) data from (to) text, binary files

[Jason O'Donnell]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, failed

Pavel,

gstore/gbstore:

The functionality worked as expected - one row, one column results of queries 
can be sent to a file or shell.  It would be nice if a test case was included 
that proves results more than one row, one column wide will fail.

The documentation included is awkward to read.  How about:

"Sends the current query input buffer to the server and stores
the result to an output file specified in the query or pipes the output 
to a shell command.  The file or command are written to only if the query 
successfully returns exactly one, non-null row and column.  If the 
query fails or does not return data, an error is raised. "


Parameterized Queries:

The functionality proposed works as expected.  Throughout the documentation, 
code and test cases the word "Parameterized" is spelled incorrectly: 
"PARAMETRIZED_QUERIES"


set_from_file/set_from_bfile:

The functionality proposed worked fine, I was able to set variables in sql from 
files.  Minor typo in the documentation:
"The content is escapeaed as bytea value."

Hope this helps!

Jason O'Donnell
Crunchy Data

The new status of this patch is: Waiting on Author

-- 
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] UNDO and in-place update

[Robert Haas]

On Mon, Jan 9, 2017 at 7:50 AM, Amit Kapila  wrote:
> Okay, I see the point.  I think here UNDO pointer can be marked
> invalid either during page-pruning or vacuum.

I explicitly want to avoid that, because it means re-dirtying the
page.  The UNDO pointer becomes invalid by removing the UNDO log to
which it points; the page itself does not need to be dirtied for the
UNDO pointer to become invalid.

> Another point which I think is not discussed till now is, how the
> locking will work.  As of now, we first lock the tuple and then xid on
> which we have to wait. In the new design, we don't know the xid which
> has updated the tuple and I am not sure there is any easy way to find
> that information.

Yes, there is: you can get it from the UNDO pointer.  Each UNDO
pointer is of the form . If the page has a regular
UNDO pointer, then you can get the XID for any modified table right
out of the UNDO pointer itself.  If the page has a TPD pointer, the
TPD contains a series of UNDO pointers and you can get all of the XIDs
that have touched the page by iterating through those UNDO pointers.
There is some work to do to make it cheap to find which XID pertains
to which updated tuple, but I think that problem can be solved by
choosing carefully what to store in the TPD.  I think, in fact, that
for performance it's absolutely essential to solve that problem well.

> One idea could be that we have some fixed number of
> slots (i think we can make it variable as well, but for simplicity,
> lets consider it as fixed) in the page header which will store the
> offset to the transaction id inside a TPD entry of the page.  Consider
> a TPD entry of page contains four transactions, so we will just store
> enough information in heap page header to reach the transaction id for
> these four transactions. I think each such page header slot could be
> three or four bits long depending upon how many concurrent
> transactions we want to support on a page after which a new
> transaction has to wait (I think in most workloads supporting
> simultaneous eight transactions on a page should be sufficient).
> Then we can have an additional byte (or less than byte) in the tuple
> header to store lock info which is nothing but an offset to the slot
> in the page header.   We might find some other locking technique as
> well, but I think keeping it same as current has benefit.

Yes, something like this can be done.  You don't really need any new
page-level header data, because you can get the XIDs from the TPD
entry (or from the page itself if there's only one).  But you could
expand the single "is-modified" bit that I've proposed adding to each
tuple to multiple bits.  0 means not recently modified.  1 means
modified by the first or only transaction that has recently modified
the page.  2 means modified by the second transaction that has
recently modified the page.  Etc.

What I was thinking about doing instead is storing an array in the TPD
containing the same information.  There would be one byte or one half
a byte or whatever per TID and it would contain the index of the XID
in the TPD that had most recently modified or locked that TID.  Your
solution might be better, though, at least for cases where the number
of tuples that have modified the page is small.  However, I'm not
totally sure.  I think it's important to keep the tuple headers VERY
small, like 3 bytes.  Or 2 bytes.  Or maybe even variable size but
only 1 byte in common cases.  So I expect bit space in those places to
be fairly scarce and precious.

Now that might be the wrong idea -- maybe it's worth expanding that
header in order to speed things up.  On the other hand, having to read
more database pages in order to process the same amount of data is
*really* expensive, especially when you start talking about data sets
that probably don't fit in memory, like a 10 TB or 100 TB table.  If
you've got 100 tuples per page (~81 bytes per tuple), increasing the
size of a tuple by 1 byte causes that 100 TB table to increase in size
by about 1.2 TB (modulo padding effects).  An extra byte of space (or
even an extra ten bytes) doesn't matter much for a table with a
million tuples in it because the whole table is going to fit in memory
either way, but when you have 10+ billion tuples those extra bytes
start to matter a LOT.  And the problem is not only or even primarily
the space consumption - the issue is that all of your queries run that
much slower because of the extra I/O.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Incorrect XLogRegisterBuffer flag for revmapbuf in brin

[Alvaro Herrera]

Kuntal Ghosh wrote:
> Added to next commitfest for tracking. I should've done it earlier.

Yeah -- I hadn't noticed this thread until last week.  I intend to apply
this very soon.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] ALTER TABLE .. ALTER COLUMN .. ERROR: attribute .. has wrong type

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Tom Lane wrote:
> >> Hmm.  The bespoke code for constructing the attno map bothers me;
> >> surely there is existing code that does that?  If not, it'd still
> >> make more sense to factor it out, I think, because there will be
> >> other needs for it in future.
> 
> > There isn't any that I could find -- all the existing callers of
> > map_variable_attnos build their map in other ways (while walking an
> > attribute array at construction time).
> 
> [ pokes around... ]  The code I was thinking of is convert_tuples_by_name
> in access/common/tupconvert.c.  There's a bit of an API mismatch in that
> it wants to wrap the mapping array in a TupleConversionMap struct; but
> maybe we could refactor tupconvert.c to offer a way to get just the map
> array.

Ah, nice gadget.  I think the attached patch should do.

> > I also modified the algorithm to use the relcache instead of walking the
> > child's attribute list for each parent attribute (that was silly).
> 
> Hmm.  That might be better in a big-O sense but I doubt it's faster for
> reasonable numbers of columns.

Hm, I was thinking in unreasonable numbers of columns, keeping in mind
that they can appear in arbitrary order in child tables.  Then again,
that probably seldom occurs in real databases.  I suppose this could
become an issue with table partitioning becoming more common, but I'm
okay with deferring the optimization work.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/common/tupconvert.c 
b/src/backend/access/common/tupconvert.c
index 945a6a2..bdd3aa4 100644
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -206,55 +206,12 @@ convert_tuples_by_name(TupleDesc indesc,
 {
TupleConversionMap *map;
AttrNumber *attrMap;
-   int n;
+   int n = outdesc->natts;
int i;
boolsame;
 
/* Verify compatibility and prepare attribute-number map */
-   n = outdesc->natts;
-   attrMap = (AttrNumber *) palloc0(n * sizeof(AttrNumber));
-   for (i = 0; i < n; i++)
-   {
-   Form_pg_attribute att = outdesc->attrs[i];
-   char   *attname;
-   Oid atttypid;
-   int32   atttypmod;
-   int j;
-
-   if (att->attisdropped)
-   continue;   /* attrMap[i] is 
already 0 */
-   attname = NameStr(att->attname);
-   atttypid = att->atttypid;
-   atttypmod = att->atttypmod;
-   for (j = 0; j < indesc->natts; j++)
-   {
-   att = indesc->attrs[j];
-   if (att->attisdropped)
-   continue;
-   if (strcmp(attname, NameStr(att->attname)) == 0)
-   {
-   /* Found it, check type */
-   if (atttypid != att->atttypid || atttypmod != 
att->atttypmod)
-   ereport(ERROR,
-   
(errcode(ERRCODE_DATATYPE_MISMATCH),
-errmsg_internal("%s", 
_(msg)),
-errdetail("Attribute 
\"%s\" of type %s does not match corresponding attribute of type %s.",
-  
attname,
-  
format_type_be(outdesc->tdtypeid),
-  
format_type_be(indesc->tdtypeid;
-   attrMap[i] = (AttrNumber) (j + 1);
-   break;
-   }
-   }
-   if (attrMap[i] == 0)
-   ereport(ERROR,
-   (errcode(ERRCODE_DATATYPE_MISMATCH),
-errmsg_internal("%s", _(msg)),
-errdetail("Attribute \"%s\" of type %s 
does not exist in type %s.",
-  attname,
-  
format_type_be(outdesc->tdtypeid),
-  
format_type_be(indesc->tdtypeid;
-   }
+   attrMap = convert_tuples_by_name_map(indesc, outdesc, msg);
 
/*
 * Check to see if the map is one-to-one and the tuple types are the 
same.
@@ -313,6 +270,69 @@ convert_tuples_by_name(TupleDesc indesc,
 }
 
 /*
+ * Return a palloc'd bare attribute map for 

Re: [HACKERS] _hash_addovflpage has a bug

[Robert Haas]

On Fri, Jan 6, 2017 at 11:32 PM, Amit Kapila  wrote:
> On Sat, Jan 7, 2017 at 2:33 AM, Robert Haas  wrote:
>> It looks to to me like the recent hash index changes have left
>> _hash_addovflpage slightly broken.  I think that if that function
>> reaches the point where it calls _hash_getbuf() to fetch the next page
>> in the bucket chain, we also need to clear retain_pin.  Otherwise,
>> we'll erroneously think that we're supposed to retain a pin even when
>> the current page is an overflow page rather than the primary bucket
>> page, which is wrong.
>>
>
> How?  I think we are ensuring that we retain pin only if it is a
> bucket page.  The check is as below:
> if ((pageopaque->hasho_flag & LH_BUCKET_PAGE) && retain_pin)

Oh, right.  So I guess there's no bug, but I still think that's pretty
ugly.  How about:

if (retain_pin)
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
else
_hash_relbuf(rel, buf);
retain_pin = false;

instead?

>> A larger question, I suppose, is why this function wants to be certain
>> of adding a new page even if someone else has already done so.  It
>> seems like it might be smarter for it to just return the newly added
>> page to the caller and let the caller try to use it.  _hash_doinsert
>> has an Assert() that the tuple fits on the returned page, but that
>> could be deleted.  As it stands, if two backends try to insert a tuple
>> into the same full page at the same time, both of them will add an
>> overflow page and we'll end up with 2 overflow pages each containing 1
>> tuple.
>
> I think it is because in the current algorithm we first get an
> overflow page and then add it to the end.  Now we can change it such
> that first, we acquire the lock on the tail page, check if we still
> need an overflow page, if so, then proceed, else return the already
> added overflow page.

For the WAL patch, this is all going to need to be atomic anyway,
right?  Like, you have to allocate the overflow page and add it to the
bucket chain as a single logged operation?  Not sure exactly how that
works out in terms of locking.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Robert Haas]

On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
 wrote:
> On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
>> We will need to make CURRENT_DATABASE a reserved keyword. But I like
>> this idea more than COMMENT ON CURRENT DATABASE.
>
> We already have the reserved key word CURRENT_CATALOG, which is the
> standard spelling.  But I wouldn't be bothered if we made
> CURRENT_DATABASE somewhat reserved as well.

Maybe I'm just lacking in imagination, but what's the argument against
spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
anything new at all, and it also looks more SQL-ish to me.  SQL
generally tries to emulate English, and I don't normally
speak_hyphenated_words.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] increasing the default WAL segment size

[Robert Haas]

On Sat, Jan 7, 2017 at 7:45 PM, Jim Nasby  wrote:
> Well, now that there's 3 places that need to do almost the same thing, I
> think it'd be best to just centralize this somewhere. I realize that's not
> going to save any significant amount of code, but it would make it crystal
> clear what's going on (assuming the excellent comment above RIGHTMOST_ONE
> was kept).

Hmm.  This sounds a lot like what fls() and my_log2() also do.  I've
been quietly advocating for fls() because we only provide an
implementation in src/port if the operating system doesn't have it,
and the operating system may have an implementation that optimizes to
a single machine-language instruction (bsrl on x86, I think, see
4f658dc851a73fc309a61be2503c29ed78a1592e).  But the fact that our
src/port implementation uses a loop instead of the RIGHTMOST_ONE()
trick seems non-optimal.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Fri, Jan 6, 2017 at 3:51 PM, Vitaly Burovoy 
wrote:

> On 1/4/17, Haribabu Kommi  wrote:
> > On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi <
> kommi.harib...@gmail.com>
> > wrote:
> >> Updated patch attached with added cast function from macaddr8 to
> >> macaddr.
> >>
> >> Currently there are no support for cross operators. Is this required
> >> to be this patch only or can be handled later if required?
> >>
> >
> > Updated patch attached to address the duplicate OID problem.
> > There are no other functional changes to the previous patch.
>
> Hello,
>
> several thoughts about the patch:
>
>
Thanks for the review.


> Documentation:
> 1.
> + The remaining six input formats are not part of any standard.
> Which ones (remaining six formats)?
>

Updated the documentation to point to correct six formats.

2.
> + trunc(macaddr8) returns a
> MAC
> +   address with the last 3 bytes set to zero.
> It is a misprinting or a copy-paste error.
> The implementation and the standard says that the last five bytes are
> set to zero and the first three are left as is.
>

Fixed.


> 3.
> + for lexicographical ordering
> I'm not a native English speaker, but I'd say just "for ordering"
> since there are no words, it is just a big number with a special
> input/output format.
>

Changed accordingly.


> The code:
> 4.
> +   if ((a == 0) && (b == 0) && (c == 0) && (d == 0)
> +   && (e == 0) && (f == 0) && (g == 0) && (h == 0))
> ...
> +   if ((a == 255) && (b == 255) && (c == 255) && (d == 255)
> +   && (e == 255) && (f == 255) && (g == 255) && (h ==
> 255))
> The standard forbids these values from using in real hardware, no
> reason to block them as input data.
> Moreover these values can be stored as a result of binary operations,
> users can dump them but can not restore.
>

Ok. Removed the above code that blocks the input.


>
> 5.
> +   if (!eight_byte_address)
> +   {
> +   result->d = 0xFF;
> ...
>
> Don't you think the next version is simplier (all sscanf for macaddr6
> skip "d" and "e")?
>
> +   count = sscanf(str, "%x:%x:%x:%x:%x:%x%1s",
> +  , , , , , , junk);
> ...
> +   if (!eight_byte_address)
> +   {
> +   d = 0xFF;
> +   e = 0xFE;
> +   }
> +   result->a = a;
> +   result->b = b;
> +   result->c = c;
> +   result->d = d;
> +   result->e = e;
> +   result->f = f;
> +   result->g = g;
> +   result->h = h;
>
> Also:
> +   if (buf->len == 6)
> +   {
> +   addr->d = 0xFF;
> +   addr->e = 0xFE;
> +   addr->f = pq_getmsgbyte(buf);
> +   addr->g = pq_getmsgbyte(buf);
> +   addr->h = pq_getmsgbyte(buf);
> +   }
> +   else
> +   {
> +   addr->d = pq_getmsgbyte(buf);
> +   addr->e = pq_getmsgbyte(buf);
> +   addr->f = pq_getmsgbyte(buf);
> +   addr->g = pq_getmsgbyte(buf);
> +   addr->h = pq_getmsgbyte(buf);
> +   }
>
> can be written as:
> +   if (buf->len == 6)
> +   {
> +   addr->d = 0xFF;
> +   addr->e = 0xFE;
> +   }
> +   else
> +   {
> +   addr->d = pq_getmsgbyte(buf);
> +   addr->e = pq_getmsgbyte(buf);
> +   }
> +   addr->f = pq_getmsgbyte(buf);
> +   addr->g = pq_getmsgbyte(buf);
> +   addr->h = pq_getmsgbyte(buf);
>
> but it is only my point of view (don't need to pay close attention
> that only those two bytes are written differently, not the last tree
> ones), it is not an error.
>

Updated as per you suggestion.


> 6.
> +errmsg("macaddr8 out of range to convert
> to macaddr")));
> I think a hint should be added which values are allowed to convert to
> "macaddr".
>

Added the hint message to explain in detail about addresses that are
eligible for
conversion from macaddr8 type to macaddr.

7.
> +DATA(insert (  829 7744123 i f ));
> +DATA(insert (  774  829   4124 i f ));
> It is a nitpicking, but try to use tabs as in the code around.
> (tab between "774" and "829" and space instead of tab between "829" and
> "4124").
>

Done the indentation to correct the problem.


> 8.
> +#define hibits(addr) \
> +  ((unsigned long)(((addr)->a<<24)|((addr)->b<<16)|((addr)->c<<8)))
> +
> +#define lobits(addr) \
> +  ((unsigned long)(((addr)->d<<16)|((addr)->e<<8)|((addr)->f)))
> +
> +#define lobits_extra(addr) \
> +  ((unsigned long)((addr)->g<<8)|((addr)->h))
>
> I mentioned that fitting all 4 bytes is a wrong idea[1]
> > The macros "hibits" should be 3 octets long, not 4;
>
> ... but now I do not think so (there is no UB[2] because source and
> destination are not signed).
> Moreover you've already fill in "hibits" the topmost byte by shifting by
> 24.
> If you use those two macros ("hibits" and "lobits") it allows to 

Re: [HACKERS] Declarative partitioning - another take

[amul sul]

Hi,

I got server crash due to assert failure at ATTACHing overlap rang
partition, here is test case to reproduce this:

CREATE TABLE test_parent(a int) PARTITION BY RANGE (a);
CREATE TABLE test_parent_part2 PARTITION OF test_parent FOR VALUES
FROM(100) TO(200);
CREATE TABLE test_parent_part1(a int NOT NULL);
ALTER TABLE test_parent ATTACH PARTITION test_parent_part1 FOR VALUES
FROM(1) TO(200);

I think, this bug exists in the following code of check_new_partition_bound():

 767 if (equal || off1 != off2)
 768 {
 769 overlap = true;
 770 with = boundinfo->indexes[off2 + 1];
 771 }

When equal is true array index should not be 'off2 + 1'.

While reading code related to this, I wondered why
partition_bound_bsearch is not immediately returns when cmpval==0?

Apologise if this has been already reported.

Regards,
Amul


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