date:20130916

Re: [HACKERS] plpgsql.print_strict_params

2013-09-16 Thread Ian Lawrence Barwick

2013/9/15 Marko Tiikkaja ma...@joh.to:

 Attached an updated patch to fix the tabs and to change this to a
 compile-time option.  Now it's not possible to flexibly disable the feature
 during runtime, but I think that's fine.

I'm taking a look at this patch as part of the current commitfest [*],
(It's the first time I've formally reviewed a patch for a commitfest
so please let me know if I'm missing something.)

[*] https://commitfest.postgresql.org/action/patch_view?id=1221

Submission review
-
* Is the patch in a patch format which has context?
Yes.

* Does it apply cleanly to the current git master?
Yes. No new files are introduced by this patch.

* Does it include reasonable tests, necessary doc patches, etc?
Yes.

However the sample function provided in the documentation throws a
runtime error due to a missing FROM-clause entry.


Usability review

* Does the patch actually implement that?
Yes.

* Do we want that?
It seems like it would be useful; no opposition has been raised on
-hackers so far.

* Do we already have it?
No.

* Does it follow SQL spec, or the community-agreed behavior?
SQL spec: n/a. I do note that it makes use of the # syntax
before the body of a PL/pgSQL function, which is currently only
used for #variable_conflict [*]. I can imagine this syntax might
be used for other purposes down the road, so it might be worth
keeping an eye on it before it becomes a hodge-podge of ad-hoc
options.

[*] http://www.postgresql.org/docs/current/static/plpgsql-implementation.html

* Does it include pg_dump support (if applicable)?
n/a

* Are there dangers?
I don't see any.

* Have all the bases been covered?

This lineis in exec_get_query_params() and exec_get_dynquery_params():

return (no parameters);

Presumably the message will escape translation and this line should be better
written as:
   return _((no parameters));

Also, if the offending query parameter contains a single quote, the output
is not escaped:

postgres=# select get_userid(E'foo''');
ERROR:  query returned more than one row
DETAIL:  p1 = 'foo''
CONTEXT:  PL/pgSQL function get_userid(text) line 9 at SQL statement

Not sure if that's a real problem though.


Feature test


* Does the feature work as advertised?
Yes.

* Are there corner cases the author has failed to consider?
I can't see any.

* Are there any assertion failures or crashes?
No.


Performance review
--

* Does the patch slow down simple tests?
No.

* If it claims to improve performance, does it?
n/a

* Does it slow down other things?
No.


Coding review
-

* Does it follow the project coding guidelines?
Yes.

The functions added in pl_exec.c - exec_get_query_params() and
exec_get_dynquery_params() do strike me as potentially misnamed,
as they don't actually execute anything but are more utility
functions for generating formatted output.

Maybe format_query_params() etc. would be better?


* Are there portability issues?
I don't think so.

* Will it work on Windows/BSD etc?
Tested on OS X and Linux. I don't see anything, e.g. system calls,
which might stop it working on Windows.

* Are the comments sufficient and accurate?
exec_get_query_params() and exec_get_dynquery_params()
could do with some brief comments explaining their purpose.


* Does it do what it says, correctly?
As far as I can tell, yes.

* Does it produce compiler warnings?
No.

* Can you make it crash?
So far, no.


Architecture review
---

* Is everything done in a way that fits together coherently with other
features/modules?
Patch affects PL/pgSQL only.

* Are there interdependencies that can cause problems?
No.


Regards

Ian Barwick


-- 
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] \h open

2013-09-16 Thread Heikki Linnakangas


On 16.09.2013 07:43, Oleg Bartunov wrote:

I noticed there is nothing available in built-in psql help about OPEN
command. Does it intentional ?


That's because there is no OPEN command. PL/pgSQL does have an OPEN 
statement, but that's just PL/pgSQL. And ecpg too, but again that's 
handled by ecpg, not the backend.


- Heikki


--
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] Statistics collection for CLUSTER command

2013-09-16 Thread Satoshi Nagayasu


(2013/08/08 20:52), Vik Fearing wrote:

As part of routine maintenance monitoring, it is interesting for us to
have statistics on the CLUSTER command (timestamp of last run, and
number of runs since stat reset) like we have for (auto)ANALYZE and
(auto)VACUUM.  Patch against today's HEAD attached.

I would add this to the next commitfest but I seem to be unable to log
in with my community account (I can log in to the wiki).  Help appreciated.


I have reviewed the patch.

Succeeded to build with the latest HEAD, and passed the regression
tests.

Looks good enough, and I'd like to add a test case here, not only
for the view definition, but also working correctly.

Please take a look at attached one.

Regards,
--
Satoshi Nagayasu sn...@uptime.jp
Uptime Technologies, LLC. http://www.uptime.jp
diff --git a/src/test/regress/expected/stats.out 
b/src/test/regress/expected/stats.out
index 56bace1..ba614b2 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -28,7 +28,13 @@ SELECT pg_sleep(2.0);
 CREATE TEMP TABLE prevstats AS
 SELECT t.seq_scan, t.seq_tup_read, t.idx_scan, t.idx_tup_fetch,
(b.heap_blks_read + b.heap_blks_hit) AS heap_blks,
-   (b.idx_blks_read + b.idx_blks_hit) AS idx_blks
+   (b.idx_blks_read + b.idx_blks_hit) AS idx_blks,
+   coalesce(t.last_vacuum, now()) AS last_vacuum,
+   coalesce(t.last_analyze, now()) AS last_analyze,
+   coalesce(t.last_cluster, now()) AS last_cluster,
+   t.vacuum_count,
+   t.analyze_count,
+   t.cluster_count
   FROM pg_catalog.pg_stat_user_tables AS t,
pg_catalog.pg_statio_user_tables AS b
  WHERE t.relname='tenk2' AND b.relname='tenk2';
@@ -111,4 +117,27 @@ SELECT st.heap_blks_read + st.heap_blks_hit = 
pr.heap_blks + cl.relpages,
  t| t
 (1 row)
 
+-- table maintenance stats
+ANALYZE tenk2;
+VACUUM tenk2;
+CLUSTER tenk2 USING tenk2_unique1;
+SELECT pg_sleep(1.0);
+ pg_sleep 
+--
+ 
+(1 row)
+
+SELECT st.last_vacuum  pr.last_vacuum,
+   st.last_analyze  pr.last_analyze,
+   st.last_cluster  pr.last_cluster,
+   st.vacuum_count  pr.vacuum_count,
+   st.analyze_count  pr.analyze_count,
+   st.cluster_count  pr.cluster_count
+  FROM pg_stat_user_tables AS st, prevstats pr
+ WHERE st.relname='tenk2';
+ ?column? | ?column? | ?column? | ?column? | ?column? | ?column? 
+--+--+--+--+--+--
+ t| t| t| t| t| t
+(1 row)
+
 -- End of Stats Test
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index bb349b2..71e5e27 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -22,7 +22,13 @@ SELECT pg_sleep(2.0);
 CREATE TEMP TABLE prevstats AS
 SELECT t.seq_scan, t.seq_tup_read, t.idx_scan, t.idx_tup_fetch,
(b.heap_blks_read + b.heap_blks_hit) AS heap_blks,
-   (b.idx_blks_read + b.idx_blks_hit) AS idx_blks
+   (b.idx_blks_read + b.idx_blks_hit) AS idx_blks,
+   coalesce(t.last_vacuum, now()) AS last_vacuum,
+   coalesce(t.last_analyze, now()) AS last_analyze,
+   coalesce(t.last_cluster, now()) AS last_cluster,
+   t.vacuum_count,
+   t.analyze_count,
+   t.cluster_count
   FROM pg_catalog.pg_stat_user_tables AS t,
pg_catalog.pg_statio_user_tables AS b
  WHERE t.relname='tenk2' AND b.relname='tenk2';
@@ -81,4 +87,20 @@ SELECT st.heap_blks_read + st.heap_blks_hit = pr.heap_blks 
+ cl.relpages,
   FROM pg_statio_user_tables AS st, pg_class AS cl, prevstats AS pr
  WHERE st.relname='tenk2' AND cl.relname='tenk2';
 
