from:"Boszormenyi Zoltan"


2013-11-12 07:15 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:01 keltezéssel, Noah Misch írta:

On Mon, Nov 11, 2013 at 10:17:54AM +0100, Boszormenyi Zoltan wrote:

The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude.

Patches of that weight show up on a regular basis.  I don't think it's rude.


OK, here it is.


I have rebased the patchset after ecpg: Split off mmfatal() from mmerror()
since it caused merge conflicts.

It's at the usual place again, you need to clone it from scratch if you are
interested in looking at git diff/log

I have removed some previous ecpg_log() debug output and
the total patch size is not so huge any more but I am going to post
the split-up set in parts.

Attached is the first few patches that are strictly generic ECPG fixes.
They can be applied independently and obvious enough.

Subsequent patches will come as reply to this email.

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/

commit f167aaa9693305e08cd6b2946af8528dada799b4
Author: BÃ¶szÃ¶rmÃ©nyi ZoltÃ¡n z...@cybertec.at
Date:   Wed Nov 20 10:31:21 2013 +0100

ECPG: Make the preprocessor emit ';' if the variable type for
a list of variables is varchar. This fixes this test case:

int main(void)
{
exec sql begin declare section;
varchar a[50], b[50];
exec sql end declare section;

return 0;
}

Since varchars are internally turned into custom structs and
the type name is emitted for these variable declarations,
the preprocessed code previously had:

struct varchar_1  { ... }  a _,_  struct varchar_2  { ... }  b ;

The comma in the generated C file was a syntax error.

There are no regression test changes since it's not exercised.

diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer
index 342b7bc..fd35dfc 100644
--- a/src/interfaces/ecpg/preproc/ecpg.trailer
+++ b/src/interfaces/ecpg/preproc/ecpg.trailer
@@ -837,7 +837,12 @@ opt_signed: SQL_SIGNED
 variable_list: variable
 			{ $$ = $1; }
 		| variable_list ',' variable
-			{ $$ = cat_str(3, $1, mm_strdup(,), $3); }
+		{
+			if (actual_type[struct_level].type_enum == ECPGt_varchar)
+$$ = cat_str(3, $1, mm_strdup(;), $3);
+			else
+$$ = cat_str(3, $1, mm_strdup(,), $3);
+		}
 		;
 
 variable: opt_pointer ECPGColLabel opt_array_bounds opt_bit_field opt_initializer
commit b6d57b3fa709757769eb27083d7231602f2d806c
Author: BÃ¶szÃ¶rmÃ©nyi ZoltÃ¡n z...@cybertec.at
Date:   Wed Nov 20 10:33:40 2013 +0100

ECPG: Free the malloc()'ed variables in the test so it comes out
clean on Valgrind runs.

diff --git a/src/interfaces/ecpg/test/expected/preproc-outofscope.c b/src/interfaces/ecpg/test/expected/preproc-outofscope.c
index 125d7d8..2438911 100644
--- a/src/interfaces/ecpg/test/expected/preproc-outofscope.c
+++ b/src/interfaces/ecpg/test/expected/preproc-outofscope.c
@@ -347,28 +347,31 @@ if (sqlca.sqlcode  0) exit (1);}
 
 	close_cur1();
 
+	free(myvar);
+	free(mynullvar);
+
 	strcpy(msg, drop);
 	{ ECPGdo(__LINE__, 0, 1, NULL, 0, ECPGst_normal, drop table a1, ECPGt_EOIT, ECPGt_EORT);
-#line 115 outofscope.pgc
+#line 118 outofscope.pgc
 
 if (sqlca.sqlcode  0) exit (1);}
-#line 115 outofscope.pgc
+#line 118 outofscope.pgc
 
 
 	strcpy(msg, commit);
 	{ ECPGtrans(__LINE__, NULL, commit);
-#line 118 outofscope.pgc
+#line 121 outofscope.pgc
 
 if (sqlca.sqlcode  0) exit (1);}
-#line 118 outofscope.pgc
+#line 121 outofscope.pgc
 
 
 	strcpy(msg, disconnect);
 	{ ECPGdisconnect(__LINE__, CURRENT);
-#line 121 outofscope.pgc
+#line 124 outofscope.pgc
 
 if (sqlca.sqlcode  0) exit (1);}
-#line 121 outofscope.pgc
+#line 124 outofscope.pgc
 
 
 	return (0);
diff --git a/src/interfaces/ecpg/test/expected/preproc-outofscope.stderr b/src/interfaces/ecpg/test/expected/preproc-outofscope.stderr
index 91d3505..c7f8771 100644
--- a/src/interfaces/ecpg/test/expected/preproc-outofscope.stderr
+++ b/src/interfaces/ecpg/test/expected/preproc-outofscope.stderr
@@ -102,13 +102,13 @@
 [NO_PID]: sqlca: code: 0, state: 0
 [NO_PID]: ecpg_execute on line 58: OK: CLOSE CURSOR
 [NO_PID]: sqlca: code: 0, state: 0
-[NO_PID]: ecpg_execute on line 115: query: drop table a1; with 0 parameter(s) on connection regress1
+[NO_PID]: ecpg_execute on line 118: query: drop table a1; with 0 parameter(s) on connection regress1
 [NO_PID]: sqlca: code: 0, state: 0
-[NO_PID]: ecpg_execute on line 115: using PQexec
+[NO_PID]: ecpg_execute on line 118: using PQexec
 [NO_PID]: sqlca: code: 0, state: 0
-[NO_PID]: ecpg_execute on line 115: OK: DROP TABLE
+[NO_PID]: ecpg_execute on line

[HACKERS] Modify the DECLARE CURSOR command tag depending on the scrollable flag


2013-11-20 14:41 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:15 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:01 keltezéssel, Noah Misch írta:

On Mon, Nov 11, 2013 at 10:17:54AM +0100, Boszormenyi Zoltan wrote:

The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude.

Patches of that weight show up on a regular basis.  I don't think it's rude.


OK, here it is.


...
Subsequent patches will come as reply to this email.


Attached is the patch that modified the command tag returned by
the DECLARE CURSOR command. It returns DECLARE SCROLL CURSOR
or DECLARE NO SCROLL CURSOR depending on the cursor's
scrollable flag that can be determined internally even if neither is
asked explicitly.

It is expected by the ECPG cursor readahead code.

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/

commit a691e262f562debd4b8991728fddd1e0895cf907
Author: BÃ¶szÃ¶rmÃ©nyi ZoltÃ¡n z...@cybertec.at
Date:   Wed Nov 20 10:50:31 2013 +0100

The backend knows whether the cursor is scrollable if neither SCROLL
nor NO SCROLL was specified. Inform the clients about this property
in the command tag: DECLARE SCROLL CURSOR or DECLARE NO SCROLL CURSOR.
The upcoming ECPG cursor handling code uses it. One contrib and some
ECPG regression tests have changed.

diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index f237c43..d2b5d69 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -442,9 +442,9 @@ SELECT dblink_close('myconn','rmt_foo_cursor');
 
 -- this should succeed because we have an open transaction
 SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo');
-  dblink_exec   
-
- DECLARE CURSOR
+  dblink_exec  
+---
+ DECLARE SCROLL CURSOR
 (1 row)
 
 -- commit remote transaction
@@ -477,9 +477,9 @@ SELECT dblink_close('myconn','rmt_foo_cursor2');
 
 -- this should succeed because we have an open transaction
 SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo');
-  dblink_exec   
-
- DECLARE CURSOR
+  dblink_exec  
+---
+ DECLARE SCROLL CURSOR
 (1 row)
 
 -- this should commit the transaction
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 5c3f42c..ef36d78 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -42,7 +42,8 @@
  */
 void
 PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
-  const char *queryString, bool isTopLevel)
+  const char *queryString, bool isTopLevel,
+  bool *scrollable)
 {
 	DeclareCursorStmt *cstmt = (DeclareCursorStmt *) stmt-utilityStmt;
 	Portal		portal;
@@ -118,6 +119,8 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
 			portal-cursorOptions |= CURSOR_OPT_NO_SCROLL;
 	}
 
+	*scrollable = !!(portal-cursorOptions  CURSOR_OPT_SCROLL);
+
 	/*
 	 * Start execution, inserting parameters if any.
 	 */
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6a7bf0d..89665cc 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -505,11 +505,15 @@ standard_ProcessUtility(Node *parsetree,
 		case T_PlannedStmt:
 			{
 PlannedStmt *stmt = (PlannedStmt *) parsetree;
+bool		scrollable;
 
 if (stmt-utilityStmt == NULL ||
 	!IsA(stmt-utilityStmt, DeclareCursorStmt))
 	elog(ERROR, non-DECLARE CURSOR PlannedStmt passed to ProcessUtility);
-PerformCursorOpen(stmt, params, queryString, isTopLevel);
+PerformCursorOpen(stmt, params, queryString, isTopLevel, scrollable);
+if (completionTag)
+	sprintf(completionTag, DECLARE %s CURSOR,
+			   (scrollable ? SCROLL : NO SCROLL));
 			}
 			break;
 
diff --git a/src/include/commands/portalcmds.h b/src/include/commands/portalcmds.h
index b123bbd..8e756a8 100644
--- a/src/include/commands/portalcmds.h
+++ b/src/include/commands/portalcmds.h
@@ -19,7 +19,8 @@
 
 
 extern void PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
-  const char *queryString, bool isTopLevel);
+  const char *queryString, bool isTopLevel,
+  bool *scrollable);
 
 extern void PerformPortalFetch(FetchStmt *stmt, DestReceiver *dest,
    char *completionTag);
diff --git a/src/interfaces/ecpg/test/expected/compat_informix-sqlda.stderr b/src/interfaces/ecpg/test/expected/compat_informix-sqlda.stderr
index f463d03..fc36a14 100644
--- a/src/interfaces/ecpg/test/expected/compat_informix-sqlda.stderr
+++ b/src/interfaces/ecpg/test/expected/compat_informix-sqlda.stderr
@@ -28,7 +28,7 @@
 [NO_PID

[HACKERS] ECPG infrastructure changes, part 2, was: Re: ECPG fixes


2013-11-20 14:41 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:15 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:01 keltezéssel, Noah Misch írta:

On Mon, Nov 11, 2013 at 10:17:54AM +0100, Boszormenyi Zoltan wrote:

The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude.

Patches of that weight show up on a regular basis.  I don't think it's rude.


OK, here it is.


...
Subsequent patches will come as reply to this email.


ecpg_log() fixes after part 1 that produces a lot of regression test changes.
This patch is over 200K in itself so I send it separately and compressed.

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/



17.patch.gz
Description: Unix tar archive

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

[HACKERS] ECPG infrastructure changes, part 3, was: Re: ECPG fixes


2013-11-20 14:41 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:15 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:01 keltezéssel, Noah Misch írta:

On Mon, Nov 11, 2013 at 10:17:54AM +0100, Boszormenyi Zoltan wrote:

The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude.

Patches of that weight show up on a regular basis.  I don't think it's rude.


OK, here it is.


...
Subsequent patches will come as reply to this email.


Further ecpglib/execute.c changes.

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/

commit c901afd59894f49001b743982d38a51b23bac8e1
Author: BÃ¶szÃ¶rmÃ©nyi ZoltÃ¡n z...@cybertec.at
Date:   Wed Nov 20 11:05:34 2013 +0100

ECPG: Explicitly decouple the tuple index from the array index
in ecpg_get_data(). Document the function arguments.

diff --git a/src/interfaces/ecpg/ecpglib/data.c b/src/interfaces/ecpg/ecpglib/data.c
index 5f9a3d4..7f3c7cb 100644
--- a/src/interfaces/ecpg/ecpglib/data.c
+++ b/src/interfaces/ecpg/ecpglib/data.c
@@ -119,8 +119,35 @@ check_special_value(char *ptr, double *retval, char **endptr)
 	return false;
 }
 
+/*
+ * ecpg_get_data
+ *   Store one field data from PQgetvalue(results, act_tuple, act_field)
+ *   into a target variable. If the field is NULL, store the indication or
+ *   emit an error about the fact that there is no NULL indicator given.
+ * Parameters:
+ *   results: result set
+ *   act_tuple:   row index in the result set
+ *   act_field:   column index in the result set
+ *   var_index:   array index in the target variable
+ *   lineno:  line number in the ECPG source file for debugging
+ *   type:type of target variable
+ *   ind_type:type of NULL indicator variable
+ *   var: target variable
+ *   ind: NULL indicator variable
+ *   varcharsize: size of the variable if it's varchar
+ *   offset:  size of the target variable
+ *(used for indexing in an array)
+ *   ind_offset:  size of the NULL indicator variable
+ *(used for indexing in an array)
+ *   isarray: array type
+ *   compat:  native PostgreSQL or Informix compatibility mode
+ *   force_indicator:
+ *if Informix compatibility mode is set and no NULL indicator
+ *is given, provide a way to indicate NULL value in the
+ *target variable itself
+ */
 bool
-ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno,
+ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int var_index, int lineno,
 			  enum ECPGttype type, enum ECPGttype ind_type,
 			  char *var, char *ind, long varcharsize, long offset,
 			  long ind_offset, enum ARRAY_TYPE isarray, enum COMPAT_MODE compat, bool force_indicator)