+-- table maintenance stats
+ANALYZE tenk2;
+VACUUM tenk2;
+CLUSTER tenk2 USING tenk2_unique1;
+
+SELECT pg_sleep(1.0);
+
+SELECT st.last_vacuum  pr.last_vacuum,
+   st.last_analyze  pr.last_analyze,
+   st.last_cluster  pr.last_cluster,
+   st.vacuum_count  pr.vacuum_count,
+   st.analyze_count  pr.analyze_count,
+   st.cluster_count  pr.cluster_count
+  FROM pg_stat_user_tables AS st, prevstats pr
+ WHERE st.relname='tenk2';
+
 -- End of Stats Test

-- 
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] Cube extension point support // GSoC'13

2013-09-16 Thread Heikki Linnakangas

On 12.07.2013 14:57, Stas Kelvich wrote:

Hello.

here is a patch adding to cube extension support for compressed representation
of point cubes. If cube is a point, i.e. has coincident lower left and upper
right corners, than only one corner is stored. First bit of the cube header
indicates whether the cube is point or not. Few moments:

* Patch preserves binary compatibility with old indices
* All functions that create cubes from user input, check whether it is a point
or not
* All internal functions that can return cubes takes care of all cases where a
cube might become a point

Great!

cube_is_point() needs to still handle old-style points. An NDBOX without
the point-flag set, where the ll and ur coordinates for each dimension
are the same, still needs to be considered a point. Even if you are
careful to never construct such structs in the code, they can be present
on-disk if you have upgraded from an earlier version with pg_upgrade.
Same in cube_out().

* Added tests for checking correct point behavior

You'll need to adjust all the expected output files, not only cube_1.out.

Also this patch includes adapted Alexander Korotkov's patch with kNN-based
ordering operator, which he wrote for postgresql-9.0beta1 with knngist patch.
More info there
http://www.postgresql.org/message-id/aanlktimhfaq6hcibrnk0tlcqmiyhywhwaq2zd87wb...@mail.gmail.com

To make review easier, it would be better to keep that as a separate
patch, actually. Could you split it up again, please?

- Heikki

--
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] Triggers on foreign tables

2013-09-16 Thread Ronan Dunklau

On Thursday 12 September 2013 12:10:01 Peter Eisentraut wrote:
 The documentation build fails:
 
 openjade:trigger.sgml:72:9:E: end tag for ACRONYM omitted, but OMITTAG
 NO was specified
 openjade:trigger.sgml:70:56: start tag was here

Thank you, I took the time to install a working doc-building environment. 
Please find attached a patch that corrects this.diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index e5ec738..08d133c 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -93,7 +93,7 @@ CREATE [ CONSTRAINT ] TRIGGER replaceable class=PARAMETERname/replaceable
 
   para
The following table summarizes which types of triggers may be used on
-   tables and views:
+   tables, views and foreign tables:
   /para
 
   informaltable id=supported-trigger-types
@@ -111,7 +111,7 @@ CREATE [ CONSTRAINT ] TRIGGER replaceable class=PARAMETERname/replaceable
   entry align=center morerows=1literalBEFORE//entry
   entry align=centercommandINSERT//commandUPDATE//commandDELETE//entry
   entry align=centerTables/entry
-  entry align=centerTables and views/entry
+  entry align=centerTables, views and foreign tables/entry
  /row
  row
   entry align=centercommandTRUNCATE//entry
@@ -122,7 +122,7 @@ CREATE [ CONSTRAINT ] TRIGGER replaceable class=PARAMETERname/replaceable
   entry align=center morerows=1literalAFTER//entry
   entry align=centercommandINSERT//commandUPDATE//commandDELETE//entry
   entry align=centerTables/entry
-  entry align=centerTables and views/entry
+  entry align=centerTables, views and foreign tables/entry
  /row
  row
   entry align=centercommandTRUNCATE//entry
@@ -244,7 +244,7 @@ UPDATE OF replaceablecolumn_name1/replaceable [, replaceablecolumn_name2/
 termreplaceable class=parametertable_name/replaceable/term
 listitem
  para
-  The name (optionally schema-qualified) of the table or view the trigger
+  The name (optionally schema-qualified) of the table, view or foreign table the trigger
   is for.
  /para
 /listitem
diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index f579340..37c0a35 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -33,7 +33,7 @@
para
 A trigger is a specification that the database should automatically
 execute a particular function whenever a certain type of operation is
-performed.  Triggers can be attached to both tables and views.
+performed.  Triggers can be attached to tables, views and foreign tables.
   /para
 
   para
@@ -64,6 +64,15 @@
/para
 
para
+On foreign tables, trigger can be defined to execute either before or after any
+commandINSERT/command,
+commandUPDATE/command or
+commandDELETE/command operation, only once per acronymSQL/acronym statement.
+This means that row-level triggers are not supported for foreign tables.
+   /para
+
+
+   para
 The trigger function must be defined before the trigger itself can be
 created.  The trigger function must be declared as a
 function taking no arguments and returning type literaltrigger/.
@@ -96,6 +105,7 @@
 after may only be defined at statement level, while triggers that fire
 instead of an commandINSERT/command, commandUPDATE/command,
 or commandDELETE/command may only be defined at row level.
+On foreign tables, triggers can not be defined at row level.
/para
 
para
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index adc74dd..68bfa9c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3141,13 +3141,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			pass = AT_PASS_MISC;
 			break;
 		case AT_EnableTrig:		/* ENABLE TRIGGER variants */
-		case AT_EnableAlwaysTrig:
-		case AT_EnableReplicaTrig:
 		case AT_EnableTrigAll:
 		case AT_EnableTrigUser:
 		case AT_DisableTrig:	/* DISABLE TRIGGER variants */
 		case AT_DisableTrigAll:
 		case AT_DisableTrigUser:
+ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+pass = AT_PASS_MISC;
+break;
+		case AT_EnableReplicaTrig:
+		case AT_EnableAlwaysTrig:
 		case AT_EnableRule:		/* ENABLE/DISABLE RULE variants */
 		case AT_EnableAlwaysRule:
 		case AT_EnableReplicaRule:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..d84b61b 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -184,12 +184,23 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 			RelationGetRelationName(rel)),
 	 errdetail(Views cannot have TRUNCATE triggers.)));
 	}
-	else
+	else if (rel-rd_rel-relkind == RELKIND_FOREIGN_TABLE)
+	{
+		if(stmt-row){
+			ereport(ERROR,
+	(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+	 errmsg(\%s\ is a foreign table,
+

Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-09-16 Thread Satoshi Nagayasu


(2013/07/06 1:16), Pavel Stehule wrote:

I am sending a patch that removes strict requirements for DROP IF
EXISTS statements. This behave is similar to our ALTER IF EXISTS
behave now.


postgres=# DROP CAST IF EXISTS (sss AS public.casttesttype);
NOTICE:  types sss and public.casttesttype does not exist, skipping
DROP CAST
postgres=#   DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget);
NOTICE:  function public.pt_in_widget(point,widget) does not exist, skipping
DROP FUNCTION
postgres=#  DROP OPERATOR IF EXISTS public.% (point, widget);
NOTICE:  operator public.% does not exist, skipping
DROP OPERATOR
postgres=# DROP TRIGGER test_trigger_exists ON no_such_table;
ERROR:  relation no_such_table does not exist
postgres=# DROP TRIGGER IF EXISTS test_trigger_exists ON no_such_table;
NOTICE:  trigger test_trigger_exists for table no_such_table does
not exist, skipping
DROP TRIGGER

This functionality is necessary for correct quite reload from dump
without possible warnings


I'm looking at this patch, and I have a question here.

Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger
and non-existing table? Or just only for non-existing trigger?

I've not understood the pg_restore issue precisely so far,
but IMHO DROP TRIGGER IF EXISTS means if the _trigger_ exists,
not if the _table_ exists.

Is this a correct and/or an expected behavior?

Sorry if I missed some consensus which we already made.

Any comments?

Regards,
--
Satoshi Nagayasu sn...@uptime.jp
Uptime Technologies, LLC. http://www.uptime.jp


--
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] GIN improvements part 1: additional information

2013-09-16 Thread Heikki Linnakangas


On 15.09.2013 12:14, Alexander Korotkov wrote:

On Sat, Jun 29, 2013 at 12:56 PM, Heikki Linnakangas
hlinnakan...@vmware.com  wrote:


There's a few open questions:

1. How are we going to handle pg_upgrade? It would be nice to be able to
read the old page format, or convert on-the-fly. OTOH, if it gets too
complicated, might not be worth it. The indexes are much smaller with the
patch, so anyone using GIN probably wants to rebuild them anyway, sooner or
later. Still, I'd like to give it a shot.

2. The patch introduces a small fixed 32-entry index into the packed
items. Is that an optimal number?

3. I'd like to see some performance testing of insertions, deletions, and
vacuum. I suspect that maintaining the 32-entry index might be fairly
expensive, as it's rewritten on every update to a leaf page.


It appears that maintaining 32-entry index is really expensive because it
required re-decoding whole page. This issue is fixed in attached version of
patch by introducing incremental updating of that index. Benchmarks will be
posted later today..


Great! Please also benchmark WAL replay; you're still doing 
non-incremental update of the 32-entry index in ginRedoInsert.


A couple of quick comments after a quick glance (in addition to the above):

The varbyte encoding stuff is a quite isolated piece of functionality. 
And potentially useful elsewhere, too. It would be good to separate that 
into a separate .c/.h files. I'm thinking of 
src/backend/access/gin/packeditemptr.c, which would contain 
ginDataPageLeafReadItemPointer, ginDataPageLeafWriteItemPointer and 
ginDataPageLeafGetItemPointerSize functions. A new typedef for 
varbyte-encoded things would probably be good too, something like 
typedef char *PackedItemPointer. In the new .c file, please also add a 
file-header comment explaining the encoding.


The README really needs to be updated. The posting tree page structure 
is a lot more complicated now, there needs to be a whole new section to 
explain it. A picture would be nice, similar to the one in bufpage.h.


It's a bit funny that we've clumped together all different kinds of GIN 
insertions into one WAL record type. ginRedoInsert does completely 
different things depending on what kind of a page it is. And the 
ginXlogInsert struct also contains different kinds of things depending 
on what kind of an insertion it is. It would be cleaner to split it into 
three. (this isn't your patch's fault - it was like that before, too.)


- Heikki


--
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] GIN improvements part 1: additional information

2013-09-16 Thread Alexander Korotkov

On Mon, Sep 16, 2013 at 11:43 AM, Heikki Linnakangas 
hlinnakan...@vmware.com wrote:

 On 15.09.2013 12:14, Alexander Korotkov wrote:

 On Sat, Jun 29, 2013 at 12:56 PM, Heikki Linnakangas
 hlinnakan...@vmware.com  wrote:

  There's a few open questions:

 1. How are we going to handle pg_upgrade? It would be nice to be able to
 read the old page format, or convert on-the-fly. OTOH, if it gets too
 complicated, might not be worth it. The indexes are much smaller with the
 patch, so anyone using GIN probably wants to rebuild them anyway, sooner
 or
 later. Still, I'd like to give it a shot.

 2. The patch introduces a small fixed 32-entry index into the packed
 items. Is that an optimal number?

 3. I'd like to see some performance testing of insertions, deletions, and
 vacuum. I suspect that maintaining the 32-entry index might be fairly
 expensive, as it's rewritten on every update to a leaf page.


 It appears that maintaining 32-entry index is really expensive because it
 required re-decoding whole page. This issue is fixed in attached version
 of
 patch by introducing incremental updating of that index. Benchmarks will
 be
 posted later today..


 Great! Please also benchmark WAL replay; you're still doing
 non-incremental update of the 32-entry index in ginRedoInsert.

 A couple of quick comments after a quick glance (in addition to the above):

 The varbyte encoding stuff is a quite isolated piece of functionality. And
 potentially useful elsewhere, too. It would be good to separate that into a
 separate .c/.h files. I'm thinking of 
 src/backend/access/gin/**packeditemptr.c,
 which would contain ginDataPageLeafReadItemPointer**,
 ginDataPageLeafWriteItemPointe**r and ginDataPageLeafGetItemPointerS**ize
 functions. A new typedef for varbyte-encoded things would probably be good
 too, something like typedef char *PackedItemPointer. In the new .c file,
 please also add a file-header comment explaining the encoding.

 The README really needs to be updated. The posting tree page structure is
 a lot more complicated now, there needs to be a whole new section to
 explain it. A picture would be nice, similar to the one in bufpage.h.

 It's a bit funny that we've clumped together all different kinds of GIN
 insertions into one WAL record type. ginRedoInsert does completely
 different things depending on what kind of a page it is. And the
 ginXlogInsert struct also contains different kinds of things depending on
 what kind of an insertion it is. It would be cleaner to split it into
 three. (this isn't your patch's fault - it was like that before, too.)


Apparently some bugs breaks index on huge updates independent on
incremental update of the 32-entry. I'm in debugging now. That's why
benchmarks are delayed.

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] patch: add MAP_HUGETLB to mmap() where supported (WIP)

2013-09-16 Thread Heikki Linnakangas


On 14.09.2013 02:41, Richard Poole wrote:

The attached patch adds the MAP_HUGETLB flag to mmap() for shared memory
on systems that support it. It's based on Christian Kruse's patch from
last year, incorporating suggestions from Andres Freund.


I don't understand the logic in figuring out the pagesize, and the 
smallest supported hugepage size. First of all, even without the patch, 
why do we round up the size passed to mmap() to the _SC_PAGE_SIZE? 
Surely the kernel will round up the request all by itself. The mmap() 
man page doesn't say anything about length having to be a multiple of 
pages size.


And with the patch, why do you bother detecting the minimum supported 
hugepage size? Surely the kernel will choose the appropriate hugepage 
size just fine on its own, no?



It is still WIP as there are a couple of points that Andres has pointed
out to me that haven't been addressed yet;


Which points are those?

I wonder if it would be better to allow setting huge_tlb_pages=try even 
on platforms that don't have hugepages. It would simply mean the same as 
'off' on such platforms.


- Heikki


--
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] UNNEST with multiple args, and TABLE with multiple funcs

2013-09-16 Thread Boszormenyi Zoltan


2013-09-13 21:03 keltezéssel, Andrew Gierth írta:

Latest version of patch, incorporating regression tests and docs, and
fixing the operator issue previously raised.


It looks good. I think it's ready for a committer.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Minmax indexes

2013-09-16 Thread Thom Brown

On 15 September 2013 01:14, Alvaro Herrera alvhe...@2ndquadrant.com wrote:

 Hi,

 Here's a reviewable version of what I've dubbed Minmax indexes.  Some
 people said they would like to use some other name for this feature, but
 I have yet to hear usable ideas, so for now I will keep calling them
 this way.  I'm open to proposals, but if you pick something that cannot
 be abbreviated mm I might have you prepare a rebased version which
 renames the files and structs.

 The implementation here has been simplified from what I originally
 proposed at 20130614222805.gz5...@eldon.alvh.no-ip.org -- in particular,
 I noticed that there's no need to involve aggregate functions at all; we
 can just use inequality operators.  So the pg_amproc entries are gone;
 only the pg_amop entries are necessary.

 I've somewhat punted on the question of doing resummarization separately
 from vacuuming.  Right now, resummarization (as well as other necessary
 index cleanup) takes place in amvacuumcleanup.  This is not optimal; I
 have stated elsewhere that I'd like to create separate maintenance
 actions that can be carried out by autovacuum.  That would be useful
 both for Minmax indexes and GIN indexes (pending insertion list); maybe
 others.  That's not part of this patch, however.

 The design of this stuff is in the file minmax-proposal at the top of
 the tree.  That file is up to date, though it still contains some open
 questions that were present in the original proposal.  (I have not fixed
 some bogosities pointed out by Noah, for instance.  I will do that
 shortly.)  In a final version, that file would be applied as
 src/backend/access/minmax/README, most likely.

 One area on which I needed to modify core code is IndexBuildHeapScan.  I
 needed a version that was able to scan only a certain range of pages,
 not the entire table, so I introduced a new IndexBuildHeapRangeScan, and
 added a quick heap_scansetlimits function.  I haven't tested that this
 works outside of the HeapRangeScan thingy, so it's probably completely
 bogus; I'm open to suggestions if people think this should be
 implemented differently.  In any case, keeping that implementation
 together with vanilla IndexBuildHeapScan makes a lot of sense.

 One thing still to tackle is when to mark ranges as unsummarized.  Right
 now, any new tuple on a page range would cause a new index entry to be
 created and a new revmap update.  This would cause huge index bloat if,
 say, a page is emptied and vacuumed and filled with new tuples with
 increasing values outside the original range; each new tuple would
 create a new index tuple.  I have two ideas about this (1. mark range as
 unsummarized if 3rd time we touch the same page range; 2. vacuum the
 affected index page if it's full, so we can maintain the index always up
 to date without causing unduly bloat), but I haven't implemented
 anything yet.

 The amcostestimate routine is completely bogus; right now it returns
 constant 0, meaning the index is always chosen if it exists.

 There are opclasses for int4, numeric and text.  The latter doesn't work
 at all, because collation info is not passed down at all.  I will have
 to figure that out (even if I find unlikely that minmax indexes have any
 usefulness on top of text columns).  I admit that numeric hasn't been
 tested, and it's quite likely that they won't work; mainly because of
 lack of some datumCopy() calls, about which the code contains some
 /* XXX */ lines.  I think this should be relatively straightforward.
 Ideally, the final version of this patch would contain opclasses for all
 supported datatypes (i.e. the same that have got btree opclasses).

 I have messed up the opclass information, as evidenced by failures in
 opr_sanity regression test.  I will research that later.


 There's working contrib/pageinspect support; pg_xlogdump (and wal_debug)
 seems to work sanely too.
 This patch compiles cleanly under -Werror.

   The research leading to these results has received funding from the
   European Union's Seventh Framework Programme (FP7/2007-2013) under
   grant agreement n° 318633


Thanks for the patch, but I seem to have immediately hit a snag:

pgbench=# CREATE INDEX minmaxtest ON pgbench_accounts USING minmax (aid);
PANIC:  invalid xlog record length 0

-- 
Thom

Re: [HACKERS] plpgsql.print_strict_params

2013-09-16 Thread Marko Tiikkaja


On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote:

I'm taking a look at this patch as part of the current commitfest [*],


Thanks!


However the sample function provided in the documentation throws a
runtime error due to a missing FROM-clause entry.


Ugh.  I'll look into fixing that.


* Does it follow SQL spec, or the community-agreed behavior?
SQL spec: n/a. I do note that it makes use of the # syntax
before the body of a PL/pgSQL function, which is currently only
used for #variable_conflict [*]. I can imagine this syntax might
be used for other purposes down the road, so it might be worth
keeping an eye on it before it becomes a hodge-podge of ad-hoc
options.


Agreed.  I have two patches like this on the commitfest and one more 
cooking, so if more than one of these make it into PG, we should 
probably discuss how the general mechanism should work and look like in 
the future.



* Have all the bases been covered?

This lineis in exec_get_query_params() and exec_get_dynquery_params():

 return (no parameters);

Presumably the message will escape translation and this line should be better
written as:
return _((no parameters));


Nice catch.  Will look into this.  Another option would be to simply 
omit the DETAIL line if there were no parameters.  At this very moment 
I'm thinking that might be a bit nicer.



Also, if the offending query parameter contains a single quote, the output
is not escaped:

postgres=# select get_userid(E'foo''');
ERROR:  query returned more than one row
DETAIL:  p1 = 'foo''
CONTEXT:  PL/pgSQL function get_userid(text) line 9 at SQL statement

Not sure if that's a real problem though.


Hmm..  I should probably look at what happens when the parameters to a 
prepared statement are currently logged and imitate that.



* Does it follow the project coding guidelines?
Yes.

The functions added in pl_exec.c - exec_get_query_params() and
exec_get_dynquery_params() do strike me as potentially misnamed,
as they don't actually execute anything but are more utility
functions for generating formatted output.

Maybe format_query_params() etc. would be better?


Agreed, they could use some renaming.


* Are the comments sufficient and accurate?
exec_get_query_params() and exec_get_dynquery_params()
could do with some brief comments explaining their purpose.


Agreed.


(It's the first time I've formally reviewed a patch for a commitfest
so please let me know if I'm missing something.)


I personally think you did an excellent job.  Huge thanks so far!


Regards,
Marko Tiikkaja


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

Re: [HACKERS] Re: [HACKERS] Is it necessary to rewrite table while increasing the scale of datatype numeric？

2013-09-16 Thread wangshuo


wangs...@highgo.com.cn wangs...@highgo.com.cn wrote:


I modified the code for this situation.I consider it very simple.



It will does not modify the table file, when the scale has been
increased exclusively.




Kevin Grittner kgri...@ymail.com wrote:

This patch would allow data in a column which was not consistent
with the column definition:

test=# create table n (val numeric(5,2));
CREATE TABLE
test=# insert into n values ('123.45');
INSERT 0 1
test=# select * from n;
  val  

 123.45
(1 row)

test=# alter table n alter column val type numeric(5,4);
ALTER TABLE
test=# select * from n;
  val  

 123.45
(1 row)

Without your patch the ALTER TABLE command gets this error (as it
should):

test=# alter table n alter column val type numeric(5,4);
ERROR:  numeric field overflow
DETAIL:  A field with precision 5, scale 4 must round to an absolute
value less than 10^1.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Thanks for your reply and test.
I had added a new function named ATNumericColumnChangeRequiresCheck to 
check the data

when the scale of numeric increase.
Now,the ALTER TABLE command could prompt this error:

postgres=# alter table tt alter  COLUMN t1 type numeric (5,4);
ERROR:  numeric field overflow
DETAIL:  A field with precision 5, scale 4 must round to an absolute 
value less than 10^1.

STATEMENT:  alter table tt alter  COLUMN t1 type numeric (5,4);
ERROR:  numeric field overflow
DETAIL:  A field with precision 5, scale 4 must round to an absolute 
value less than 10^1.


I packed a new patch about this modification.

I think this  ' altering field  type model ' could modify all the type 
in database.
Make different modification to column‘s datatype for different 
situation.
For example when you modify the scale of numeric, if you think that the 
5.0 and 5.00 is different,

the table file must be rewritten; otherwise, needn't be rewritten.


 Wang Shuo
 HighGo Software Co.,Ltd.
 September 16, 2013
diff -uNr b/src/backend/commands/tablecmds.c a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c	2013-08-31 17:11:00.529744869 +0800
+++ a/src/backend/commands/tablecmds.c	2013-09-16 16:33:49.527455560 +0800
@@ -367,7 +367,10 @@
 	  AlteredTableInfo *tab, Relation rel,
 	  bool recurse, bool recursing,
 	  AlterTableCmd *cmd, LOCKMODE lockmode);
-static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
+static void ATNumericColumnChangeRequiresCheck(AlteredTableInfo *tab);
+static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno,
+	int32 oldtypemod, int32 newtypemod,
+			AlteredTableInfo *tab);
 static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	  AlterTableCmd *cmd, LOCKMODE lockmode);
 static void ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -7480,7 +7483,7 @@
 		newval-expr = (Expr *) transform;
 
 		tab-newvals = lappend(tab-newvals, newval);
-		if (ATColumnChangeRequiresRewrite(transform, attnum))
+		if (ATColumnChangeRequiresRewrite(transform, attnum, attTup-atttypmod, targettypmod, tab))
 			tab-rewrite = true;
 	}
 	else if (transform)