@@ -167,20 +194,20 @@ ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno,
 	{
 		case ECPGt_short:
 		case ECPGt_unsigned_short:
-			*((short *) (ind + ind_offset * act_tuple)) = value_for_indicator;
+			*((short *) (ind + ind_offset * var_index)) = value_for_indicator;
 			break;
 		case ECPGt_int:
 		case ECPGt_unsigned_int:
-			*((int *) (ind + ind_offset * act_tuple)) = value_for_indicator;
+			*((int *) (ind + ind_offset * var_index)) = value_for_indicator;
 			break;
 		case ECPGt_long:
 		case ECPGt_unsigned_long:
-			*((long *) (ind + ind_offset * act_tuple)) = value_for_indicator;
+			*((long *) (ind + ind_offset * var_index)) = value_for_indicator;
 			break;
 #ifdef HAVE_LONG_LONG_INT
 		case ECPGt_long_long:
 		case ECPGt_unsigned_long_long:
-			*((long long int *) (ind + ind_offset * act_tuple)) = value_for_indicator;
+			*((long long int *) (ind + ind_offset * var_index)) = value_for_indicator;
 			break;
 #endif   /* HAVE_LONG_LONG_INT */
 		case ECPGt_NO_INDICATOR:
@@ -192,7 +219,7 @@ ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno,
 	 * Informix has an additional way to specify NULLs note
 	 * that this uses special values to denote NULL
 	 */
-	ECPGset_noind_null(type, var + offset * act_tuple);
+	ECPGset_noind_null(type, var + offset * var_index);
 }
 else
 {
@@ -243,10 +270,10 @@ ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno,
 		if (binary)
 		{
 			if (varcharsize == 0 || varcharsize * offset = size)
-memcpy(var + offset * act_tuple, pval, size);
+memcpy(var + offset * var_index, pval, size);
 			else
 			{
-memcpy(var + offset * act_tuple, pval, varcharsize * offset);
+memcpy(var + offset * var_index, pval, varcharsize * offset);
 
 if (varcharsize * offset  size)
 {
@@ -255,20

[HACKERS] ECPG infrastructure changes, part 4, was: Re: ECPG fixes


2013-11-20 14:41 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:15 keltezéssel, Boszormenyi Zoltan írta:

2013-11-12 07:01 keltezéssel, Noah Misch írta:

On Mon, Nov 11, 2013 at 10:17:54AM +0100, Boszormenyi Zoltan wrote:

The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude.

Patches of that weight show up on a regular basis.  I don't think it's rude.


OK, here it is.


...
Subsequent patches will come as reply to this email.


This is another, semi independent subfeature of ECPG readahead.
It's about 150K by itself, so I send it compressed.

The purpose of this patch is to track (sub-)transactions and cursors
in ecpglib to reduce network turnaround and speed up the application
in certain cases. E.g. cursors are discarded upon ROLLBACK TO
SAVEPOINT and ecpglib needs to know about it. When an unknown
savepoint or cursor name is sent, ecpglib would not send the command
to the server in an open transaction after this patch. Instead, it flips
a client-side error flag and returns the same error the backend
would in this case.

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/



23.patch.gz
Description: Unix tar archive

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

Re: [HACKERS] ECPG FETCH readahead

2013-11-11 Thread Boszormenyi Zoltan


2013-10-11 00:16 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan escribió:

2013-09-10 03:04 keltezéssel, Peter Eisentraut írta:

You need to update the dblink regression tests.

Done.

Dude, this is an humongous patch.  I was shocked by it initially, but on
further reading, I observed that it's only a huge patch which also does
some mechanical changes to test output.  I think it'd be better to split
the part that's responsible for the changed lines in test output
mentioning ecpg_process_output.  That should be a reasonably small
patch which changes ecpg_execute slightly and adds the new function, is
followed by the enormous resulting mechanical changes in test output.
It should be possible to commit that relatively quickly.  Then there's
the rest of the patch, which would adds a huge pile of new code.

I think there are some very minor changes to backend code as well --
would it make sense to post that as a separate piece?


I had to rebase the patch against current (today morning's) GIT, since
there were a few changes against ECPG in the meantime.

The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude. You can pull
the commits individually from the above repository. For the same
reason, I won't add the DECLARE CURSOR command tag change
separately since this is also part of this big feature.

I have reordered some patches, like some independent bug fixes
against the ECPG parser and regression tests. The backend change
is also added early.

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] Commitfest II CLosed

2013-10-21 Thread Boszormenyi Zoltan


2013-10-21 17:11 keltezéssel, Robert Haas írta:

On Mon, Oct 21, 2013 at 9:18 AM, Andres Freund and...@2ndquadrant.com wrote:

On 2013-10-21 09:15:36 -0400, Peter Eisentraut wrote:

On 10/21/13 1:31 AM, Andres Freund wrote:

The point of the CF is exactly that all
patches get at least one good round of review. Moving unreviewed patches
to the next CF will let them just suffer the same fate there.

What is the alternative?

I am not 100% sure, but what's the point of the CF if we're not actually
reviewing patches that wouldn't get review without it? So I guess it's
not starting the next one before we've finished - which we obviously
haven't in this case - the last one.

Yeah.  There were a huge number of patches in this CommitFest that sat
around in the waiting on author state for hugely long periods of time.
  One of the critical functions of the CommitFest manager(s) IMV is to
make sure that patches that are in that state get pushed to Returned
with Feedback so that it's more obvious which things are still alive
and kicking.  That really wasn't done until about a week before the
end of the CommitFest, when I stepped in and did some of it.  But that
really needs to be more of an ongoing process.

Supposedly, we have a policy that for each patch you submit, you ought
to review a patch.  That right there ought to provide enough reviewers
for all the patches, but clearly it didn't.  And I'm pretty sure that
some people (like me) looked at a lot MORE patches than they
themselves submitted.  I think auditing who is not contributing in
that area and finding tactful ways to encourage them to contribute
would be a very useful service to the project.


I wanted to get to this point, too.

I hoped that reviewing 4 patches in this CF (UNNEST, Extension templates,
DISCARD SEQUENCES, and extended RETURNING syntax) gets my huge patch reviewed.

I even provided a repo @github where it was broken up into pieces that can be 
sanely reviewed.
It still wasn't enough. Even Michael Meskes (ECPG is his pet project) and the 
guy @Fujitsu
who contacted me privately and expressed interest in this patch didn't chime in.
As a social experiment, the CF looks like a clear failure from this seat of 
mine. (Sorry.)



Finally, I think we need to have some discussion of the patches that
are ready for committer but got punted, and see if we can figure out
whether any committer has plans to look at them.  Those patches are:

Extension Templates - I think Peter Eisentraut commented on this one
at some stage, but I'm not sure if he's planning to work further on
it.
UNNEST with multiple args, and TABLE with multiple functions - Heikki
did some work on this, maybe he's planning to commit it?
Numeric Aggregates Performance Improvement - I looked at this one
previously so should probably look it over again.
Statistics collection for CLUSTER command - Noah recommended rejecting
this on performance grounds.  Maybe we should do that.
simple date time constructors - Alvaro previously looked at this, but
I don't know whether he plans to work on it further.
simple LO API - no committer interest to my knowledge
Bugfix for timeout in LDAP connection parameter resolution - I think
Peter Eisentraut is planning to commit this




--
--
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] Commitfest II CLosed

2013-10-21 Thread Boszormenyi Zoltan


2013-10-21 18:25 keltezéssel, Stephen Frost írta:

Zoltan,

* Boszormenyi Zoltan (z...@cybertec.at) wrote:

I even provided a repo @github where it was broken up into pieces that can be 
sanely reviewed.

You also gave the first person looking at the patch a hard time about
asking for it to be broken up; unnecessairly, imv.


Sorry if it felt that way.  I gave the commit IDs about the points he brought 
up.


   Thanks for breaking
it up and for doing patch review of other patches- it does help the
project move forward.  Try to be understanding when someone asks a
question that's already been answered; we're all quite busy and may
forget or miss things.

I don't know if Alvaro will have time to look into this or if he perhaps
already has, but I had noticed this patch earlier and it was one of the
ones I had hoped to take a look at, as I've been in the ECPG bits due to
the work with Coverity that I've been doing.  I'll try and provide
feedback later this week/weekend on it.

Thanks,

Stephen



--
--
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] Commitfest II CLosed

2013-10-21 Thread Boszormenyi Zoltan


2013-10-21 19:10 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan escribió:


I hoped that reviewing 4 patches in this CF (UNNEST, Extension templates,
DISCARD SEQUENCES, and extended RETURNING syntax) gets my huge patch reviewed.

I'm still on the hook for parts of this one (and also for Pavel's date
constructors stuff).


Thank you very much.


I won't touch the ones that modify the core of
ecpg, but I hope I hope I will be able to look at the other ones to ease
work for Michael.


I hope he has time for this patch soon. When this patch was brought up
last time, he expressed interest.


I spent the last two weeks moving to another city,
which was pretty exhausting, so I wasn't able to get as much done as I
hoped.  It's finally done now and today I got a stable network
connection in the new place.


I wish you the best with your new place and friendly neighbours! :-)

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] Commitfest II CLosed

2013-10-20 Thread Boszormenyi Zoltan


Hi,

2013-10-19 17:20 keltezéssel, David Fetter írta:

Thanks very much to Mike Blackwell and Craig Kerstiens for their
persistence through what most people would consider a tedious and
thankless task.  Thanks also to the patch submitters, reviewers and
other participants.

That the formal commitfest is over does not mean that your patch won't
get reviewed or committed until November.  What it does mean is that
people will be setting patch review as a lower priority, frequently so
they can live their lives, work on new stuff, do their day jobs...

We got 20 patches, many quite significant, committed this time.
Kudos!

Cheers,
David.


what will happen to patches left in pending state in the 2013-09 CF?

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] ECPG FETCH readahead

2013-10-13 Thread Boszormenyi Zoltan


2013-10-11 00:16 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan escribió:

2013-09-10 03:04 keltezéssel, Peter Eisentraut írta:

You need to update the dblink regression tests.

Done.

Dude, this is an humongous patch.


You *know* that the patch is available in pieces at 
https://github.com/zboszor/ecpg-readahead
right? You must have read the new initial announcement at
http://archives.postgresql.org/message-id/520f4b90.2010...@cybertec.at

You can review and merge them one by one.
The transformation of ecpg_execute() and friends starts at
2d04ff83984c63c34e55175317e3e431eb58e00c


   I was shocked by it initially, but on
further reading, I observed that it's only a huge patch which also does
some mechanical changes to test output.  I think it'd be better to split
the part that's responsible for the changed lines in test output
mentioning ecpg_process_output.


It's 1e0747576e96aae3ec8c60c46baea130aaf8916e in the above repository.

Also, another huge regression test patch is where the cursor readahead
is enabled unconditionally: 27e43069082b29cb6fa4d3414e6484ec7fb80cbe


   That should be a reasonably small
patch which changes ecpg_execute slightly and adds the new function, is
followed by the enormous resulting mechanical changes in test output.
It should be possible to commit that relatively quickly.  Then there's
the rest of the patch, which would adds a huge pile of new code.

I think there are some very minor changes to backend code as well --
would it make sense to post that as a separate piece?


It's 2ad207e6371a33d6a985c76ac066dd51ed5681cb
Also, cd881f0363b1aff1508cfa347e8df6b4981f0ee7 to fix the dblink regression 
test.

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

[HACKERS] WHERE CURRENT OF behaviour is not what's documented

2013-09-18 Thread Boszormenyi Zoltan


Hi,

I have experimented with cursors a little and found that the part about FOR SHARE/FOR 
UPDATE in


http://www.postgresql.org/docs/9.2/interactive/sql-declare.html

i.e. the sensitive cursor is not what actually happens. BTW, 9.3 has the same contents 
for the same page.



If the cursor's query includes FOR UPDATE or FOR SHARE, then returned rows are locked at 
the time they are first fetched, in the same way as for a regular SELECT 
http://www.postgresql.org/docs/9.3/interactive/sql-select.html command with these 
options. In addition, the returned rows will be the most up-to-date versions; therefore 
these options provide the equivalent of what the SQL standard calls a sensitive cursor. 
(Specifying INSENSITIVE together with FOR UPDATE or FOR SHARE is an error.)



The statement that the most up-to-date versions of the rows are returned
doesn't reflect the reality anymore:

$ psql
psql (9.2.4)
Type help for help.

zozo= create table xxx (id serial primary key, t text);
NOTICE:  CREATE TABLE will create implicit sequence xxx_id_seq for serial column 
xxx.id
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index xxx_pkey for table 
xxx
CREATE TABLE
zozo= insert into xxx (t) values ('a'), ('b'), ('c');
INSERT 0 3
zozo= begin;
BEGIN
zozo= declare mycur cursor for select * from xxx for update;
DECLARE CURSOR
zozo= fetch all from mycur;
 id | t
+---
  1 | a
  2 | b
  3 | c
(3 rows)

zozo= move absolute 0 in mycur;
MOVE 0
zozo= fetch from mycur;
 id | t
+---
  1 | a
(1 row)

zozo= update xxx set t = t || '_x' where current of mycur;
UPDATE 1
zozo= move absolute 0 in mycur;
MOVE 0
zozo= fetch all from mycur;
 id | t
+---
  2 | b
  3 | c
(2 rows)

What happened to the most up-to-date row of id == 1?

zozo= select * from xxx where id = 1;
 id |  t
+-
  1 | a_x
(1 row)

The same behaviour is experienced under 9.2.4 and 9.3.0.

As a side note, I couldn't test 8.4.17, 9.0.13 and 9.1.9 under Fedora 19,
because initdb fails for all 3 versions. I am bitten by the same as what's
described here: 
http://www.postgresql.org/message-id/14242.1365200...@sss.pgh.pa.us

It the above cursor behaviour is the accepted/expected one?

Since SCROLL (with or without INSENSITIVE) cannot be specified together
with FOR UPDATE/FOR SHARE, I know the MOVE ABSOLUTE 0 is on the
verge of being invalid in this case.

But in any case, either the documentation should tell that the UPDATEd
rows will be missing from a reset executor run or MOVE ABSOLUTE
with a value smaller than portal-portalPos should also be refused
just like MOVE BACKWARD.

As another side note, portal-portalPos mentions it can overflow,
so I suggest using int64 explicitly, so it's ensured that 32-bit systems
get the same overflow behaviour as 64-bit ones. Or (the horror, the horror!) 
int128_t.

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/

Re: [HACKERS] WHERE CURRENT OF behaviour is not what's documented

2013-09-18 Thread Boszormenyi Zoltan


2013-09-18 14:27 keltezéssel, Andres Freund írta:

On 2013-09-18 14:23:19 +0200, Boszormenyi Zoltan wrote:

Hi,

I have experimented with cursors a little and found that the part about FOR
SHARE/FOR UPDATE in

http://www.postgresql.org/docs/9.2/interactive/sql-declare.html

i.e. the sensitive cursor is not what actually happens. BTW, 9.3 has the
same contents for the same page.


If the cursor's query includes FOR UPDATE or FOR SHARE, then returned rows
are locked at the time they are first fetched, in the same way as for a
regular SELECT
http://www.postgresql.org/docs/9.3/interactive/sql-select.html command
with these options. In addition, the returned rows will be the most
up-to-date versions; therefore these options provide the equivalent of what
the SQL standard calls a sensitive cursor. (Specifying INSENSITIVE
together with FOR UPDATE or FOR SHARE is an error.)


The statement that the most up-to-date versions of the rows are returned
doesn't reflect the reality anymore:

I think it's not referring to the behaviour inside a single session but
across multiple sessions. I.e. when we follow the ctid chain of a tuple
updated in a concurrent session.


But the documentation doesn't spell it out. Perhaps a little too terse.

Quoting the SQL2011 draft, 4.33.2 Operations on and using cursors, page 112:

If a cursor is open, and the SQL-transaction in which the cursor was opened makes a 
significant change to
SQL-data, then whether that change is visible through that cursor before it is closed is 
determined as follows:

— If the cursor is insensitive, then significant changes are not visible.
— If the cursor is sensitive, then significant changes are visible.
— If the cursor is asensitive, then the visibility of significant changes is 
implementation-dependent.


SQL2003 has the same wording in 4.32.2 Operations on and using cursors
on page 96.

So, a SENSITIVE cursor shows significant changes (I guess a modified
row counts as one) and they should be shown in the _same_ transaction
where the cursor was opened. If anything, the PostgreSQL cursor
implementation for FOR SHARE/FOR UPDATE is asensitive.

Also, 14.10 update statement: positioned, paragraph 14) in General Rules
in SQL2003 (page 848) or 15.6 Effect of a positioned update, paragraph 16)
in SQL2011 draft (page 996) says the new row replaces the old row
*in the cursor*, not just in the table. Quote:


Let R1 be the candidate new row and let R be the current row of CR.
...
The current row R of CR is replaced by R1.


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

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

2013-08-27 Thread Boszormenyi Zoltan


2013-08-27 01:24 keltezéssel, Andrew Gierth írta:

Latest version of patch. This should be it as far as code goes; there
may be some more regression test work, and a doc patch will be
forthcoming.

This version supports, in addition to the previous stuff:

[snip]


In my limited testing, it works well, and the patch looks good.

When adding regression tests, can you please add intentional
syntax error cases to exercise all the new ereport()s?

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] Extension Templates S03E11

2013-08-27 Thread Boszormenyi Zoltan


2013-08-27 18:09 keltezéssel, Dimitri Fontaine írta:

Hi,

Boszormenyi Zoltan z...@cybertec.at writes:

Here's a review for this patch.

Thanks for that review Zoltan!


You're welcome.


I would like to see the control parameters documented in allcaps
in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
TEMPLATE should reference the CREATE instead of the longer
text in xref linkend=extend-extensions. This xref can still
be there for reference in all three of CREATE/ALTER/DROP
EXTENSION TEMPLATE.

I didn't follow exactly your proposal here because I didn't want to have
to maintain the control parameter description list in two different
places. I've still added a detailed list with references and details and
more importantly with coverage of e.g. NORELOCATABLE which is not
covered in the referenced material.


It looks better. Now that the lowercase keywords are expected
by the eye in the referenced text, they will be much easier to find.
Thanks.

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] GSOC13 proposal - extend RETURNING syntax


Hi,

2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:

W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:

Here's a new one, for v7:

setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]

  bind_returning_variables(rlist, before_index, after_index);
  ^
setrefs.c:1957:21: note: ‘before_index’ was declared here
  int after_index=0, before_index;
 ^

Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka


With this fixed, a more complete review:

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Yes.

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

There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where returning.sql resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.

doc/src/sgml/ref/update.sgml describes this feature.

doc/src/sgml/dml.sgml should also be touched to cover this feature.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.

It implements the community-agreed behaviour, according to
the new regression test coverage.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:

zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 
text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
 id | i1 | i2 | t1 | t2
++++
  1 |  1 |  1 | a  | a
  2 |  2 |  2 | b  | b
  3 |  3 |  3 | c  | c
(3 rows)

zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1, 
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;

 i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
+++++++-
  2 |  2 |  2 |  4 | b  | b  | b  | bx2
(1 row)

UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 3 
returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1, 
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1  | t2

++++++-+-
  3 |  3 |  9 |  6 | c  | c  | cx3 | cx2
(1 row)

UPDATE 1



* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines?

Mostly.

In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.

In b/src/backend/optimizer/plan/setrefs.c:

+static void bind_returning_variables(List *rlist, int bef, int aft);

but later it becomes not public:

+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{

Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.

Similar for parse_clause.c:

+extern void addAliases(ParseState *pstate);

+void addAliases(ParseState *pstate)

This external declaration is not needed since it is already
in src/include/parser/parse_clause.h

In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so before and after are spelled out.
These are not C keywords.

Some assignments, like:

+   var=(Var*)tle;
and
+   int index_rel=1;

in setrefs.c need spaces.

if() statements need a space before the ( and not after.

Add spaces in the {} list in addAliases():
+   char*aliases[] = {before,after};
like this: { before, after }

Spaces are needed here, too:
+   for(i=0 ; inoal; i++)

This noal should be naliases or n_aliases if you really want
a variable. I would simply use the constant 2 for the two for()
loops in addAliases() instead, its purpose is obvious enough.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block.


* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

Yes.

* Are the comments sufficient

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax


2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:

Hi,

2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:

W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:

Here's a new one, for v7:

setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]

  bind_returning_variables(rlist, before_index, after_index);
  ^
setrefs.c:1957:21: note: ‘before_index’ was declared here
  int after_index=0, before_index;
 ^

Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka


With this fixed, a more complete review:

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Yes.

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

There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where returning.sql resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.

doc/src/sgml/ref/update.sgml describes this feature.

doc/src/sgml/dml.sgml should also be touched to cover this feature.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.

It implements the community-agreed behaviour, according to
the new regression test coverage.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:

zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 
text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
 id | i1 | i2 | t1 | t2
++++
  1 |  1 |  1 | a  | a
  2 |  2 |  2 | b  | b
  3 |  3 |  3 | c  | c
(3 rows)

zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1, 
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;

 i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
+++++++-
  2 |  2 |  2 |  4 | b  | b  | b  | bx2
(1 row)

UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 
3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1, 
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1  | t2

++++++-+-
  3 |  3 |  9 |  6 | c  | c  | cx3 | cx2
(1 row)

UPDATE 1



* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines?

Mostly.

In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.

In b/src/backend/optimizer/plan/setrefs.c:

+static void bind_returning_variables(List *rlist, int bef, int aft);

but later it becomes not public:

+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{

Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.

Similar for parse_clause.c:

+extern void addAliases(ParseState *pstate);

+void addAliases(ParseState *pstate)

This external declaration is not needed since it is already
in src/include/parser/parse_clause.h

In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so before and after are spelled out.
These are not C keywords.

Some assignments, like:

+   var=(Var*)tle;
and
+   int index_rel=1;

in setrefs.c need spaces.

if() statements need a space before the ( and not after.

Add spaces in the {} list in addAliases():
+   char*aliases[] = {before,after};
like this: { before, after }

Spaces are needed here, too:
+   for(i=0 ; inoal; i++)

This noal should be naliases or n_aliases if you really want
a variable. I would simply use the constant 2 for the two for()
loops in addAliases() instead, its purpose is obvious enough.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block.


* Are there portability issues?

No.

* Will it work

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax


2013-08-21 20:00 keltezéssel, Boszormenyi Zoltan írta:

2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:

Hi,

2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:

W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:

Here's a new one, for v7:

setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]

  bind_returning_variables(rlist, before_index, after_index);
  ^
setrefs.c:1957:21: note: ‘before_index’ was declared here
  int after_index=0, before_index;
 ^

Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka


With this fixed, a more complete review:

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Yes.

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

There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where returning.sql resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.

doc/src/sgml/ref/update.sgml describes this feature.

doc/src/sgml/dml.sgml should also be touched to cover this feature.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.

It implements the community-agreed behaviour, according to
the new regression test coverage.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:

zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 
text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
 id | i1 | i2 | t1 | t2
++++
  1 |  1 |  1 | a  | a
  2 |  2 |  2 | b  | b
  3 |  3 |  3 | c  | c
(3 rows)

zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1, 
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;

 i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
+++++++-
  2 |  2 |  2 |  4 | b  | b  | b  | bx2
(1 row)

UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id 
= 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1, 
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1  | t2

++++++-+-
  3 |  3 |  9 |  6 | c  | c  | cx3 | cx2
(1 row)

UPDATE 1



* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines?

Mostly.

In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.

In b/src/backend/optimizer/plan/setrefs.c:

+static void bind_returning_variables(List *rlist, int bef, int aft);

but later it becomes not public:

+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{

Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.

Similar for parse_clause.c:

+extern void addAliases(ParseState *pstate);

+void addAliases(ParseState *pstate)

This external declaration is not needed since it is already
in src/include/parser/parse_clause.h

In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so before and after are spelled out.
These are not C keywords.

Some assignments, like:

+   var=(Var*)tle;
and
+   int index_rel=1;

in setrefs.c need spaces.

if() statements need a space before the ( and not after.

Add spaces in the {} list in addAliases():
+   char*aliases[] = {before,after};
like this: { before, after }

Spaces are needed here, too:
+   for(i=0 ; inoal; i++)

This noal should be naliases or n_aliases if you really want
a variable. I would simply use the constant 2 for the two for()
loops in addAliases() instead, its purpose is obvious enough.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax


Hi,

2013-08-21 20:52 keltezéssel, Karol Trzcionka írta:

W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:

With this fixed, a more complete review:

Thanks.

There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where returning.sql resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.

Modified to work correct in parallel testing

doc/src/sgml/ref/update.sgml describes this feature.

doc/src/sgml/dml.sgml should also be touched to cover this feature.
I don't think so, there is not any info about returning feature, I think it shouldn't be 
part of my patch.

In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.

Fixed


In b/src/backend/optimizer/plan/setrefs.c:

+static void bind_returning_variables(List *rlist, int bef, int aft);

but later it becomes not public:

+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{

I've change to static in the definition.


+extern void addAliases(ParseState *pstate);

+void addAliases(ParseState *pstate)

This external declaration is not needed since it is already
in src/include/parser/parse_clause.h

Removed.


In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so before and after are spelled out.
These are not C keywords.

Changed.

Some assignments, like:

+   var=(Var*)tle;
and
+   int index_rel=1;

in setrefs.c need spaces.

if() statements need a space before the ( and not after.

Add spaces in the {} list in addAliases():
+   char*aliases[] = {before,after};
like this: { before, after }

Spaces are needed here, too:
+   for(i=0 ; inoal; i++)

This noal should be naliases or n_aliases if you really want
a variable. I would simply use the constant 2 for the two for()
loops in addAliases() instead, its purpose is obvious enough.

Added some whitespaces.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block.

Added.

Other comments:

This #define in pg_class:

diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 49c4f6f..1b09994 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -154,6 +154,7 @@ DESCR();
 #define  RELKIND_COMPOSITE_TYPE  'c' /* composite type */
 #define  RELKIND_FOREIGN_TABLE   'f' /* foreign table */
 #define  RELKIND_MATVIEW 'm'   /* materialized view */
+#define  RELKIND_BEFORE 'b'   /* virtual table for 
before/after statements */


 #define  RELPERSISTENCE_PERMANENT 'p' /* regular 
table */
 #define  RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent 
table */


RELKIND_BEFORE removed - it probably left over previous work and/or I needed it because 
RTE_BEFORE wasn't available (not included?).

Speaking of which, RTE_BEFORE is more properly named
RTE_RETURNING_ALIAS or something like that because it
covers both before and after. Someone may have a better
idea for naming this symbol.

Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases)


Others may have also a word in this topic, but consider that
this is *not* a regular alias and for those, RTE_ALIAS is not used,
like in

UPDATE table AS alias SET ...

Maybe RTE_RETURNING_ALIAS takes a little more typing, but
it becomes obvious when reading and new code won't confuse
it with regular aliases.


One question, though, about this part:


@@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
int rtoffset)
 {
indexed_tlist *itlist;
+   int after_index=0, before_index=0;
+   Query  *parse = root-parse;

+   ListCell   *rt;
+   RangeTblEntry *bef;
+
+   int index_rel=1;
+
+   foreach(rt,parse-rtable)
+   {
+   bef = (RangeTblEntry *)lfirst(rt);
+   if(strcmp(bef-eref-aliasname,after) == 0  bef-rtekind == 
RTE_BEFORE )

+   {
+   after_index = index_rel;
+   }
+   if(strcmp(bef-eref-aliasname,before) == 0  bef-rtekind == 
RTE_BEFORE )

+   {
+   before_index = index_rel;
+   }
+   index_rel++;
+   }
/*
 * We can perform the desired Var fixup by abusing the fix_join_expr
 * machinery that formerly handled inner indexscan fixup.  We search the
@@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
resultRelation,
  rtoffset);

+   bind_returning_variables(rlist, before_index, after_index);
pfree(itlist);

return rlist

Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs


2013-08-20 08:13 keltezéssel, Pavel Stehule írta:




2013/8/20 David Fetter da...@fetter.org mailto:da...@fetter.org

On Mon, Aug 19, 2013 at 07:45:23PM +0200, Pavel Stehule wrote:
 Hello

 Harder maybe but it may still be cleaner in the long run.
 
   Overall, it's my intention here to remove as many as feasible of the 
old
  reasons why one might use an SRF in the select list.
 
 
  Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having
  WITH ORDINALITY and this feature, I would vote for removing
  SRF-in-targetlist and call the release PostgreSQL 10.0.
 

 Although I would to remove SRF from targetlist, I don't think so this 
hurry
 strategy is good idea. We should to provide new functionality and old
 functionality one year as minimum, and we should to announce so this
 feature is deprecated

We could do this in 9.3, but all it would be is an announcement, i.e.
no code change of any nature.

 - and maybe use a GUC for disabling, warning and deprecating.



To really ensure backward compatibility, this sentence should read as
add a GUC for disabling *the* warning and deprecating. :-

As I said, I am such an agent provocateur.
Let this side track die and concentrate on the merits of the patch itself. :-)



With utmost respect, I think the general idea of setting SQL grammar
via GUC is a really bad one.  When we've done so in the past, it's
done more harm than good, and we should not repeat it.


so as minumum is controlling warning via GUC, we should to help with identification of 
problematic queries.


Regards

Pavel


 More, I would to see 9.4 release:).

Same here! :)

 x.4 are happy PostgreSQL releases :)

Each one has been at least baseline happy for me since 7.1.  Some have
made me overjoyed, though.

Cheers,
David.
--
David Fetter da...@fetter.org mailto:da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778 tel:%2B1%20415%20235%203778  AIM: dfetter666  
Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com 
mailto:david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
http://www.tripit.com/feed/ical/people/david74/tripit.ics

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





--
--
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/

Re: [HACKERS] Backup throttling


2013-08-20 08:37 keltezéssel, Heikki Linnakangas írta:

On 19.08.2013 21:15, Boszormenyi Zoltan wrote:

2013-08-19 19:20 keltezéssel, Andres Freund írta:

Based on a quick look it seems like you're throttling on the receiving
side. Is that a good idea? Especially over longer latency links, TCP
buffering will reduce the effect on the sender side considerably.


Throttling on the sender side requires extending the syntax of
BASE_BACKUP and maybe START_REPLICATION so both can be
throttled but throttling is still initiated by the receiver side.


Throttling in the client seems much better to me. TCP is designed to handle a 
slow client.


Maybe throttling the walsender is not a good idea, it can lead
to DoS via disk space shortage.


If a client can initiate a backup and/or streaming replication, he can already do much 
more damage than a DoS via out of disk space. And a nothing stops even a non-privileged 
user from causing an out of disk space situation anyway. IOW that's a non-issue.


I got to the same conclusion this morning, but because of wal_keep_segments.

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



- Heikki





--
--
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] Extension Templates S03E11


Hi,

2013-08-04 15:20 keltezéssel, Dimitri Fontaine írta:

Thom Brown t...@linux.com writes:

Could you please resubmit this without using SnapshotNow as it's no longer
supported?

Sure, sorry that I missed that, please find v12 attached.


Here's a review for this patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

No, it has one reject in src/bin/pg_dump/pg_backup_archiver.c.
It was obvious to fix, version 12a is attached.

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

It has extended the SQL reference nicely but the reference to
xref linkend=extend-extensions was not obvious enough
regarding the list of control parameters.

The SQL syntax has them in allcaps, while the referenced section
has them in lower case. It's easy to miss them while just browsing
for e.g. RELOCATABLE. I had to go back twice to find the proper
part of the text.

I would like to see the control parameters documented in allcaps
in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
TEMPLATE should reference the CREATE instead of the longer
text in xref linkend=extend-extensions. This xref can still
be there for reference in all three of CREATE/ALTER/DROP
EXTENSION TEMPLATE.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

There's no such provision in the spec.
As far as I can tell, it follows the community-agreed behavior.

* Does it include pg_dump support (if applicable)?

Yes.

But the version check is already wrong in src/bin/pg_dump/pg_dump.c
since this patch missed 9.3.

+   /*
+* Before 9.3, there are no extension templates.
+*/
+   if (fout-remoteVersion  90300)
+   {
+   *numExtensionTemplates = 0;
+   return NULL;
+   }
+

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines?

Yes.

Nitpicking. This chunk has an extra unnecessary space:

diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index c4d3f3c..689dc37 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -38,6 +38,7 @@ POSTGRES_BKI_SRCS = $(addprefix 
$(top_srcdir)/src/include/catalog/,\
pg_authid.h pg_auth_members.h pg_shdepend.h pg_shdescription.h \
pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
pg_ts_parser.h pg_ts_template.h pg_extension.h \
+pg_extension_control.h pg_extension_template.h pg_extension_uptmpl.h \
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
pg_foreign_table.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h 
pg_range.h \

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

According to the added regression tests, yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other 
features/modules?

I think so.

* Are there interdependencies that can cause problems?

I don't know.

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/



templates.v12a.patch.gz
Description: Unix tar archive

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

Re: [HACKERS] PL/pgSQL PERFORM with CTE


2013-08-20 14:35 keltezéssel, David E. Wheeler írta:

On Aug 20, 2013, at 2:31 PM, Pavel Stehule pavel.steh...@gmail.com wrote:


but it works

postgres=# do $$begin with x as (select 10) insert into omega select * from x; 
end;$$;
DO

But this does not:

david=# DO $$
david$# BEGIN
david$# PERFORM * FROM (
david$# WITH inserted AS (
david$# INSERT INTO foo values (1) RETURNING id
david$# ) SELECT inserted.id
david$# ) x;
david$# END;
david$# $$;
ERROR:  WITH clause containing a data-modifying statement must be at the top 
level
LINE 2: WITH inserted AS (
  ^
QUERY:  SELECT * FROM (
 WITH inserted AS (
 INSERT INTO foo values (1) RETURNING id
 ) SELECT inserted.id
 ) x
CONTEXT:  PL/pgSQL function inline_code_block line 3 at PERFORM


This is the same error as if you put the WITH into a subquery,
which is what PERFORM does.

Proof:

SELECT * FROM (
WITH inserted AS (
INSERT INTO foo values (1) RETURNING id
) SELECT inserted.id
) x;


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] GSOC13 proposal - extend RETURNING syntax


Hi,

2013-08-19 21:52 keltezéssel, Boszormenyi Zoltan írta:

2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:

W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:

* Does it apply cleanly to the current git master?

No. There's a reject in src/backend/optimizer/plan/initsplan.c

Thank you, merged in attached version.

* Does it include reasonable tests?

Yes but the test fails after trying to fix the rejected chunk of the
patch.

It fails because the HINT was changed, fixed.
That version merges some nested ifs left over from earlier work.


I tried to compile your v5 patch and I got:

initsplan.c: In function ‘add_vars_to_targetlist’:
initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]

rel-reltargetlist = lappend(rel-reltargetlist,
^

You shouldn't change the assignment at declaration:

- RelOptInfo *rel = find_base_rel(root, var-varno);
+ RelOptInfo *rel;
...
+ if (root-parse-commandType == CMD_UPDATE)
+ {
... (code using rel)
+ }
+ rel = find_base_rel(root, varno);


Let me say it again: the new code in initsplan.c::add_vars_to_targetlist() is 
fishy.
The compiler says that rel used on line 216 may be uninitialized.

Keeping it that way passes make check, perhaps rel was initialized
in a previous iteration of foreach(temp, vars), possibly in the
else if (IsA(node, PlaceHolderVar))
branch, which means that PlaceHolderInfo *phinfo may be interpreted
as RelOptInfo *, stomping on memory.

Moving the assignment back to the declaration makes make check
fail with the attached regression.diffs file.

Initializing it as RelOptInfo *rel = NULL; makes the regression check
die with a segfault, obviously.

Change the code to avoid the warning and still produce the wanted effect.

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/

*** 
/home/zozo/crosscolumn/review/returning-before-after/postgresql/src/test/regress/expected/returning_before_after.out
2013-08-20 15:01:03.365314482 +0200
--- 
/home/zozo/crosscolumn/review/returning-before-after/postgresql/src/test/regress/results/returning_before_after.out
 2013-08-20 15:30:21.091641454 +0200
***
*** 6,47 
);
  INSERT INTO foo VALUES (1, 'x'),(2,'y');
  UPDATE foo SET bar1=bar1+1 RETURNING before.*, bar1, bar2;
!  bar1 | bar2 | bar1 | bar2 
! --+--+--+--
! 1 | x|2 | x
! 2 | y|3 | y
! (2 rows)
! 
  UPDATE foo SET bar1=bar1-1 RETURNING after.bar1, before.bar1*2;
!  bar1 | ?column? 
! --+--
! 1 |4
! 2 |6
! (2 rows)
! 
  UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'z' RETURNING before.*, after.*;
!  bar1 | bar2 | bar1 | bar2 
! --+--+--+--
! 1 | x|2 | xz
! 2 | y|3 | yz
! (2 rows)
! 
  -- check single after
  UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'a' RETURNING after.*;
!  bar1 | bar2 
! --+--
! 3 | xza
! 4 | yza
! (2 rows)
! 
  -- check single before
  UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'b' RETURNING before.*;
!  bar1 | bar2 
! --+--
! 3 | xza
! 4 | yza
! (2 rows)
! 
  -- it should fail
  UPDATE foo SET bar1=bar1+before.bar1 RETURNING before.*;
  ERROR:  missing FROM-clause entry for table before
--- 6,22 
);
  INSERT INTO foo VALUES (1, 'x'),(2,'y');
  UPDATE foo SET bar1=bar1+1 RETURNING before.*, bar1, bar2;
! ERROR:  no relation entry for relid 2
  UPDATE foo SET bar1=bar1-1 RETURNING after.bar1, before.bar1*2;
! ERROR:  no relation entry for relid 3
  UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'z' RETURNING before.*, after.*;
! ERROR:  no relation entry for relid 2
  -- check single after
  UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'a' RETURNING after.*;
! ERROR:  no relation entry for relid 3
  -- check single before
  UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'b' RETURNING before.*;
! ERROR:  no relation entry for relid 2
  -- it should fail
  UPDATE foo SET bar1=bar1+before.bar1 RETURNING before.*;
  ERROR:  missing FROM-clause entry for table before
***
*** 53,90 
   ^
  -- test before/after aliases
  UPDATE foo AS before SET bar1=bar1+1 RETURNING before.*,after.*;
!  bar1 | bar2 | bar1 | bar2 
! --+--+--+--
! 5 | xzab |5 | xzab
! 6 | yzab |6 | yzab
! (2 rows)
! 
  UPDATE foo AS after SET bar1=bar1-1 RETURNING before.*,after.*;
!  bar1 | bar2 | bar1 | bar2 
! --+--+--+--
! 5 | xzab |4 | xzab
! 6 | yzab |5 | yzab
! (2 rows)
! 
  -- test inheritance
  CREATE TABLE foo2 (bar INTEGER) INHERITS(foo);
  INSERT INTO foo2 VALUES (1,'b',5);
  UPDATE foo2 SET bar1=bar1*2, bar=bar1+5, bar2=bar1::text || bar::text 
RETURNING before.*, after.*, *;
!  bar1 | bar2 | bar | bar1 | bar2 | bar | bar1 | bar2 | bar

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax


2013-08-20 17:30 keltezéssel, Karol Trzcionka írta:

W dniu 20.08.2013 16:47, Karol Trzcionka pisze:

Thank you for the review and tests. New version introduce a lot of
improvements:
- Fix regression test for view (wrong table_name)
- Add regression test for inheritance
- Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
uninitialized issue
- Revert changing varno in add_vars_to_targetlist
- Add all before variables to targetlist
- Avoid adding variables to slot for AFTER.
- Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
- All before/after are now set on OUTER_VAR
- Rename fix_varno_varattno to bind_returning_variables
- Add comment about bind_returning_variables
- Remove unneeded code in fix_join_expr_mutator (it was changing varno
 of RTE_BEFORE - now there is not any var with varno assigned to it)

I've just realized the prepare_returning_before() is unneeded right now
so I've removed it. Version 7, hopefully the last. ;)


Here's a new one, for v7:

setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]

  bind_returning_variables(rlist, before_index, after_index);
  ^
setrefs.c:1957:21: note: ‘before_index’ was declared here
  int after_index=0, before_index;
 ^

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


Regards,
Karol Trzcionka





--
--
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/

Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs


Hi,

2013-08-13 15:54 keltezéssel, Andrew Gierth írta:

Summary:

This patch implements a method for expanding multiple SRFs in parallel
that does not have the surprising LCM behaviour of SRFs-in-select-list.
(Functions returning fewer rows are padded with nulls instead.)

It then uses this method combined with a parse-time hack to implement
the (intended to be) spec-conforming behaviour of UNNEST with multiple
parameters, including flattening of composite results.

The upshot is that given a table like this:

postgres=# select * from t1;
a   | b |  c
---+---+--
  {11,12,13}| {wombat}  |
  {5,10}| {foo,bar} | 
{(123,xyzzy),(456,plugh),(789,plover)}
  {21,31,41,51} | {fred,jim,sheila} | {(111,xyzzy),(222,plugh)}
(3 rows)

(where column c is an array of a composite type with 2 cols, x and y)

You can do this:

postgres=# select u.* from t1, unnest(a,b,c) with ordinality as u;
  ?column? | ?column? |  x  |   y| ordinality
--+--+-++
11 | wombat   | ||  1
12 |  | ||  2
13 |  | ||  3
 5 | foo  | 123 | xyzzy  |  1
10 | bar  | 456 | plugh  |  2
   |  | 789 | plover |  3
21 | fred | 111 | xyzzy  |  1
31 | jim  | 222 | plugh  |  2
41 | sheila   | ||  3
51 |  | ||  4
(10 rows)

Or for an example of general combination of functions:

postgres=# select * from table(generate_series(10,20,5), 
unnest(array['fred','jim']));
  ?column? | ?column?
--+--
10 | fred
15 | jim
20 |
(3 rows)

Implementation Details:

The spec syntax for table function calls, table function derived table
in table reference, looks like TABLE(func(args...)) AS ...

This patch implements that, plus an extension: it allows multiple
functions, TABLE(func1(...), func2(...), func3(...)) [WITH ORDINALITY]
and defines this as meaning that the functions are to be evaluated in
parallel.

This is implemented by changing RangeFunction, function RTEs, and the
FunctionScan node to take lists of function calls rather than a single
function. The calling convention for SRFs is completely unchanged; each
function returns its own rows (or a tuplestore in materialize mode) just
as before, and FunctionScan combines the results into a single output
tuple (keeping track of which functions are exhausted in order to
correctly fill in nulls on a backwards scan).

Then, a hack in the parser converts unnest(...) appearing as a
func_table (and only there) into a list of unnest() calls, one for each
parameter.  So

select ... from unnest(a,b,c)

is converted to

select ... from TABLE(unnest(a),unnest(b),unnest(c))

and if unnest appears as part of an existing list inside TABLE(), it's
expanded to multiple entries there too.

This parser hackery is of course somewhat ugly. But given the objective
of implementing the spec's unnest syntax, it seems to be the least ugly
of the possible approaches. (The hard part of doing it any other way
would be generating the description of the result type; composite array
parameters expand into multiple result columns.)


Harder maybe but it may still be cleaner in the long run.


Overall, it's my intention here to remove as many as feasible of the old
reasons why one might use an SRF in the select list.


Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having
WITH ORDINALITY and this feature, I would vote for removing
SRF-in-targetlist and call the release PostgreSQL 10.0.


  This should also
address the points that Josh brought up in discussion of ORDINALITY
regarding use of SRF-in-select to unnest multiple arrays.

(As a side issue, this patch also sets up pathkeys for ordinality along
the lines of a patch I suggested to Greg a while back in response to
his.)

Current patch status:

This is a first working cut: no docs, no tests, not enough comments, the
deparse logic probably needs more work (it deparses correctly but the
formatting may be suboptimal). However all the functionality is believed
to be in place.


With this last paragraph in mind, I am trying a little review.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Applies with some offsets on a few files but without fuzz.

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

No, as told by the patch author.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL spec says these:

In 7.6 table reference

table primary ::=
...
| table

Re: [HACKERS] Backup throttling


2013-07-31 22:50 keltezéssel, Antonin Houska írta:

On 07/31/2013 07:13 AM, Gibheer wrote:

Hi,

That is a really nice feature.

I don't pretend it's my idea, I just coded it. My boss proposed the feature as 
such :-)

I took a first look at your patch and some empty lines you added (e.g. line 60 
your patch). Can you remove
them?

Sure, will do in the next version.

Why did you move localGetCurrentTimestamp() into streamutil.c? Is sys/time.h 
still needed in receivelog.c after the move?
Because both receivelog.c and pg_basebackup.c need it now. I thought I could move 
localTimestampDifference() and localTimestampDifferenceExceeds() as well for the sake of 
consistency (these are actually utilities too) but I didn't get convinced enough that 
the feature alone justifies such a change.


As mentioned in 
http://www.postgresql.org/message-id/20130731173624.gx14...@eldon.alvh.no-ip.org these 
functions ideally shouldn't have separate implementation at all. However the problem is 
that pg_basebackup is not linked to the backend.


Have you considered the src/common directory?



You're right about sys/time.h, it's included via via streamutil.h. I'll fix 
that too.

I will try your patch later today to see, if it works.


Whenever you have time. Thanks!

// Tony


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/

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax


Hi,

mini-review follows.


2013-07-22 21:52 keltezéssel, Karol Trzcionka írta:

I've noticed problem with UPDATE ... FROM statement. Fix in new version.
Regards,
Karol


* Does it apply cleanly to the current git master?

No. There's a reject in src/backend/optimizer/plan/initsplan.c

* Does it include reasonable tests?

Yes but the test fails after trying to fix the rejected chunk of the patch.

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] Backup throttling