@@ -7520,6 +7523,102 @@
 }
 
 /*
+ * check the data when the scale of numeric increase
+ */
+
+static void
+ATNumericColumnChangeRequiresCheck(AlteredTableInfo *tab)
+{
+	Relation	oldrel;
+	TupleDesc	oldTupDesc;
+	int			i;
+	ListCell   *l;
+	EState	   *estate;
+	ExprContext *econtext;
+	bool	   *isnull;
+	TupleTableSlot *oldslot;
+	HeapScanDesc scan;
+	HeapTuple	tuple;
+	Snapshot	snapshot;
+	List	   *dropped_attrs = NIL;
+	ListCell   *lc;
+	Datum	   *values;
+
+	/*
+	 * Open the relation(s).  We have surely already locked the existing
+	 * table.
+	 */
+
+	oldrel = heap_open(tab-relid, NoLock);
+	oldTupDesc = tab-oldDesc;
+
+	for (i = 0; i  oldTupDesc-natts; i++)
+	{
+		if (oldTupDesc-attrs[i]-attisdropped)
+			dropped_attrs = lappend_int(dropped_attrs, i);
+	}
+	/*
+	 * Generate the constraint and default execution states
+	 */
+
+	estate = CreateExecutorState();	
+
+	foreach(l, tab-newvals)
+	{
+		NewColumnValue *ex = lfirst(l);
+
+		/* expr already planned */
+		ex-exprstate = ExecInitExpr((Expr *) ex-expr, NULL);
+	}
+
+	econtext = GetPerTupleExprContext(estate);
+	oldslot = MakeSingleTupleTableSlot(oldTupDesc);
+	
+	/* Preallocate values/isnull arrays */
+	i = oldTupDesc-natts;
+	values = (Datum *) palloc(i * sizeof(Datum));
+	isnull = (bool *) palloc(i * sizeof(bool));
+	memset(values, 0, i * sizeof(Datum));
+	memset(isnull, true, i * sizeof(bool));
+
+	/*
+	 * Scan through the rows, generating a new row if needed and then
+	 * checking all the constraints.
+	 */
+	snapshot = RegisterSnapshot(GetLatestSnapshot());
+	scan = heap_beginscan(oldrel, snapshot, 0, NULL);
+	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+	{
+		foreach(lc, dropped_attrs)
+			isnull[lfirst_int(lc)] = true;
+
+		heap_deform_tuple(tuple, oldTupDesc, values,

Re: [HACKERS] Minmax indexes

2013-09-16 Thread Heikki Linnakangas


On 15.09.2013 03:14, Alvaro Herrera wrote:

+ Partial indexes are not supported; since an index is concerned with minimum 
and
+ maximum values of the involved columns across all the pages in the table, it
+ doesn't make sense to exclude values.  Another way to see partial indexes
+ here would be those that only considered some pages in the table instead of 
all
+ of them; but this would be difficult to implement and manage and, most likely,
+ pointless.


Something like this seems completely sensible to me:

create index i_accounts on accounts using minmax (ts) where valid = true;

The situation where that would be useful is if 'valid' accounts are 
fairly well clustered, but invalid ones are scattered all over the 
table. The minimum and maximum stoed in the index would only concern 
valid accounts.


- Heikki


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

Re: [HACKERS] patch: add MAP_HUGETLB to mmap() where supported (WIP)

2013-09-16 Thread Andres Freund

On 2013-09-16 11:15:28 +0300, Heikki Linnakangas wrote:
 On 14.09.2013 02:41, Richard Poole wrote:
 The attached patch adds the MAP_HUGETLB flag to mmap() for shared memory
 on systems that support it. It's based on Christian Kruse's patch from
 last year, incorporating suggestions from Andres Freund.
 
 I don't understand the logic in figuring out the pagesize, and the smallest
 supported hugepage size. First of all, even without the patch, why do we
 round up the size passed to mmap() to the _SC_PAGE_SIZE? Surely the kernel
 will round up the request all by itself. The mmap() man page doesn't say
 anything about length having to be a multiple of pages size.

I think it does:
   EINVAL We don't like addr, length, or offset (e.g., they are  too
  large,  or  not aligned on a page boundary).
and
   A file is mapped in multiples of the page size.  For a file that is not 
a multiple
   of  the  page size, the remaining memory is zeroed when mapped, and 
writes to that
   region are not written out to the file.  The effect of changing the  
size  of  the
   underlying  file  of  a  mapping  on the pages that correspond to added 
or removed
   regions of the file is unspecified.

And no, according to my past experience, the kernel does *not* do any
such rounding up. It will just fail.

 And with the patch, why do you bother detecting the minimum supported
 hugepage size? Surely the kernel will choose the appropriate hugepage size
 just fine on its own, no?

It will fail if it's not a multiple.

 It is still WIP as there are a couple of points that Andres has pointed
 out to me that haven't been addressed yet;
 
 Which points are those?

I don't know which point Richard already has fixed, so I'll let him
comment on that.

 I wonder if it would be better to allow setting huge_tlb_pages=try even on
 platforms that don't have hugepages. It would simply mean the same as 'off'
 on such platforms.

I wouldn't argue against that.

Greetings,

Andres Freund

-- 
 Andres Freund 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] Minmax indexes

2013-09-16 Thread Chris Travers



 On 16 September 2013 at 11:03 Heikki Linnakangas hlinnakan...@vmware.com
 wrote:


 Something like this seems completely sensible to me:

 create index i_accounts on accounts using minmax (ts) where valid = true;

 The situation where that would be useful is if 'valid' accounts are
 fairly well clustered, but invalid ones are scattered all over the
 table. The minimum and maximum stoed in the index would only concern
 valid accounts.

Here's one that occurs to me:

CREATE INDEX i_billing_id_mm ON billing(id) WHERE paid_in_full IS NOT TRUE;

Note that this would be a frequently moving target and over years of billing,
the subset would be quite small compared to the full system (imagine, say, 50k
rows out of 20M).

Best Wises,
Chris Travers

 - Heikki


 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers
Best Wishes,
Chris Travers
http://www.2ndquadrant.com
PostgreSQL Services, Training, and Support

Re: [HACKERS] git apply vs patch -p1

2013-09-16 Thread Andres Freund

On 2013-09-16 10:16:37 +0530, Ashutosh Bapat wrote:
 On Sun, Sep 15, 2013 at 12:38 AM, Andres Freund and...@2ndquadrant.comwrote:
 
  On 2013-09-14 15:03:52 -0400, Andrew Dunstan wrote:
  
   On 09/14/2013 02:37 PM, Josh Berkus wrote:
   Folks,
   
   Lately I've been running into a lot of reports of false conflicts
   reported by git apply.  The most recent one was the points patch,
   which git apply rejected for completely ficticious reasons (it claimed
   that the patch was trying to create a new file where a file already
   existed, which it wasn't).
   
   I think we should modify the patch review and developer instructions to
   recommend always using patch -p1 (or -p0, depending), even if the patch
   was produced with git diff.
   
   Thoughts?
   
  
  
   FWIW that's what I invariably use.
  
   You do have to be careful to git-add/git-rm any added/deleted files,
  which
   git-apply does for you (as well as renames) - I've been caught by that a
   couple of times.
 
  git reset?
 
 
 git reset wouldn't remove the files that were newly added by the patch,
 would it?

Depends on how you do it. I simply commit patches I look at - then they
can easily be removed using git reset --hard HEAD^. And it allows to
make further changes/annotations during review that are clearly
separated from the patch.

Greetings,

Andres Freund

-- 
 Andres Freund 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] Minmax indexes

2013-09-16 Thread Andres Freund

On 2013-09-16 11:19:19 +0100, Chris Travers wrote:
 
 
  On 16 September 2013 at 11:03 Heikki Linnakangas hlinnakan...@vmware.com
  wrote:
 
 
  Something like this seems completely sensible to me:
 
  create index i_accounts on accounts using minmax (ts) where valid = true;
 
  The situation where that would be useful is if 'valid' accounts are
  fairly well clustered, but invalid ones are scattered all over the
  table. The minimum and maximum stoed in the index would only concern
  valid accounts.

Yes, I wondered the same myself.

 Here's one that occurs to me:
 
 CREATE INDEX i_billing_id_mm ON billing(id) WHERE paid_in_full IS NOT TRUE;
 
 Note that this would be a frequently moving target and over years of billing,
 the subset would be quite small compared to the full system (imagine, say, 50k
 rows out of 20M).

In that case you'd just use a normal btree index, no?

Greetings,

Andres Freund

-- 
 Andres Freund 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] Not In Foreign Key Constraint

2013-09-16 Thread Misa Simic

Hi hackers,

I just wonder how hard would be to implement something like Not In FK
Constraint or opposite to FK...

i.e:

 FK ensures that value of FK column of inserted row exists in refferenced
Table

 NotInFK should ensure  that value of NotInFK column of inserted row does
not Exist in referenced Table...


The only difference/problem I see is that adding that constraint on an
Table - Forces the same Constraint on another table (but in opposite
direction)


i.e.

TableA(tableA_pk, other_columns)
TableB(tableb_fk_tableA_pk, other_columns)
TableC(tablec_notInfk_tableA_pk, other_column)


each _pk column is Primary Key of its Table
TableB has on PK FK to TableA on the same time...

INSERT INTO TableA VALUES ('tableAPK1', 'somedata')

INSERT INTO TableB VALUES ('tableAPK1'. 'somedata')

everything ok,


now, we would like to Add NotInFK on TableC To TableA

INSERT INTO TableC VALUES ('tableAPK1'. 'somedata')

Should Fail - because of 'tableAPK1' exists in TableA

INSERT INTO TableC VALUES ('tableAPK2'. 'somedata')

Should pass - because of 'tableAPK2'  does not exist in TableA...

How ever, now

INSERT INTO TableA VALUES ('tableAPK2'. 'somedata')

should fail as well - because of that value exists in TableC


I guess that rule can be achieved with triigers on TableA and TableC - but
the same is true for FK (and FK constraint is more effective then trigger -
that is why I wonder would it be useful/achievable to create that kind of
constraint)

Thoughts, ideas?

Many thanks,

Misa








* *