2013-08-19 19:20 keltezéssel, Andres Freund írta:

Hi,

On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:

Hello,
the purpose of this patch is to limit impact of pg_backup on running server.
Feedback is appreciated.

Based on a quick look it seems like you're throttling on the receiving
side. Is that a good idea? Especially over longer latency links, TCP
buffering will reduce the effect on the sender side considerably.

Greetings,

Andres Freund


Throttling on the sender side requires extending the syntax of
BASE_BACKUP and maybe START_REPLICATION so both can be
throttled but throttling is still initiated by the receiver side.

Maybe throttling the walsender is not a good idea, it can lead
to DoS via disk space shortage.

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


2013-08-19 20:03 keltezéssel, Josh Berkus írta:

On 08/19/2013 09:23 AM, Boszormenyi Zoltan wrote:

Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having
WITH ORDINALITY and this feature, I would vote for removing
SRF-in-targetlist and call the release PostgreSQL 10.0.

That's not realistic.


I know. I am such an agent provocateur. :-)


We'd have to deprecate that syntax and
repeatedly and loudly warn people about it for at least 3 years after
the release of 9.3.   You're talking about asking people to refactor
hundreds or thousands of lines of code which makes current use of things
like regex_match() in the target list.




--
--
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] [GENERAL] currval and DISCARD ALL


Hi,

2013-04-19 16:58 keltezéssel, Fabrízio de Royes Mello írta:


On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas robertmh...@gmail.com 
mailto:robertmh...@gmail.com wrote:


On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com mailto:fabriziome...@gmail.com wrote:
 The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
 if we decide to don't add a new subcommand to DISCARD, then its easier to
 modify the patch.

This patch is quite wrong.  It frees seqtab without clearing the
pointer, so the next reference will stomp on memory that may have been
reallocated.  And it doesn't even free seqtab correctly, since it only
frees the first node in the linked list.


Ohh sorry... you're all right... I completely forgot to finish the ReleaseSequenceCaches 
to transverse 'seqtab' linked list and free each node.


The attached patch have this correct code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello


I am reviewing your patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Almost. No rejects, no fuzz, only offset for some files.

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

Documentation, yes. Tests, no.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL standard doesn't have DISCARD.
Otherwise the behaviour is obvious.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

It changes applications' assumptions slightly but takes the
behaviour closer to the wording of the documentation.

* Have all the bases been covered?

Yes.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

No.

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines?

Yes.

Maybe a little stylistic comment:

+void
+ReleaseSequenceCaches()
+{
+   SeqTableData *ptr = seqtab;
+   SeqTableData *tmp = NULL;
+
+   while (ptr != NULL)
+   {
+   tmp = ptr;
+   ptr = ptr-next;
+   free(tmp);
+   }
+
+   seqtab = NULL;
+}

I would rename the variables to seq and next from
ptr and tmp, respectively, to make them even more
obvious. This looks a little better:

+void
+ReleaseSequenceCaches()
+{
+   SeqTableData *seq = seqtab;
+   SeqTableData *next;
+
+   while (seq)
+   {
+   next = seq-next;
+   free(seq);
+   seq = next;
+   }
+
+   seqtab = NULL;
+}

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. There are no extra system calls.

* Are the comments sufficient and accurate?

The feature needs very little code which is downright obvious.
There are no comments but I don't think the code needs it.

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

Only one:

src/backend/commands/sequence.c should have

#include commands/sequence.h

because of this:

sequence.c:1608:1: warning: no previous prototype for 'ReleaseSequenceCaches' 
[-Wmissing-prototypes]

 ReleaseSequenceCaches()
 ^

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other 
features/modules?

Yes.

* Are there interdependencies that can cause problems?

I don't think so.

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/

Re: [HACKERS] [GENERAL] currval and DISCARD ALL


2013-08-19 21:02 keltezéssel, Boszormenyi Zoltan írta:

Hi,

2013-04-19 16:58 keltezéssel, Fabrízio de Royes Mello írta:


On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas robertmh...@gmail.com 
mailto:robertmh...@gmail.com wrote:


On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com mailto:fabriziome...@gmail.com wrote:
 The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
 if we decide to don't add a new subcommand to DISCARD, then its easier to
 modify the patch.

This patch is quite wrong.  It frees seqtab without clearing the
pointer, so the next reference will stomp on memory that may have been
reallocated.  And it doesn't even free seqtab correctly, since it only
frees the first node in the linked list.


Ohh sorry... you're all right... I completely forgot to finish the 
ReleaseSequenceCaches to transverse 'seqtab' linked list and free each node.


The attached patch have this correct code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello


I am reviewing your patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Almost. No rejects, no fuzz, only offset for some files.

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

Documentation, yes. Tests, no.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL standard doesn't have DISCARD.
Otherwise the behaviour is obvious.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

It changes applications' assumptions slightly but takes the
behaviour closer to the wording of the documentation.

* Have all the bases been covered?

Yes.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

No.

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines?

Yes.

Maybe a little stylistic comment:

+void
+ReleaseSequenceCaches()
+{
+   SeqTableData *ptr = seqtab;
+   SeqTableData *tmp = NULL;
+
+   while (ptr != NULL)
+   {
+   tmp = ptr;
+   ptr = ptr-next;
+   free(tmp);
+   }
+
+   seqtab = NULL;
+}

I would rename the variables to seq and next from
ptr and tmp, respectively, to make them even more
obvious. This looks a little better:

+void
+ReleaseSequenceCaches()
+{
+   SeqTableData *seq = seqtab;
+   SeqTableData *next;
+
+   while (seq)
+   {
+   next = seq-next;
+   free(seq);
+   seq = next;
+   }
+
+   seqtab = NULL;
+}

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. There are no extra system calls.

* Are the comments sufficient and accurate?

The feature needs very little code which is downright obvious.
There are no comments but I don't think the code needs it.

* Does it do what it says, correctly?

Yes.


I lied.

There is one little problem. There is no command tag
reported for DISCARD SEQUENCES:

zozo=# create sequence s1;
CREATE SEQUENCE
zozo=# select nextval('s1');
 nextval
-
   1
(1 row)

zozo=# select currval('s1');
 currval
-
   1
(1 row)

zozo=# discard all;
DISCARD ALL
zozo=# discard sequences;
???
zozo=# select currval('s1');
ERROR:  currval of sequence s1 is not yet defined in this session



* Does it produce compiler warnings?

Only one:

src/backend/commands/sequence.c should have

#include commands/sequence.h

because of this:

sequence.c:1608:1: warning: no previous prototype for 'ReleaseSequenceCaches' 
[-Wmissing-prototypes]

 ReleaseSequenceCaches()
 ^

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other 
features/modules?

Yes.

* Are there interdependencies that can cause problems?

I don't think so.

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/



--
--
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/

Re: [HACKERS] Backup throttling


2013-08-19 21:11 keltezéssel, Andres Freund írta:

On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote:

2013-08-19 19:20 keltezéssel, Andres Freund írta:

Hi,

On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:

Hello,
the purpose of this patch is to limit impact of pg_backup on running server.
Feedback is appreciated.

Based on a quick look it seems like you're throttling on the receiving
side. Is that a good idea? Especially over longer latency links, TCP
buffering will reduce the effect on the sender side considerably.

Throttling on the sender side requires extending the syntax of
BASE_BACKUP and maybe START_REPLICATION so both can be
throttled but throttling is still initiated by the receiver side.

Seems fine to me. Under the premise that the idea is decided to be
worthwile to be integrated. Which I am not yet convinced of.


Maybe throttling the walsender is not a good idea, it can lead
to DoS via disk space shortage.

Not in a measurably different way than receiver side throttling?


No, but that's not what I meant.

START_BACKUP has to deal with big data but it finishes after some time.
But  pg_receivexlog may sit there indefinitely...

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] GSOC13 proposal - extend RETURNING syntax


2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:

W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:

* Does it apply cleanly to the current git master?

No. There's a reject in src/backend/optimizer/plan/initsplan.c

Thank you, merged in attached version.

* Does it include reasonable tests?

Yes but the test fails after trying to fix the rejected chunk of the
patch.

It fails because the HINT was changed, fixed.
That version merges some nested ifs left over from earlier work.


I tried to compile your v5 patch and I got:

initsplan.c: In function ‘add_vars_to_targetlist’:
initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]

rel-reltargetlist = lappend(rel-reltargetlist,
^

You shouldn't change the assignment at declaration:

- RelOptInfo *rel = find_base_rel(root, var-varno);
+ RelOptInfo *rel;
...
+ if (root-parse-commandType == CMD_UPDATE)
+ {
... (code using rel)
+ }
+ rel = find_base_rel(root, varno);

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


Regards,
Karol Trzcionka



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


2013-08-19 22:04 keltezéssel, Andrew Gierth írta:

Boszormenyi Zoltan wrote:

This parser hackery is of course somewhat ugly. But given the objective
of implementing the spec's unnest syntax, it seems to be the least ugly
of the possible approaches. (The hard part of doing it any other way
would be generating the description of the result type; composite array
parameters expand into multiple result columns.)

Harder maybe but it may still be cleaner in the long run.

I'm not so sure.

As far as I'm concerned, though, the situation is fairly simple: there
are no proposals on the table for any mechanism that would allow the
deduction of a return type structure for multi-arg unnest, I have
tried and failed to come up with a usable alternative proposal, and
there is no prospect of one resulting from any other work that I know
about. So the parser hack is the way it goes, and anyone who doesn't
like it is welcome to suggest a better idea.


Overall, it's my intention here to remove as many as feasible of the old
reasons why one might use an SRF in the select list.

Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having
WITH ORDINALITY and this feature, I would vote for removing
SRF-in-targetlist and call the release PostgreSQL 10.0.

I want to make this ABSOLUTELY clear: I am not advocating removing
SRF-in-targetlist in the near future and I will not support anyone who
does. Please do not use this code as an argument for that (at least
until a few releases have elapsed). All I'm interested in at this
point is providing an alternative with better semantics.


The SQL spec says these:

In 7.6 table reference

[mega-snip]

As far as I can tell, these should also be allowed but isn't:

No, they're not allowed. You missed this rule (in 7.6 Syntax Rules):

2) If a table primary TP simply contains a table function derived
table TFDT, then:

a) The collection value expression immediately contained in TFDT
   shall be a routine invocation.

In other words, func(...) is the only allowed form for the collection
value expression inside TABLE( ).  (Same rule exists in 201x, but
numbered 6 rather than 2.)


You are right, I missed it in the standard. Sorry.


Largely as a matter of consistency, the patch does presently allow
expressions that are not routine invocations but which are part of
the func_expr_windowless production, so things like TABLE(USER) work.
(This is because historically these are allowed in the FROM clause as
tables.) I'm not sure this is a good idea in general; should it be
tightened up to only allow func_application?


* If it claims to improve performance, does it?

It certainly improves writing queries, as functions inside
unnest() get processed in one scan.

I'm not making any specific performance claims, but I have tested it
against the idea of doing separate function scans with a full join on
ordinality columns, and my approach is faster (1.5x to 2x in my tests)
even with pathkey support and with elimination of extra materialize
nodes (by allowing mark/restore in FunctionScan).

--

Since the original patch was posted I have done further work on it,
including some tests. I have also come up with an additional
possibility: that of allowing multiple SRFs that return RECORD with
column definition lists, and SRFs-returning-RECORD combined with
ORDINALITY, by extending the syntax further:

select * from TABLE(func1() AS (a text, b integer),
 func2() AS (c integer, d text));

select * from TABLE(func1() AS (a text, b integer))
   WITH ORDINALITY AS f(a,b,o);

-- shame to have to duplicate the column names, but avoiding that would
-- not have been easy

This removes the restriction of the previous ORDINALITY patch that
prevented its use with SRFs that needed coldef lists.


Very nice.


I'm open to other suggestions on the syntax for this.


It's consistent with straight SRFs in FROM, the function
parameters are attached to the functions themselves.
I think it's good as is.


(My implementation of this works by making the column definition list
a property of the function call,


Which also makes it easier on the eyes and brain when reading
someone else's SQL.


  rather than of the RTE or the
FunctionScan node.  This eliminates a few places where TYPEFUNC_RECORD
had to be handled as a special case.)


This is the other plus. No special casing is good.

The only minus is that you haven't attached the new patch. :-)

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

New ECPG idea, was: Re: [HACKERS] ECPG FETCH readahead

2013-08-17 Thread Boszormenyi Zoltan


2013-08-17 12:08 keltezéssel, Boszormenyi Zoltan írta:

Hi,

I am restarting this old thread... :-)

2012-04-24 10:17 keltezéssel, Michael Meskes írta:

OK, I will implement #2. Another question popped up: what to do
with FETCH ALL? The current readahead window size or temporarily
bumping it to say some tens of thousands can be used. We may not
know how much is the all records. This, although lowers performance,
saves memory.

I would say doing a large fetch in two or three batches won't cost too much in
terms of performance.


Please, don't apply this patch yet. I discovered a rather big hole
that can confuse the cursor position tracking if you do this:
...
That will also need a new round of review. Sorry for that.

No problem, better to find it now instead of after release.

Anyway, I moved the patch to 2012-next (I hope I did it correctly) so 2012-1
can be closed. Let's try to get this patch done in the next commit fest.

Michael


I had time to look into this patch of mine again after about 1.5 years.
Frankly, this time was too long to remember every detail of the patch
and looking at parts of the patch as a big entity was confusing.

So I started fresh and to make review easier, I broke the patch up
into small pieces that all build on each other. I have also fixed quite
a few bugs, mostly in my code, but some in the ECPG parser and
the regression tests as well.