Re: [HACKERS] GUC for data checksums

2013-09-16 Thread Heikki Linnakangas


On 15.09.2013 17:05, Andres Freund wrote:

On 2013-09-15 03:34:53 +0200, Bernd Helmle wrote:



--On 15. September 2013 00:25:34 +0200 Andres Freund
and...@2ndquadrant.com  wrote:


Looks like a good idea to me. The implementation looks sane as well,
except that I am not sure if we really need to introduce that faux
variable. If the variable cannot be set and we have a SHOW hook, do we
need it?


It's along the line with the other informational variables like block_size
et al. Do you want to have a function instead or what's your intention?


Well, you've added a data_checksums variable that won't ever get used,
right? You can't set the variable and the show hook doesn't actually use
it.
The reason you presumably did so is that there is no plain variable that
contains information about data checksums, we first need to read the
control file to know whether it's enabled and GUCs are initialized way
earlier than that.

A quick look unfortunately shows that there's no support for GUCs
without an actual underlying variable, so unless somebody adds that,
there doesn't seem to be much choice.

I think a comment documenting that the data_checksums variable is not
actually used would be appropriate.


Surprisingly we don't have any other gucs that would be set at initdb 
time, and not changed after that. But we used to have two, lc_collate 
and lc_ctype, until we changed them to be per-database settings. We used 
to do this in ReadControlFile:



/* Make the fixed locale settings visible as GUC variables, too */
SetConfigOption(lc_collate, ControlFile-lc_collate,
PGC_INTERNAL, PGC_S_OVERRIDE);
SetConfigOption(lc_ctype, ControlFile-lc_ctype,
PGC_INTERNAL, PGC_S_OVERRIDE);


I did the same for data_checksums, and committed. Thanks for the patch.

- Heikki


--
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] GUC for data checksums

2013-09-16 Thread Andres Freund

On 2013-09-16 14:43:27 +0300, Heikki Linnakangas wrote:
 On 15.09.2013 17:05, Andres Freund wrote:
 On 2013-09-15 03:34:53 +0200, Bernd Helmle wrote:
 
 
 --On 15. September 2013 00:25:34 +0200 Andres Freund
 and...@2ndquadrant.com  wrote:
 
 Looks like a good idea to me. The implementation looks sane as well,
 except that I am not sure if we really need to introduce that faux
 variable. If the variable cannot be set and we have a SHOW hook, do we
 need it?
 
 It's along the line with the other informational variables like block_size
 et al. Do you want to have a function instead or what's your intention?
 
 Well, you've added a data_checksums variable that won't ever get used,
 right? You can't set the variable and the show hook doesn't actually use
 it.
 The reason you presumably did so is that there is no plain variable that
 contains information about data checksums, we first need to read the
 control file to know whether it's enabled and GUCs are initialized way
 earlier than that.
 
 A quick look unfortunately shows that there's no support for GUCs
 without an actual underlying variable, so unless somebody adds that,
 there doesn't seem to be much choice.
 
 I think a comment documenting that the data_checksums variable is not
 actually used would be appropriate.
 
 Surprisingly we don't have any other gucs that would be set at initdb time,
 and not changed after that. But we used to have two, lc_collate and
 lc_ctype, until we changed them to be per-database settings. We used to do
 this in ReadControlFile:
 
  /* Make the fixed locale settings visible as GUC variables, too */
  SetConfigOption(lc_collate, ControlFile-lc_collate,
  PGC_INTERNAL, PGC_S_OVERRIDE);
  SetConfigOption(lc_ctype, ControlFile-lc_ctype,
  PGC_INTERNAL, PGC_S_OVERRIDE);
 
 I did the same for data_checksums, and committed. Thanks for the patch.

I don't think it's fatal - as you say we've done so before - but note
that both the committed and Bernd's version don't report the correct
value on postgres -C data_checksums because we haven't read the control
file yet...
Maybe we should do that earlier?

Greetings,

Andres Freund

-- 
 Andres Freund 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] Fix picksplit with nan values

2013-09-16 Thread Andrew Gierth

 Alexander == Alexander Korotkov aekorot...@gmail.com writes:

 Alexander 2) NaN coordinates should be processed in GiST index scan
 Alexander like in sequential scan.

postgres=# select * from pts order by a - '(0,0)' limit 10;
a 
--
 (1,1)
 (7,nan)
 (9,nan)
 (11,nan)
 (4,nan)
 (nan,6)
 (2,1)
 (1,2)
 (2,2)
 (3,1)
(10 rows)

postgres=# set enable_indexscan=false;
SET

postgres=# select * from pts order by a - '(0,0)' limit 10;
   a   
---
 (1,1)
 (2,1)
 (1,2)
 (2,2)
 (3,1)
 (1,3)
 (3,2)
 (2,3)
 (4,1)
 (1,4)
(10 rows)

this data set was created by:
insert into pts
  select point(i,j)
from (select generate_series(1,100)::float8 union all select 'nan') s1(i),
 (select generate_series(1,100)::float8 union all select 'nan') s2(j)
   order by random();

-- 
Andrew (irc:RhodiumToad)


-- 
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] UTF8 national character data type support WIP patch and list of open issues.

2013-09-16 Thread MauMau


Hello,

I think it would be nice for PostgreSQL to support national character types 
largely because it should ease migration from other DBMSs.


[Reasons why we need NCHAR]
--
1. Invite users of other DBMSs to PostgreSQL.  Oracle, SQL Server, MySQL, 
etc. all have NCHAR support.  PostgreSQL is probably the only database out 
of major ones that does not support NCHAR.
Sadly, I've read a report from some Japanese government agency that the 
number of MySQL users exceeded that of PostgreSQL here in Japan in 2010 or 
2011.  I wouldn't say that is due to NCHAR support, but it might be one 
reason.  I want PostgreSQL to be more popular and regain those users.


2. Enhance the open image of PostgreSQL by implementing more features of 
SQL standard.  NCHAR may be a wrong and unnecessary feature of SQL standard 
now that we have Unicode support, but it is defined in the standard and 
widely implemented.


3. I have heard that some potential customers didn't adopt PostgreSQL due to 
lack of NCHAR support.  However, I don't know the exact reason why they need 
NCHAR.


4. I guess some users really want to continue to use ShiftJIS or EUC_JP for 
database encoding, and use NCHAR for a limited set of columns to store 
international text in Unicode:

- to avoid code conversion between the server and the client for performance
- because ShiftJIS and EUC_JP require less amount of storage (2 bytes for 
most Kanji) than UTF-8 (3 bytes)
This use case is described in chapter 6 of Oracle Database Globalization 
Support Guide.

--


I think we need to do the following:

[Minimum requirements]
--
1. Accept NCHAR/NVARCHAR as data type name and N'...' syntactically.
This is already implemented.  PostgreSQL treats NCHAR/NVARCHAR as synonyms 
for CHAR/VARCHAR, and ignores N prefix.  But this is not documented.


2. Declare support for national character support in the manual.
1 is not sufficient because users don't want to depend on undocumented 
behavior.  This is exactly what the TODO item national character support 
in PostgreSQL TODO wiki is about.


3. Implement NCHAR/NVARCHAR as distinct data types, not as synonyms so that:
- psql \d can display the user-specified data types.
- pg_dump/pg_dumpall can output NCHAR/NVARCHAR columns as-is, not as 
CHAR/VARCHAR.
- To implement additional features for NCHAR/NVARCHAR in the future, as 
described below.

--




[Optional requirements]
--
1. Implement client driver support, such as:
- NCHAR host variable type (e.g. NCHAR var_name[12];) in ECPG, as 
specified in the SQL standard.
- national character methods (e.g. setNString, getNString, 
setNCharacterStream) as specified in JDBC 4.0.
I think at first we can treat these national-character-specific features as 
the same as CHAR/VARCHAR.


2. NCHAR/NVARCHAR columns can be used in non-UTF-8 databases and always 
contain Unicode data.
I think it is sufficient at first that NCHAR/NVARCHAR columns can only be 
used in UTF-8 databases and they store UTF-8 strings.  This allows us to 
reuse the input/output/send/recv functions and other infrastructure of 
CHAR/VARCHAR.  This is a reasonable compromise to avoid duplication and 
minimize the first implementation of NCHAR support.


3. Store strings in UTF-16 encoding in NCHAR/NVARCHAR columns.
Fixed-width encoding may allow faster string manipulation as described in 
Oracle's manual.  But I'm not sure about this, because UTF-16 is not a real 
fixed-width encoding due to supplementary characters.

--


I don't think it is good to implement NCHAR/NVARCHAR types as extensions 
like contrib/citext, because NCHAR/NVARCHAR are basic types and need 
client-side support.  That is, client drivers need to be aware of the fixed 
NCHAR/NVARCHAR OID values.


How do you think we should implement NCHAR support?

Regards
MauMau



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

Re: [HACKERS] patch: add MAP_HUGETLB to mmap() where supported (WIP)

2013-09-16 Thread Heikki Linnakangas


On 16.09.2013 13:15, Andres Freund wrote:

On 2013-09-16 11:15:28 +0300, Heikki Linnakangas wrote:

On 14.09.2013 02:41, Richard Poole wrote:

The attached patch adds the MAP_HUGETLB flag to mmap() for shared memory
on systems that support it. It's based on Christian Kruse's patch from
last year, incorporating suggestions from Andres Freund.


I don't understand the logic in figuring out the pagesize, and the smallest
supported hugepage size. First of all, even without the patch, why do we
round up the size passed to mmap() to the _SC_PAGE_SIZE? Surely the kernel
will round up the request all by itself. The mmap() man page doesn't say
anything about length having to be a multiple of pages size.


I think it does:
EINVAL We don't like addr, length, or offset (e.g., they are  too
   large,  or  not aligned on a page boundary).


That doesn't mean that they *all* have to be aligned on a page boundary. 
It's understandable that 'addr' and 'offset' have to be, but it doesn't 
make much sense for 'length'.



and
A file is mapped in multiples of the page size.  For a file that is not 
a multiple
of  the  page size, the remaining memory is zeroed when mapped, and 
writes to that
region are not written out to the file.  The effect of changing the  
size  of  the
underlying  file  of  a  mapping  on the pages that correspond to added 
or removed
regions of the file is unspecified.

And no, according to my past experience, the kernel does *not* do any
such rounding up. It will just fail.


I wrote a little test program to play with different values (attached). 
I tried this on my laptop with a 3.2 kernel (uname -r: 3.10-2-amd6), and 
on a VM with a fresh Centos 6.4 install with 2.6.32 kernel 
(2.6.32-358.18.1.el6.x86_64), and they both work the same:


$ ./mmaptest 100 # mmap 100 bytes

in a different terminal:
$ cat /proc/meminfo  | grep HugePages_Rsvd
HugePages_Rsvd:1

So even a tiny allocation, much smaller than any page size, succeeds, 
and it reserves a huge page. I tried the same with larger values; the 
kernel always uses huge pages, and rounds up the allocation to a 
multiple of the huge page size.


So, let's just get rid of the /sys scanning code.

Robert, do you remember why you put the pagesize = 
sysconf(_SC_PAGE_SIZE); call in the new mmap() shared memory allocator?


- Heikki
#include sys/mman.h
#include stdio.h
#include errno.h
#include string.h

int main(int argc, char **argv)
{
	char *ptr;
	int size;

	size = (argc  1) ? atoi(argv[1]) : (100 * 4096);

	ptr = mmap(NULL, size, PROT_READ | PROT_WRITE,
			   MAP_SHARED | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);

	if (ptr != (void *) -1)
		printf(success: %p\n, ptr);
	else
		printf(failure: %s\n, strerror(errno));

	sleep(10);

	return 0;
}

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

Re: [HACKERS] patch: add MAP_HUGETLB to mmap() where supported (WIP)

2013-09-16 Thread Andres Freund

On 2013-09-16 16:13:57 +0300, Heikki Linnakangas wrote:
 On 16.09.2013 13:15, Andres Freund wrote:
 On 2013-09-16 11:15:28 +0300, Heikki Linnakangas wrote:
 On 14.09.2013 02:41, Richard Poole wrote:
 The attached patch adds the MAP_HUGETLB flag to mmap() for shared memory
 on systems that support it. It's based on Christian Kruse's patch from
 last year, incorporating suggestions from Andres Freund.
 
 I don't understand the logic in figuring out the pagesize, and the smallest
 supported hugepage size. First of all, even without the patch, why do we
 round up the size passed to mmap() to the _SC_PAGE_SIZE? Surely the kernel
 will round up the request all by itself. The mmap() man page doesn't say
 anything about length having to be a multiple of pages size.
 
 I think it does:
 EINVAL We don't like addr, length, or offset (e.g., they are  too
large,  or  not aligned on a page boundary).
 
 That doesn't mean that they *all* have to be aligned on a page boundary.
 It's understandable that 'addr' and 'offset' have to be, but it doesn't make
 much sense for 'length'.
 
 and
 A file is mapped in multiples of the page size.  For a file that is 
  not a multiple
 of  the  page size, the remaining memory is zeroed when mapped, and 
  writes to that
 region are not written out to the file.  The effect of changing the  
  size  of  the
 underlying  file  of  a  mapping  on the pages that correspond to 
  added or removed
 regions of the file is unspecified.
 
 And no, according to my past experience, the kernel does *not* do any
 such rounding up. It will just fail.
 
 I wrote a little test program to play with different values (attached). I
 tried this on my laptop with a 3.2 kernel (uname -r: 3.10-2-amd6), and on a
 VM with a fresh Centos 6.4 install with 2.6.32 kernel
 (2.6.32-358.18.1.el6.x86_64), and they both work the same:
 
 $ ./mmaptest 100 # mmap 100 bytes
 
 in a different terminal:
 $ cat /proc/meminfo  | grep HugePages_Rsvd
 HugePages_Rsvd:1
 
 So even a tiny allocation, much smaller than any page size, succeeds, and it
 reserves a huge page. I tried the same with larger values; the kernel always
 uses huge pages, and rounds up the allocation to a multiple of the huge page
 size.

When developing the prototype I am pretty sure I had to add the rounding
up - but I am not sure why now, because after chatting with Heikki about
it, I've looked around and the initial MAP_HUGETLB support in the kernel
(commit 4e52780d41a741fb4861ae1df2413dd816ec11b1) has support for
rounding up.

 So, let's just get rid of the /sys scanning code.

Alternatively we could round up NBuffers to actually use the
additionally allocated space. Not sure if that's worth the amount of
code, but wasting several megabytes - or even gigabytes - of memory
isn't nice either.

Greetings,

Andres Freund

-- 
 Andres Freund 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: add MAP_HUGETLB to mmap() where supported (WIP)

2013-09-16 Thread Andres Freund

On 2013-09-16 15:18:50 +0200, Andres Freund wrote:
  So even a tiny allocation, much smaller than any page size, succeeds, and it
  reserves a huge page. I tried the same with larger values; the kernel always
  uses huge pages, and rounds up the allocation to a multiple of the huge page
  size.
 
 When developing the prototype I am pretty sure I had to add the rounding
 up - but I am not sure why now, because after chatting with Heikki about
 it, I've looked around and the initial MAP_HUGETLB support in the kernel
 (commit 4e52780d41a741fb4861ae1df2413dd816ec11b1) has support for
 rounding up.

Ok, the reason for that seems to have been the following bug
https://bugzilla.kernel.org/show_bug.cgi?id=56881

Greetings,

Andres Freund


-- 
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: json_populate_record and nested json objects

2013-09-16 Thread Merlin Moncure

On Sat, Sep 14, 2013 at 9:27 PM, chris travers ch...@2ndquadrant.com wrote:
 Hi all;

 Currently json_populate_record and json_populate_recordset cannot work with
 nested json objects.  This creates two fundamental problems when trying to
 use JSON as an interface format.

 The first problem is you can't easily embed a json data type in an json
 object and have it populate a record.  This means that storing extended
 attributes in the database is somewhat problematic if you accept the whole
 row in as a json object.

 The second problem is that nested data structures and json don't go together
 well.  You can't have  a composite type which has as an attribute an array
 of another composite type and populate this from a json object.  This makes
 json largely an alternative to hstore for interfaces in its current shape.

 I would propose handling the json_populate_record and friends as such:

 1. Don't throw errors initially as a pre-check if the json object is nested.
 2. If one comes to a nested fragment, check the attribute type it is going
 into first.
 2.1 If it is a json type, put the nested fragment there.
 2.2 If it is a composite type (i.e. anything in pg_class), push it
 through another json_populate_record run
 2.3 If it is neither, then see if a json::[type] cast exists, if so call
 it.
 2.4 Otherwise raise an exception

 I have a few questions before I go on to look at creating a patch.

 1.  Are there any problems anyone spots with this approach?

 2.  Is anyone working on something like this?

 3.  Would it be preferable to build something like this first as an
 extension (perhaps with different function names) or first as a patch?