I have put the broken up patchset into a GIT tree of mine at GitHub:
https://github.com/zboszor/ecpg-readahead/
but the huge compressed patch is also attached for reference.
It was generated with

$ git diff 
221e92f64c6e136e550ec2592aac3ae0d4623209..870922676e6ae0faa4ebbf94b92e0b97ec418e16


ECPG regression tests are now Valgrind-clean except two of them
but both are pre-existing bugs.

1. ecpg/test/compat_informix/rfmtlong.pgc points out a problem in
   ecpg/compatlib/informix.c

==5036== 1 errors in context 1 of 4:
==5036== Invalid read of size 4
==5036==at 0x4E3453C: rfmtlong (informix.c:941)
==5036==by 0x4007DA: fmtlong.constprop.0 (rfmtlong.pgc:22)
==5036==by 0x4006BE: main (rfmtlong.pgc:45)
==5036==  Address 0x60677d8 is 24 bytes inside a block of size 25 alloc'd
==5036==at 0x4C28409: malloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==5036==by 0x4E34268: rfmtlong (informix.c:783)
==5036==by 0x4007DA: fmtlong.constprop.0 (rfmtlong.pgc:22)
==5036==by 0x4006BE: main (rfmtlong.pgc:45)

The same error is reported 4 times.

2. ecpg_add_mem() seems to leak memory:

==5463== 256 bytes in 16 blocks are definitely lost in loss record 1 of 1
==5463==at 0x4C2A121: calloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==5463==by 0x4E3E153: ecpg_alloc (memory.c:21)
==5463==by 0x4E3E212: ecpg_add_mem (memory.c:110)
==5463==by 0x4E3542B: ecpg_store_result (execute.c:409)
==5463==by 0x4E37E5A: ecpg_process_output (execute.c:1777)
==5463==by 0x4E38CCA: ecpg_do (execute.c:2137)
==5463==by 0x4E38D8A: ECPGdo (execute.c:2159)
==5463==by 0x400A82: fn (alloc.pgc:51)
==5463==by 0x5152C52: start_thread (pthread_create.c:308)
==5463==by 0x545C13C: clone (clone.S:113)

The last two issue we talked about in this thread are also implemented:
- permanently raise the readahead window if the application sends a
  bigger FETCH command, and
- temporarily raise the readahead window for FETCH ALL commands

The cursor position tracking was completely rewritten, so the client side
properly follows the cursor position known by the backend and doesn't
skip MOVE statements where it shouldn't. The previously known
bug is completely eliminated this way.

Please, review that patch.


I have another idea to make ECPG building on this huge patch.

Currently, UPDATE/DELETE WHERE CURRENT OF has to issue a MOVE
before the command in case the cursor positions known by the application
and the backend are different.

My idea builds on the fact that UPDATE/DELETE RETURNING is present
in all supported back branches.

A mini-parser only understanding SELECT, UPDATE and DELETE should
be added to ecpglib, so

DECLARE cursor CURSOR FOR SELECT ...

and

PREPARE prepared_stmt FROM :query;
DECLARE cursor CURSOR FOR prepared_stmt;

can be analyzed and tweaked behind the application's back.

This is needed to detect whether a query is a simple updatable
scan of a table, and returning errors early to the application if it's not,
without actually sending the UPDATE/DELETE WHERE CURRENT OF
query to the backend.

For the purpose of WHERE CURRENT OF, I would add a ctid
column at the end of the targelist that is treated like resjunk
in the backend when returning data to the application.

So, SELECTs would return the ctid information of the tuples.
The cursor query was a FETCH N with abs(N)1 because of
the readahead. For this reason, the cursor positions known
by the application and the backend are different.

The extra MOVE can be eliminated by replacing

UPDATE table SET ... WHERE CURRENT

Re: [HACKERS] Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-06 Thread Boszormenyi Zoltan


2013-08-05 16:01 keltezéssel, Stephen Frost írta:

* Greg Stark (st...@mit.edu) wrote:

On Fri, Aug 2, 2013 at 4:05 PM, Stephen Frost sfr...@snowman.net wrote:

I'm not even clear we do want this in /etc since none of our GUC
options are repeatable things like Apache virtual servers. It actually
makes *more* sense for pg_hba than it does for gucs. I think we can
assume that in the future we'll have something like it however.

I tend to agree with this also, though I can imagine wanting to separate
things in a conf.d directory ala exim's conf.d directories, to allow
tools like puppet to manage certain things environment-wide (perhaps
krb_server_keyfile) while other configuration options are managed
locally.

Extensions are actually a pretty good argument for why conf.d in /etc
(or wherever the non-auto-config is) is pretty important useful.
That's the kind of thing conf.d directories are meant for. A user can
install a package containing an extension and the extension would
automatically drop in the config entries needed in that directory.

Agreed, though I think there should be a difference between shared
library load being added-to for extensions, and random
extension-specific GUC..


Now that you mention shared library load, it may be a good idea
to add an append-to-this-GUC flag instead of overwriting the
previous value. Two GUCs may make use of it: shared_preload_libraries
and local_preload_libraries. It would make packagers' of extensions
and DBA's lives easier.

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



Thanks,

Stephen



--
--
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] Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-06 Thread Boszormenyi Zoltan


2013-08-06 19:41 keltezéssel, Bruce Momjian írta:

On Tue, Aug  6, 2013 at 06:34:35PM +0200, Boszormenyi Zoltan wrote:

2013-08-05 16:01 keltezéssel, Stephen Frost írta:

* Greg Stark (st...@mit.edu) wrote:

On Fri, Aug 2, 2013 at 4:05 PM, Stephen Frost sfr...@snowman.net wrote:

I'm not even clear we do want this in /etc since none of our GUC
options are repeatable things like Apache virtual servers. It actually
makes *more* sense for pg_hba than it does for gucs. I think we can
assume that in the future we'll have something like it however.

I tend to agree with this also, though I can imagine wanting to separate
things in a conf.d directory ala exim's conf.d directories, to allow
tools like puppet to manage certain things environment-wide (perhaps
krb_server_keyfile) while other configuration options are managed
locally.

Extensions are actually a pretty good argument for why conf.d in /etc
(or wherever the non-auto-config is) is pretty important useful.
That's the kind of thing conf.d directories are meant for. A user can
install a package containing an extension and the extension would
automatically drop in the config entries needed in that directory.

Agreed, though I think there should be a difference between shared
library load being added-to for extensions, and random
extension-specific GUC..

Now that you mention shared library load, it may be a good idea
to add an append-to-this-GUC flag instead of overwriting the
previous value. Two GUCs may make use of it: shared_preload_libraries
and local_preload_libraries. It would make packagers' of extensions
and DBA's lives easier.

'search_path' might also use it, though we might need append and
prepend.


Indeed. Although I was thinking along the lines of the GUC parser, so:

shared_preload_library += 'some_new_lib'

would append to the old value.

Maybe += is intuitive enough for prepending, or someone may
come up with a better idea.

But the extra flag would still be needed to indicate the GUC is a list,
so these new operators are usable on them and not on regular GUCs.

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] Error code returned by lock_timeout

2013-06-27 Thread Boszormenyi Zoltan


2013-06-27 17:03 keltezéssel, Fujii Masao írta:

On Thu, Jun 27, 2013 at 2:22 PM, Boszormenyi Zoltan z...@cybertec.at wrote:

Hi,

I just realized that in the original incarnation of lock_timeout,
I used ERRCODE_LOCK_NOT_AVAILABLE (to be consistent with NOWAIT)
but the patch that was accepted into 9.3 contained ERRCODE_QUERY_CANCELED
which is the same as for statement_timeout.

Which would be more correct?

ISTM that ERRCODE_LOCK_NOT_AVAILABLE is more suitable...


I think, too. Can someone commit this one-liner to master and 9.3?

Best regards,
Zoltán Bsö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/

--- src/backend/tcop/postgres.c~	2013-06-27 07:05:08.0 +0200
+++ src/backend/tcop/postgres.c	2013-06-27 17:10:28.040972642 +0200
@@ -2900,7 +2900,7 @@
 			DisableNotifyInterrupt();
 			DisableCatchupInterrupt();
 			ereport(ERROR,
-	(errcode(ERRCODE_QUERY_CANCELED),
+	(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 	 errmsg(canceling statement due to lock timeout)));
 		}
 		if (get_timeout_indicator(STATEMENT_TIMEOUT, true))

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

[HACKERS] Error code returned by lock_timeout

2013-06-26 Thread Boszormenyi Zoltan


Hi,

I just realized that in the original incarnation of lock_timeout,
I used ERRCODE_LOCK_NOT_AVAILABLE (to be consistent with NOWAIT)
but the patch that was accepted into 9.3 contained ERRCODE_QUERY_CANCELED
which is the same as for statement_timeout.

Which would be more correct?

Thanks in advance,
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/

Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-06-18 Thread Boszormenyi Zoltan


Hi,

review below.

2013-06-13 14:35 keltezéssel, Amit Kapila írta:

On Friday, June 07, 2013 9:45 AM Amit Kapila wrote:

On Thursday, June 06, 2013 10:22 PM Robert Haas wrote:

On Wed, Jun 5, 2013 at 7:24 AM, Amit Kapila amit.kap...@huawei.com
wrote:

On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:

On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:

On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:

There are 2 options to proceed for this patch for 9.4

1. Upload the SET PERSISTENT syntax patch for coming CF by fixing

existing

review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail

Could you suggest me what could be best way to proceed for this

patch?

I'm still in favor of some syntax involving ALTER, because it's still
true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)

rather

the ones that use SET (which change the current settings for some
period of time).


I will change the patch as per below syntax if there are no objections:

  ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};

Modified patch contains:

1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};

If user specifies DEFAULT, it will remove entry from auto conf file.

2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf

3. Added a new page for Alter System command in docs. In compatability
section, I had mentioned that
this statement is an PostgresQL extension. I had tried to search in
SQL-92 spec but couldn't find such command.

4. During test of this patch, I had observed one problem for PGC_BACKEND
parameters like log_connections.
If I change these parameters directly in postgresql.conf and then do
pg_reload_conf() and reconnect, it will
still show the old value. This is observed only on WINDOWS. I will
investigate this problem and update you.
Due to this problem, I had removed few test cases.


I can't reproduce this error under Linux (Fedora 19/x86_64).

The bug might be somewhere else and not caused by your patch
if manually adding/removing log_connections = on from postgresql.conf
exhibits the same behaviour under Windows. (If I read it correctly,
you tested it exactly this way.) Does the current GIT version behave
the same way?



Kindly let me know your suggestions.

With Regards,
Amit Kapila.


* Is the patch in a patch format which has context?

Yes.

* Does it apply cleanly to the current git master?

Yes.

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

Yes.

Read what the patch is supposed to do, and consider:

* Does the patch actually implement that?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

No such spec, follows the agreed behaviour.

* Does it include pg_dump support (if applicable)?

N/A

* Are there dangers?

No.

* Have all the bases been covered?

According to the above note under Windows, not yet.

The name persistent.auto.conf is not yet set in stone.
postgresql.auto.conf might be better.

Apply the patch, compile it and test:

* Does the feature work as advertised?

Yes, with the noted exception above for log_connections.

* Are there corner cases the author has failed to consider?

I can't see any. It is through

* Are there any assertion failures or crashes?

No.

* 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.

* Does it follow the project coding guidelines 
http://developer.postgresql.org/pgdocs/postgres/source.html?


Mostly, yes. Some nits, though. At some places, you do:

- ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail);
+ ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail,NULL);

without a space between the last comma and the NULL keyword.

Also, in the comment above ParseConfigFile() there are unnecessary
white space changes and spaces at the end of the line.

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. It doesn't use any platform-dependent code.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

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/

Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-06-18 Thread Boszormenyi Zoltan

2013-06-14 05:12 keltezéssel, Amit Kapila írta:

On Friday, June 14, 2013 3:17 AM Josh Berkus wrote:

On 06/13/2013 05:35 AM, Amit Kapila wrote:

On Friday, June 07, 2013 9:45 AM Amit Kapila wrote:

On Thursday, June 06, 2013 10:22 PM Robert Haas wrote:

On Wed, Jun 5, 2013 at 7:24 AM, Amit Kapila

amit.kap...@huawei.com

wrote:

On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:

On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:

On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:

There are 2 options to proceed for this patch for 9.4

1. Upload the SET PERSISTENT syntax patch for coming CF by fixing

existing

review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail

Could you suggest me what could be best way to proceed for this

patch?

I'm still in favor of some syntax involving ALTER, because it's

still

true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)

rather

the ones that use SET (which change the current settings for some
period of time).

I will change the patch as per below syntax if there are no

objections:

ALTER SYSTEM SET configuration_parameter {TO | =} {value, |

'value'};

Modified patch contains:

1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};

If user specifies DEFAULT, it will remove entry from auto conf file.

2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf

Why? Shouldn't it just be auto.conf? Or system.auto.conf?

I had kept same name as it was decided for 9.3, but now as command has
changed so it makes more
sense to change name as well. I think it can be one of what you suggested or
postgresql.auto.conf.

I prefer auto.conf, personally.

Thanks. if no body has any other suggestion I will change it.

I think one of system.auto.conf or postgresql.auto.conf is more appropriate because it either resembles command or existing configuration settings

file.

I like postgresql.auto.conf better.

With Regards,
Amit Kapila.

--
--
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-06-18 Thread Boszormenyi Zoltan


2013-06-18 14:11 keltezéssel, Amit Kapila írta:

On Tuesday, June 18, 2013 3:26 PM Boszormenyi Zoltan wrote:

Hi,
review below.

   Thanks for the review.



There are 2 options to proceed for this patch for 9.4
1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
existing
review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail
Could you suggest me what could be best way to proceed for this
patch?

I'm still in favor of some syntax involving ALTER, because it's still
true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)
rather
the ones that use SET (which change the current settings for some
period of time).



I will change the patch as per below syntax if there are no objections:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};

Modified patch contains:
1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};
If user specifies DEFAULT, it will remove entry from auto conf file.
2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf
3. Added a new page for Alter System command in docs. In compatability
section, I had mentioned that
   this statement is an PostgresQL extension. I had tried to search in
SQL-92 spec but couldn't find such command.
4. During test of this patch, I had observed one problem for PGC_BACKEND
parameters like log_connections.

If I change these parameters directly in postgresql.conf and then do

pg_reload_conf() and reconnect, it will
   still show the old value. This is observed only on WINDOWS. I will
investigate this problem and update you.
   Due to this problem, I had removed few test cases.