Huge +1 on on this.  Couple random thoughts:

*) Hard to see how you would structure this as an extension as you're
adjusting the behaviors of existing functions, unless you wanted to
introduce new function names for testing purposes?

*) Would like to at least consider being able to use casting syntax as
a replacement for populate_record and (the missing) populate_array
for most usages.

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] Assertions in PL/PgSQL

2013-09-16 Thread Peter Eisentraut

On 9/15/13 10:49 AM, Marko Tiikkaja wrote:
 On 2013-09-15 16:34, Peter Eisentraut wrote:
 On Sat, 2013-09-14 at 20:47 +0200, Marko Tiikkaja wrote:
 Attached is a patch for supporting assertions in PL/PgSQL.  These are
 similar to the Assert() backend macro: they can be disabled during
 compile time, but when enabled, abort execution if the passed expression
 is not true.

 Doesn't build:
 
 Ugh.  Accidentally edited an auto-generated file.  Fixed in the
 attached, thanks!

Please fix the tabs in the SGML files.



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

Proposal: UPDATE/DELETE ... WHERE OFFSET n OF cursor_name, was: Re: New ECPG idea, was: Re: [HACKERS] ECPG FETCH readahead

2013-09-16 Thread Boszormenyi Zoltan


Hi,

2013-08-17 13:02 keltezéssel, Boszormenyi Zoltan írta:
[snip, discussion of WHERE CURRENT OF in the ECPG client lib]

I had a second thought about it and the client side caching and
parser behind the application's back seems to be an overkill.

Instead, I propose a different solution, which is a logical extension of
FETCH { FORWARD | BACKWARD } N, which is a PostgreSQL extension.

The proposed solution would be:

UPDATE / DELETE ... WHERE OFFSET SignedIconst OF cursor_name

I imagine that FETCH would keep the array of TIDs/ItemPointerDatas
of the last FETCH statement.

The argument to OFFSET would be mostly in negative terms,
with 0 being equivalent of WHERE CURRENT OF.

E.g.:

FETCH 2 FROM mycur; -- fetches two rows
UPDATE mytab SET ... WHERE OFFSET -1 OF mycur; -- updates the first row
UPDATE mytab SET ... WHERE OFFSET 0 OF mycur; -- updates current row

or

FETCH 3 FROM mycur; -- fetches two rows, reaches end of the cursor
UPDATE mytab SET ... WHERE OFFSET -2 OF mycur; -- updates the first row
UPDATE mytab SET ... WHERE OFFSET -1 OF mycur; -- updates the second row
UPDATE mytab SET ... WHERE OFFSET 0 OF mycur; -- throws an error like WHERE 
CURRENT OF

or

FETCH 3 FROM mycur; -- fetches two rows, reaches end of the cursor
MOVE BACKWARD 2 IN mycur;
UPDATE mytab SET ... WHERE OFFSET 0 OF mycur; -- updates the first row (now 
current)
UPDATE mytab SET ... WHERE OFFSET 1 OF mycur; -- updates the second row

The cached array can be kept valid until the next FETCH statement,
even if  moves out of the interval of the array, except in case the
application changes the sign of the cursor position, e.g. previously it used
MOVE ABSOLUTE with positive numbers and suddenly it switches to
backward scanning with MOVE ABSOLUTE negative number or vice-versa.

This would solve the only source of slowdown in the client side cursor caching
in ECPG present in my current ECPG cursor readahead patch, there would be
no more MOVE + UPDATE/DELETE WHERE CURRENT OF. On the other hand,
exploiting this proposed feature in ECPG would make it incompatible with older
servers unless it detects the server version it connects to and uses the current
method.

Comments?

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] insert throw error when year field len 4 for timestamptz datatype

2013-09-16 Thread Haribabu kommi

On 14 August 2013 Rushabh Lathia wrote:

postgres=# create table test ( a timestamptz);
CREATE TABLE

-- Date with year 1000
postgres=#  insert into test values ( 'Sat Mar 11 23:58:48 1000 IST');
INSERT 0 1

-- Now try with year 1 it will return error
postgres=#  insert into test values ( 'Sat Mar 11 23:58:48 1 IST');
ERROR:  invalid input syntax for type timestamp with time zone: Sat Mar 11 
23:58:48 1 IST
LINE 1: insert into test values ( 'Sat Mar 11 23:58:48 1 IST');

here error coming from timestamptz_in() - datefields_to_timestamp() -
DecodeDateTime() stack.

Looking more at the DecodeDateTime() function, here error coming while trying
to Decode year field which is 1 in the our test. For year field ftype is
DTK_NUMBER, and under DTK_NUMBER for this case if drop in to following 
condition:

else if (flen  4)
{
dterr = DecodeNumberField(flen, field[i], fmask,
 tmask, tm,
 fsec, is2digits);
if (dterr  0)
return dterr;
}

because flen in out case flen is 5 (1).

As per the comment above DecodeNumberField(), it interpret numeric string as a
concatenated date or time field. So ideally we should be into DecodeNumberField
function only with (fmask  DTK_DATE_M) == 0 or (fmask  DTK_TIME_M) == 0,
right ??

So, I tried the same and after that test working fine.

PFA patch and share your input/suggestions.

Patch applies cleanly to HEAD. As this patch tries to improve in inserting the 
date of the year value to be more than 4 in length.
But it didn't solve all the ways to insert the year field more than 4 in 
length. Please check the following test.


postgres=# insert into test values ('10001010 10:10:10 IST');
INSERT 0 1
postgres=# insert into test values ('100011010 10:10:10 IST');
ERROR:  invalid input syntax for type timestamp with time zone: 100011010 
10:10:10 IST at character 26
STATEMENT:  insert into test values ('100011010 10:10:10 IST');
ERROR:  invalid input syntax for type timestamp with time zone: 100011010 
10:10:10 IST
LINE 1: insert into test values ('100011010 10:10:10 IST');
^

I feel it is better to provide the functionality of inserting year field more 
than 4 in length in all flows.

Regards,
Hari babu.

Re: [HACKERS] Proposal: json_populate_record and nested json objects

2013-09-16 Thread Chris Travers



 On 16 September 2013 at 14:43 Merlin Moncure mmonc...@gmail.com wrote:


 Huge +1 on on this. Couple random thoughts:

 *) Hard to see how you would structure this as an extension as you're
 adjusting the behaviors of existing functions, unless you wanted to
 introduce new function names for testing purposes?

Yeah, and reading the source, it looks like some parts of the JSON parsing code
will have to be rewritten because the nested object errors are thrown quite
deeply in the parsing stage.  It looks to me as if this will require some
significant copying as a POC into a new file with different publicly exposed
function names.

 *) Would like to at least consider being able to use casting syntax as
 a replacement for populate_record and (the missing) populate_array
 for most usages.

Yes.  I am trying to figure out how best to do this at present.  Initially I
think I would be happy to settle for casts wrapping functions which themselves
just wrap the call to populate_record.

What I will probably do for my POC is expose the following methods:

1.  json_populate_type()
2.  json_populate_array()

Then we can talk about whether to merge the changes into the core,

This will be an interesting way to get into PostgreSQL hacking.

Best Wishes,
Chris Travers


 merlin


 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers
Best Wishes,
Chris Travers
http://www.2ndquadrant.com
PostgreSQL Services, Training, and Support

Re: [HACKERS] Freezing without write I/O

2013-09-16 Thread Heikki Linnakangas


On 27.08.2013 19:37, Heikki Linnakangas wrote:

On 27.08.2013 18:56, Heikki Linnakangas wrote:

Here's an updated patch.


Ah, forgot one thing:

Here's a little extension I've been using to test this. It contains two
functions; one to simply consume N xids, making it faster to hit the
creation of new XID ranges and wraparound. The other,
print_xidlsnranges(), prints out the contents of the current XID-LSN
range array.

Also, just ran into two silly bugs in the patch version I posted, while
checking that that xidtest extension hasn't bitrotted. A fixed version
has been pushed to the git repository, so make sure you use that version
if you want to actually run it.


Here's a rebased version of the patch, including the above-mentioned 
fixes. Nothing else new.


- Heikki

--
- Heikki


xid-lsn-ranges-3.patch.gz
Description: GNU Zip compressed data

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

77 matches

Mail list logo