I can't reproduce this error under Linux (Fedora 19/x86_64).
The bug might be somewhere else and not caused by your patch
if manually adding/removing log_connections = on from postgresql.conf
exhibits the same behaviour under Windows. (If I read it correctly,
you tested it exactly this way.) Does the current GIT version behave
the same way?

Yes, the current Git has problem which I had reported in below mail:

http://www.postgresql.org/message-id/009801ce68f7$3a746340$af5d29c0$@kapila@
huawei.com

This problem is not related to this patch, it occurs only on WINDOWS or
under EXEC_BACKEND flag.
I think we can discuss this problem in above mail thread.




* Have all the bases been covered?
According to the above note under Windows, not yet.
The name persistent.auto.conf is not yet set in stone.
postgresql.auto.conf might be better.

I think, the decision of name, we can leave to committer with below
possibilities,
as it is very difficult to build consensus on any particular name.

Auto.conf
System.auto.conf
Postgresql.auto.conf
Persistent.auto.conf


Apply the patch, compile it and test:



* Does it follow the project coding guidelines?
Mostly, yes. Some nits, though. At some places, you do:
- ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail);
+ ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head,

tail,NULL);

I think you mean ParseConfigFp()?


Sorry, yes.



without a space between the last comma and the NULL keyword.


Also, in the comment above ParseConfigFile() there are unnecessary
white space changes and spaces at the end of the line.

Do you mean to say whitespaces in below text?

!  * and absolute-ifying the path name if necessary.

!  *
!  * While parsing, it records if it has parsed persistent.auto.conf file.
!  * This information can be used by the callers to ensure if the parameters
!  * set by ALTER SYSTEM SET command will be effective.
*/


Yes.

Anyway, both would be fixed by a pgindent run. (I think.)



With Regards,
Amit Kapila.




--
--
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] Re: pg_basebackup with -R option and start standby have problems with escaped password

2013-05-17 Thread Boszormenyi Zoltan

2013-05-17 16:05 keltezéssel, Heikki Linnakangas írta:

On 18.02.2013 16:35, Boszormenyi Zoltan wrote:

2013-01-29 11:15 keltezéssel, Magnus Hagander írta:

On Thu, Jan 24, 2013 at 7:04 AM, Hari Babu haribabu.ko...@huawei.com
wrote:

On Wed, Jan 23, 2013 11:48 PM, Magnus Hagander wrote:

On Wed, Jan 23, 2013 at 10:18 AM, Hari Babu haribabu.ko...@huawei.com

wrote:

Test scenario to reproduce:
1. Start the server
2. create the user as follows
./psql postgres -c create user user1 superuser login
password 'use''1'

3. Take the backup with -R option as follows.
./pg_basebackup -D ../../data1 -R -U user1 -W

The following errors are occurring when the new standby on the backup
database starts.

FATAL: could not connect to the primary server: missing = after 1'

connection info string

What does the resulting recovery.conf file look like?

The recovery.conf which is generated is as follows

standby_mode = 'on'
primary_conninfo = 'user=''user1'' password=''use''1'' port=''5432'' '

I observed the problem is while reading primary_conninfo from the
recovery.conf file
the function GUC_scanstr removes the quotes of the string and also
makes
the
continuos double quote('') as single quote(').

By using the same connection string while connecting to primary
server the
function conninfo_parse the escape quotes are not able to parse
properly
and it is leading
to problem.

please correct me if any thing wrong in my observation.

Well, it's clearly broken at least :O

Zoltan, do you have time to look at it? I won't have time until at
least after FOSDEM, unfortunately.

I looked at it shortly. What I tried first is adding another pair of single
quotes manually like this:

primary_conninfo = 'user=''user1'' password=''use1''
host=''192.168.1.2'' port=''5432'' sslmode=''disable''
sslcompression=''1'' '

But it doesn't solve the problem either, I got:

FATAL: could not connect to the primary server: missing = after '1'
in connection info string

This worked though:

primary_conninfo = 'user=user1 password=use\'1 host=192.168.1.2
port=5432 sslmode=disable sslcompression=1 '

When I added an elog() to print the conninfo string in libpqrcv_connect(),
I saw that the double quotes were properly eliminated by ParseConfigFp()
in the first case.

So, there is a bug in generating recovery.conf by not double-escaping
the values and another bug in parsing the connection string in libpq
when the parameter value starts with a single-quote character.

No, the libpq connection string parser is working as intended. Per the docs on
PQconnectdb:

The passed string can be empty to use all default parameters, or it
can contain one or more parameter settings separated by whitespace.
Each parameter setting is in the form keyword = value. Spaces around
the equal sign are optional. To write an empty value, or a value
containing spaces, surround it with single quotes, e.g., keyword = 'a
value'. Single quotes and backslashes within the value must be
escaped with a backslash, i.e., \' and \\.

So, the proper way to escape a quote in a libpq connection string is \', not ''. There
are two escaping systems layered on top of each other; the recovery.conf parser's, where
you use '', and the libpq system, where you use \'. So we need two different escaping
functions in pg_basebackup to get this right.

Why is extending the libpq parser to allow doubling the single
quotes not a good solution? It would be consistent in this regard
with the recovery.conf parser. From this POV the first patch only
needs a little change in the libpq docs.

Also, my patch for libpq only changed the libpq parser when the
parameter argument starts with \'. So in that state the doubled
single quotes should be an accepted escaping in the middle of
the string, too. If you look at the code, there are two branches
that process the argument differently depending on whether
it starts with a \' or not.

Apart from that, does it bother anyone else that the the primary_conninfo line that
pg_basebackup creates is butt-ugly?

primary_conninfo = 'user=''heikki'' host=''localhost'' port=''5432'' sslmode=''prefer''
sslcompression=''1'' '

Not a single bit. It's machine generated and the code is generic.
The parser can deal with it.

We can't avoid quoting option values that need it, but that's probably very rare in
practice. I think we should work a bit harder and leave out the quotes where not necessary.

Maybe. :-)

Also, do we really need to include the ssl options when they are the defaults?

I think we were through about this bullet point. IIRC the reasoning and
the consensus was that the backup and the generated recovery.conf
should also work on another machine with a possibly different set of
compilation options for libpq and envvars present on the system.

I think the attached patch fixes the original test scenario correctly, without changing
libpq's quoting rules, and only quotes when necessary. I didn't do anything about the
ssl options. Please take

Re: [HACKERS] commit fest schedule for 9.4

2013-05-15 Thread Boszormenyi Zoltan


2013-05-15 20:05 keltezéssel, Andrew Dunstan írta:


On 05/15/2013 02:00 PM, Josh Berkus wrote:

Obviously we need a meta-manager who makes sure we have managers, and is
able to notice that a manager is MIA and needs replaced (or at least
backed-up).

And then a meta-meta-manager to make sure that the meta-manager is
meta-managing.

And an Inspector General.  Anyone have Danny Kaye's phone number?



Or Gogol's?


You have to learn the dialing chant to call them... ;-)



cheers

andrew





--
--
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] 9.3 Beta1 status report

2013-04-22 Thread Boszormenyi Zoltan


2013-04-21 15:10 keltezéssel, Bruce Momjian írta:

On Sun, Apr 21, 2013 at 09:34:10AM +0200, Boszormenyi Zoltan wrote:

2013-04-21 07:02 keltezéssel, Bruce Momjian írta:

I am not sure if Tom shared yet, but we are planning to package 9.3
beta1 on April 29, with a release on May 2.  Those dates might change,
but that is the current plan.  I have completed a draft 9.3 release
notes, which you can view here:

http://momjian.us/pgsql_docs/release-9-3.html

I will be working on polishing them for the next ten days, so any
feedback, patches, or commits are welcome.  I still need to add lots of
SGML markup.

How comes Álvaro's name comes out right in your page but not at
http://www.postgresql.org/docs/devel/static/release-9-3.html ?

Anyway, I attached a patch to fix my name in your page using markups.

Thanks, applied.


Thank you.


   I had not yet gotten to doing the SGML markup for
non-ASCII characters.




--
--
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] Draft release notes up for review

2013-04-21 Thread Boszormenyi Zoltan


2013-03-29 02:46 keltezéssel, Tom Lane írta:

Since there has been some, um, grumbling about the quality of the
release notes of late, I've prepared draft notes for next week's
releases, covering commits through today.  These are now committed
into the master branch for review, and should show up at
http://www.postgresql.org/docs/devel/static/
after guaibasaurus' next buildfarm run, about three hours from now.

Please comment if you find anything that could be improved.


The sgml converter seems to choke on the UTF-8 characters that my name contains:

Add configuration variable lock_timeout to limit lock wait duration (ZoltÃ¡n 
BÃ¶szÃ¶rmÃ©nyi)

I don't mind if my name is written without the accented characters like in the 
other entry:

Have pg_basebackup --write-recovery-conf output a minimal recovery.conf (Zoltan 
Boszormenyi, Magnus Hagander)


Alternatively the sgml tool can be taught to emit proper HTML accented 
characters
or my name can be written as Zoltaacute;n Bouml;szouml;rmeacute;nyi

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



regards, tom lane





--
--
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] Draft release notes up for review

2013-04-21 Thread Boszormenyi Zoltan


2013-04-21 08:28 keltezéssel, Boszormenyi Zoltan írta:

2013-03-29 02:46 keltezéssel, Tom Lane írta:

Since there has been some, um, grumbling about the quality of the
release notes of late, I've prepared draft notes for next week's
releases, covering commits through today.  These are now committed
into the master branch for review, and should show up at
http://www.postgresql.org/docs/devel/static/
after guaibasaurus' next buildfarm run, about three hours from now.

Please comment if you find anything that could be improved.


The sgml converter seems to choke on the UTF-8 characters that my name contains:

Add configuration variable lock_timeout to limit lock wait duration (ZoltÃ¡n 
BÃ¶szÃ¶rmÃ©nyi)


I don't mind if my name is written without the accented characters like in the 
other entry:

Have pg_basebackup --write-recovery-conf output a minimal recovery.conf (Zoltan 
Boszormenyi, Magnus Hagander)


Alternatively the sgml tool can be taught to emit proper HTML accented 
characters
or my name can be written as Zoltaacute;n Bouml;szouml;rmeacute;nyi


doc/src/sgml/release.sgml suggest using the last one but when I looked at
release-9.3, I saw (AlvaroAacute;lvaro Herrera) in the webpage several times
where the sgml contains (Aacute;lvaro Herrera), so it's not bulletproof 
either.



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



regards, tom lane








--
--
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] 9.3 Beta1 status report

2013-04-21 Thread Boszormenyi Zoltan


2013-04-21 07:02 keltezéssel, Bruce Momjian írta:

I am not sure if Tom shared yet, but we are planning to package 9.3
beta1 on April 29, with a release on May 2.  Those dates might change,
but that is the current plan.  I have completed a draft 9.3 release
notes, which you can view here:

http://momjian.us/pgsql_docs/release-9-3.html

I will be working on polishing them for the next ten days, so any
feedback, patches, or commits are welcome.  I still need to add lots of
SGML markup.


How comes Álvaro's name comes out right in your page but not at
http://www.postgresql.org/docs/devel/static/release-9-3.html ?

Anyway, I attached a patch to fix my name in your page using markups.

Thanks,
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/

diff --git a/doc/src/sgml/release-9.3.sgml b/doc/src/sgml/release-9.3.sgml
index 76ba128..2927272 100644
--- a/doc/src/sgml/release-9.3.sgml
+++ b/doc/src/sgml/release-9.3.sgml
@@ -367,7 +367,7 @@
   listitem
para
 Add configuration variable lock_timeout to limit lock wait duration
-(ZoltÃ¡n BÃ¶szÃ¶rmÃ©nyi)
+(Zoltaacute;n Bouml;szouml;rmeacute;nyi)
/para
   /listitem
 
@@ -488,7 +488,7 @@
   listitem
para
 Have pg_basebackup --write-recovery-conf output a minimal
-recovery.conf (Zoltan Boszormenyi, Magnus Hagander)
+recovery.conf (Zoltaacute;n Bouml;szouml;rmeacute;nyi, Magnus Hagander)
/para
 
para
@@ -1357,7 +1357,7 @@
 
   listitem
para
-Create a centralized timeout API (ZoltÃ¡n BÃ¶szÃ¶rmÃ©nyi)
+Create a centralized timeout API (Zoltaacute;n Bouml;szouml;rmeacute;nyi)
/para
   /listitem
 

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

Re: [HACKERS] Inconsistent DB data in Streaming Replication

2013-04-10 Thread Boszormenyi Zoltan


2013-04-10 18:46 keltezéssel, Fujii Masao írta:

On Wed, Apr 10, 2013 at 11:16 PM, Andres Freund and...@2ndquadrant.com wrote:

On 2013-04-10 10:10:31 -0400, Tom Lane wrote:

Amit Kapila amit.kap...@huawei.com writes:

On Wednesday, April 10, 2013 3:42 PM Samrat Revagade wrote:

Sorry, this is incorrect. Streaming replication continuous, master is not
waiting, whenever the master writes the data page it checks that the WAL
record is written in standby till that LSN.

I am not sure it will resolve the problem completely as your old-master can
have some WAL extra then new-master for same timeline. I don't remember
exactly will timeline switch feature
take care of this extra WAL, Heikki can confirm this point?
Also I think this can serialize flush of data pages in checkpoint/bgwriter
which is currently not the case.

Yeah.  TBH this entire discussion seems to be let's cripple performance
in the normal case so that we can skip doing an rsync when resurrecting
a crashed, failed-over master.  This is not merely optimizing for the
wrong thing, it's positively hazardous.  After a fail-over, you should
be wondering whether it's safe to resurrect the old master at all, not
about how fast you can bring it back up without validating its data.
IOW, I wouldn't consider skipping the rsync even if I had a feature
like this.

Agreed. Especially as in situations where you fall over in a planned
way, e.g. for a hardware upgrade, you can avoid the need to resync with
a littlebit of care.

It's really worth documenting that way.


So its mostly in catastrophic situations this
becomes a problem and in those you really should resync - and its a good
idea not to use a normal rsync but a rsync --checksum or similar.

If database is very large, rsync --checksum takes very long. And I'm concerned
that most of data pages in master has the different checksum from those in the
standby because of commit hint bit. I'm not sure how rsync --checksum can
speed up the backup after failover.


rsync --checksum alone may not but rsync --inplace may speed up backup a 
lot.



Regards,




--
--
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: [PATCH] Exorcise zero-dimensional arrays (Was: Re: [HACKERS] Should array_length() Return NULL)

2013-04-04 Thread Boszormenyi Zoltan


2013-04-03 20:58 keltezéssel, Gavin Flower írta:

On 04/04/13 05:36, David E. Wheeler wrote:

On Apr 3, 2013, at 9:30 AM, Tom Lanet...@sss.pgh.pa.us  wrote:


Fortran ... Basic ... actually I'd have thought that zero was a
minority position.  Fashions change I guess.

I say we turn the default lower bound up to 11.

David


In keeping with the level of irrationality in this thread, maybe we should set it to an 
irrational number like the square root of 2, or transcend our selves and make in a 
transcendental number like pi!  :-)


I suppose using the square root of minus one would be consider too 
imaginative???  :-)


Nah, that would make arrays have 2 dimensions as a minimum... :-)




Cheers,
Gavin



--
--
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/

Re: [HACKERS] Strange Windows problem, lock_timeout test request

2013-03-18 Thread Boszormenyi Zoltan


2013-03-18 06:47 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

The volatile marking shouldn't even be necessary there.
The signal handler doesn't writes to it, only the main code.

Well, (a) that's not the case in the patch as committed, and (b) even if
it were true, the volatile marking is still *necessary*, because that's
what tells the compiler it can't optimize away changes to the variable,
say on the grounds of there being another store with no intervening
fetches in the main-line code.  Without that, a compiler that had
aggressively inlined the smaller functions could well deduce that the
disable_alarm() assignment was useless.


Also, since the the variable assignment doesn't depend on other code
in the same function (or vice-versa) the compiler is still free to
reorder it.
Volatile is about not caching the variable in a CPU register since
it's volatile...

I don't believe you understand what volatile is about at all.  Go read
the C standard: it's about requiring objects' values to actually match
the nominal program-specified values at sequence points.  A more
accurate heuristic is that volatile tells the compiler there may be
other forces reading or writing the variable besides the ones it can see
in the current function's source code, and so it can't drop or reorder
accesses to the variable.

regards, tom lane


After reading up on a lot of volatile and memory barriers,
I am still not totally convinced.

For the record, sig_atomit_t is a plain int without any special
treatment from the compiler. It's atomic by nature on every 32-bit
and 64-bit CPU.

How about the attached patch over current GIT? In other words,
why I am wrong with this idea?

The problem that may arise if it's wrong is that transactions are
left waiting for the lock when the interrupt triggers and the variable
is still seen as false from the interrupt handler. But this doesn't happen.

FWIW, this small patch seems to give a 0,7 percent performance
boost for my settings:

shared_buffers = 256MB
work_mem = 8MB
effective_io_concurrency = 8
wal_level = hot_standby
checkpoint_segments = 64
random_page_cost = 1.8

Everything else is the default. I tested the patch on a 8-core
AMD FX-8120 CPU with this pgbench script below. Basically, it's
the default transaction prepended with SET lock_timeout = 1;
I have used the attached quick-and-dirty patch to pgbench to
ignore SQL errors coming from statements. -s 100 was used
to initialize the test database.

\set nbranches 1 * :scale
\set ntellers 10 * :scale
\set naccounts 10 * :scale
\setrandom aid 1 :naccounts
\setrandom bid 1 :nbranches
\setrandom tid 1 :ntellers
\setrandom delta -5000 5000
BEGIN;
SET lock_timeout = 1;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, 
:delta, CURRENT_TIMESTAMP);

END;

Results of pgbench -c 32 -j 32 -t 1 -e -f script.sql are
for the GIT version:

tps = 3366.844023 (including connections establishing)
tps = 3367.645454 (excluding connections establishing)

tps = 3379.784707 (including connections establishing)
tps = 3380.622317 (excluding connections establishing)

tps = 3385.198238 (including connections establishing)
tps = 3386.132433 (excluding connections establishing)

and with the barrier patch:

tps = 3412.799044 (including connections establishing)
tps = 3413.670832 (excluding connections establishing)

tps = 3389.796287 (including connections establishing)
tps = 3390.602187 (excluding connections establishing)

tps = 3405.924548 (including connections establishing)
tps = 3406.726997 (excluding connections establishing)

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/

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 3c3e41e..8737a86 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -16,6 +16,7 @@
 
 #include sys/time.h
 
+#include storage/barrier.h
 #include storage/proc.h
 #include utils/timeout.h
 #include utils/timestamp.h
@@ -58,10 +59,10 @@ static timeout_params *volatile active_timeouts[MAX_TIMEOUTS];
  * since the probability of the interrupt actually occurring while we have
  * it disabled is low.	See comments in schedule_alarm() about that.
  */
-static volatile sig_atomic_t alarm_enabled = false;
+static sig_atomic_t alarm_enabled = false;
 
-#define disable_alarm() (alarm_enabled = false)
-#define enable_alarm()	(alarm_enabled = true)
+#define disable_alarm() do { alarm_enabled = false; pg_write_barrier

Re: [HACKERS] Strange Windows problem, lock_timeout test request


2013-03-17 04:48 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

  [ 2-lock_timeout-v37.patch ]

Applied after a fair amount of additional hacking.


Thank you, thank you, thank you! :-)


I was disappointed to find that the patch introduced a new race
condition into timeout.c, or at least broke a safety factor that had
been there.  The argument why enable_timeout() could skip disabling
the timer interrupt on entry, if num_active_timeouts is zero, doesn't
work for enable_timeouts(): as soon as you've incremented
num_active_timeouts for the first new timeout, the signal handler would
mess around with the data structure if it were to fire.  What I did
about that was to modify disable_alarm() to forcibly disable the
interrupt if we're adding multiple timeouts in enable_timeouts(), even
if we think no interrupt is pending.  This might be overly paranoid,
but since all of this is new code for 9.3 and hasn't been through any
beta testing, I felt it best to preserve that safety feature.  We can
revisit it later if it proves to be an issue.  (It's conceivable for
instance that we could avoid incrementing the stored value of
num_active_timeouts until we're done adding all the new timeouts;
but that seemed pretty messy.)


Your reasoning seems to be correct. However, if we take it to the
extreme, enable_timeout_at/enable_timeout_after/enable_timeouts
should also disable the interrupt handler at the beginning of the
function and enabled at the end with pqsignal(). An evil soul may
send SIGALRM externally to the backend processes at the proper
moment and create a race condition while enable_timeout*() is in
progress and the itimer is about to trigger at the same time (but
doesn't since it was disabled).


   For the current usage pattern I'm not
too sure that it matters anyway: a savings is only possible if you
have enabled lock_timeout but not statement_timeout, and I'm doubtful
that that will be a common use-case.


I am not too sure about it. With lock_timeout in place, code migrated
from Informix, MSSQL, etc. can have the same semantic behaviour.



I also rearranged the handling of the LOCK_TIMEOUT interrupt some more,
since I didn't see a need for it to be different from STATEMENT_TIMEOUT,
and got rid of some non-C89 coding practices that didn't seem to me to
be improving readability anyway.


Thanks for that, too. I was too blind to see that choice, even after
thinking about why the statement_timeout cannot be done from
SIGALRM context and why does the code need to also send SIGINT
to the process group. (To kill children, too, like scripts executed via
system()...)

Thanks again,
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] Strange Windows problem, lock_timeout test request


2013-03-17 16:07 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

2013-03-17 04:48 keltezéssel, Tom Lane írta:

[ worries about stray SIGALRM events ]

Your reasoning seems to be correct. However, if we take it to the
extreme, enable_timeout_at/enable_timeout_after/enable_timeouts
should also disable the interrupt handler at the beginning of the
function and enabled at the end with pqsignal(). An evil soul may
send SIGALRM externally to the backend processes at the proper
moment and create a race condition while enable_timeout*() is in
progress and the itimer is about to trigger at the same time (but
doesn't since it was disabled).

Well, a malefactor with the ability to send signals to a backend
process could also send SIGKILL, or any number of other signals that
would mess things up.  I'm not too concerned about that scenario.
I *am* concerned about leftover timer events, both because this code
isn't very well tested, and because third-party code loaded into the
backend (think pl/perl for instance) could easily set up timer events
we weren't expecting.


I see.


It suddenly occurs to me though that there's more than one way to skin
this cat.  We could easily add another static flag variable called
sigalrm_allowed or some such, and have the signal handler test that
and immediately return without doing anything if it's off.  Clearing
and setting such a variable would be a lot cheaper than an extra
setitimer call, as well as more robust since it would protect us from
all sources of SIGALRM not just ITIMER_REAL.


Something like the attached patch?

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/

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index b5a3c8f..314a601 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -49,52 +49,29 @@ static bool all_timeouts_initialized = false;
 static volatile int num_active_timeouts = 0;
 static timeout_params *volatile active_timeouts[MAX_TIMEOUTS];
 
+/*
+ * Safeguard for the signal handler.
+ */
+static volatile bool alarm_enabled = false;
 
 /*
  * Internal helper functions
  *
  * For all of these, it is caller's responsibility to protect them from
- * interruption by the signal handler.	Generally, call disable_alarm()
- * first to prevent interruption, then update state, and last call
+ * interruption by the signal handler.	Generally, set alarm_enabled to false
+ * first to prevent interrupt to do anything, then update state, and last call
  * schedule_alarm(), which will re-enable the interrupt if needed.
- */
-
-/*
- * Disable alarm interrupts
  *
- * multi_insert must be true if the caller intends to activate multiple new
- * timeouts.  Otherwise it should be false.
- */
-static void
-disable_alarm(bool multi_insert)
-{
-	struct itimerval timeval;
+ * There may be a pending interrupt during enable_timeout_at(),
+ * enable_timeout_after() or enablwe_timeouts(). But since alarm_enabled is
+ * false, it is harmless when it triggers. We will reschedule the interrupt
+ * that just fired. In the worst case, it will be late by one unit of the
+ * the timer resolution in the OS.
+ *
+ */
 
-	/*
-	 * If num_active_timeouts is zero, and multi_insert is false, we don't
-	 * have to call setitimer.	There should not be any pending interrupt, and
-	 * even if there is, the worst possible case is that the signal handler
-	 * fires during schedule_alarm.  (If it fires at any point before
-	 * insert_timeout has incremented num_active_timeouts, it will do nothing,
-	 * since it sees no active timeouts.)  In that case we could end up
-	 * scheduling a useless interrupt ... but when the extra interrupt does
-	 * happen, the signal handler will do nothing, so it's all good.
-	 *
-	 * However, if the caller intends to do anything more after first calling
-	 * insert_timeout, the above argument breaks down, since the signal
-	 * handler could interrupt the subsequent operations leading to corrupt
-	 * state.  Out of an abundance of caution, we forcibly disable the timer
-	 * even though it should be off already, just to be sure.  Even though
-	 * this setitimer call is probably useless, we're still ahead of the game
-	 * compared to scheduling two or more timeouts independently.
-	 */
-	if (multi_insert || num_active_timeouts  0)
-	{
-		MemSet(timeval, 0, sizeof(struct itimerval));
-		if (setitimer(ITIMER_REAL, timeval, NULL) != 0)
-			elog(FATAL, could not disable SIGALRM timer: %m);
-	}
-}
+#define disable_alarm() \
+	do { alarm_enabled = false; } while (0)
 
 /*
  * Find

Re: [HACKERS] Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]


2013-03-17 17:05 keltezéssel, Greg Smith írta:

On 3/14/13 4:48 PM, Boszormenyi Zoltan wrote:

The attached patch makes
SET PERSISTENT transactional and uses one setting per file.
It uses the currently existing parsing and validating code
and because of this, the patch is half the size of v12 from Amit.


That's not a completely fair comparison, because you lost all the regression testing 
code too.


True.

This does look like a usefully tighter implementation in a few ways, so good progress on 
that.


I still can't see any reason to prefer this one setting per file idea.  As I see it, 
that is pushing the complexity toward the user in a bad way, seemingly just so it's 
easier to implement.  Most of my customers now use tools like Puppet to manage their 
PostgreSQL configuration.  I do not want to have this conversation:


Greg:  You can use SET PERSISTENT to modify settings instead of changing the 
postgresql.conf


User:  That's useful.  How do we adjust Puppet to make sure it picks up the 
changes?

Greg:  You just scan this config directory and add every file that appears in there!  
Each setting will be in its own file.


User:  shocked look  It creates new files?  That isn't going to fit into our version 
control approach easily.  Can we disable this feature so no one accidentally uses it?


I'm not exaggerating here--one setting per file makes this feature less than useless 
to me.  It becomes something where I will have to waste time defending against people 
using it.  I'd prefer to not have this at all than to do it that way.


That we're breaking these settings off into their own file, instead of trying to edit 
the postgresql.conf, to me is a pragmatic trade-off to keep the implementation from 
being really complicated.  It's also a step forward in a larger series for how to 
improve configuration management.  Just because that change introduces an entire 
directory being scanned, I don't see that as an excuse to clutter it with a long list of 
files too.


OK. I just wanted to show an alternative implementation.

I admit, I haven't read all mails from this thread so I don't know
how important for this feature to be non-transactional, or
whether it would be better to be transactional.

The one-file-to-rule-them-all approach can also be added to this
patch as well, but synchronizing transactions over parsing and
rewriting the extra file would decrease the relaxed effects of
synchronous_commit. On the other hand, writing out one file
per setting, although atomic to the level of one file, cannot be
guaranteed to be atomic as a whole for all settings changed
in the transaction without some synchronization.

As far as I can see, AtEOXact_GUC() is called outside the
critical section and is not synchronized any other way.
(Currently, there's no need to.)

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] Strange Windows problem, lock_timeout test request


2013-03-18 03:52 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

2013-03-17 16:07 keltezéssel, Tom Lane írta:

It suddenly occurs to me though that there's more than one way to skin
this cat.  We could easily add another static flag variable called
sigalrm_allowed or some such, and have the signal handler test that
and immediately return without doing anything if it's off.  Clearing
and setting such a variable would be a lot cheaper than an extra
setitimer call, as well as more robust since it would protect us from
all sources of SIGALRM not just ITIMER_REAL.

Something like the attached patch?

Well, something like that, but it still needed some improvements.  In
particular I thought it best to leave the signal handler still releasing
the procLatch unconditionally --- that behavior is independent of what
this module is doing.  Also you seem to have some odd ideas about what
do-while will accomplish.  AFAIK, in this usage it's purely a syntactic
trick without much semantic content.  It's the marking of the variable
as volatile that counts for telling the compiler not to re-order
operations.


The volatile marking shouldn't even be necessary there.
The signal handler doesn't writes to it, only the main code.
I just put it there saying why not? to myself.
IIRC, volatile is needed if both the signal handler and the
main code changes the same variable.

I got the reordering idea from here:
http://yarchive.net/comp/linux/compiler_barriers.html

Thanks for committing,
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] Strange Windows problem, lock_timeout test request