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

[David Fetter]

On Fri, Sep 30, 2016 at 04:23:13PM +1300, Thomas Munro wrote:
> On Thu, Sep 29, 2016 at 6:19 PM, David Fetter  wrote:
> > On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote:
> >> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
> >>  wrote:
> >> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
> >> >  wrote:
> >> >>
> >> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
> >> >> >
> >> >> > [training_wheels_004.patch]
> >> >>
> >> >> [review]
> >>
> >> Ping.
> >
> > Please find attached the next revision.
> 
> I'm not sold on ERRCODE_SYNTAX_ERROR.  There's nothing wrong with the
> syntax, since parsing succeeded.  It would be cool if we could use
> ERRCODE_E_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED, though I'm not sure
> what error class 38 really means.  Does require_where's hook function
> count as an 'external routine' and on that basis is it it allowed to
> raise this error?  Thoughts, anyone?
> 
> I am still seeing the underscore problem when building the docs.
> Please find attached fix-docs.patch which applies on top of
> training_wheels_005.patch.  It fixes that problem for me.
> 
> The regression test fails.  The expected error messages show the old
> wording, so I think you forgot to add a file.  Also, should
> contrib/require_where/Makefile define REGRESS = require_where?  That
> would allow 'make check' from inside that directory, which is
> convenient and matches other extensions.  Please find attached
> fix-regression-test.patch which also applies on top of
> training_wheels_005.patch and fixes those things for me, and also adds
> a couple of extra test cases.
> 
> Robert Haas mentioned upthread that the authors section was too short,
> and your follow-up v3 patch addressed that.  Somehow two authors got
> lost on their way to the v5 patch.  Please find attached
> fix-authors.patch which also applies on top of
> training_wheels_005.patch to restore them.
> 
> It would be really nice to be able to set this to 'Ready for
> Committer' in this CF.  Do you want to post a v6 patch or are you
> happy for me to ask a committer to look at v5 + these three
> corrections?

Thanks!

I've rolled your patches into this next one and clarified the commit
message, as there appears to have been some confusion about the scope.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..4bd456f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -40,6 +40,7 @@ SUBDIRS = \
pgstattuple \
pg_visibility   \
postgres_fdw\
+   require_where   \
seg \
spi \
tablefunc   \
diff --git a/contrib/require_where/Makefile b/contrib/require_where/Makefile
new file mode 100644
index 000..9c41691
--- /dev/null
+++ b/contrib/require_where/Makefile
@@ -0,0 +1,19 @@
+# contrib/require_where/Makefile
+
+MODULE_big = require_where
+OBJS = require_where.o
+
+PGFILEDESC = 'require_where - require DELETE and/or UPDATE to have a WHERE 
clause'
+
+REGRESS = require_where
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS = $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/require_where
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_builddir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/require_where/data/test_require_where.data 
b/contrib/require_where/data/test_require_where.data
new file mode 100644
index 000..d4a29d8
--- /dev/null
+++ b/contrib/require_where/data/test_require_where.data
@@ -0,0 +1,16 @@
+Four
+score
+and
+seven
+years
+ago
+our
+fathers
+brought
+forth
+on
+this
+continent
+a
+new
+nation
diff --git a/contrib/require_where/expected/require_where.out 
b/contrib/require_where/expected/require_where.out
new file mode 100644
index 000..adfd358
--- /dev/null
+++ b/contrib/require_where/expected/require_where.out
@@ -0,0 +1,22 @@
+--
+--   Test require_where
+--
+\set echo all
+LOAD 'require_where';
+CREATE TABLE test_require_where(t TEXT);
+\copy test_require_where from 'data/test_require_where.data'
+UPDATE test_require_where SET t=t; -- succeeds
+SET require_where.update TO ON;
+UPDATE test_require_where SET t=t; -- fails
+ERROR:  UPDATE requires a WHERE clause when require_where.delete is set to on
+HINT:  To update all rows, use "WHERE true" or similar.
+UPDATE test_require_where SET t=t WHERE true; -- succeeds
+SET require_where.update TO OFF;
+UPDATE test_require_where SET t=t; -- succeeds
+SET require_where.delete TO ON;
+DELETE FROM test_require_where; -- fails
+ERROR:  DELETE requires a WHERE clause when 

Re: [HACKERS] Commit fest 2017-01 will begin soon!

[Michael Paquier]

On Tue, Dec 27, 2016 at 12:41 PM, Michael Paquier
 wrote:
> There are still a couple of days to register patches! So if you don't
> want your fancy feature to be forgotten, please add it in time to the
> CF app. Speaking of which, I am going to have a low bandwidth soon as
> that's a period of National Holidays in Japan for the new year, and I
> don't think I'll be able to mark the CF as in progress AoE time. So if
> somebody could do it for me that would be great :)

Reminder: just 1 more day. Be careful to have your patches registered!
-- 
Michael


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

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

[Michael Paquier]

On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander  wrote:
> On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
>  wrote:
>> Recovery tests are broken by this patch, the backup() method in
>> PostgresNode.pm uses pg_basebackup -x:
>> sub backup
>> {
>> my ($self, $backup_name) = @_;
>> my $backup_path = $self->backup_dir . '/' . $backup_name;
>> my $port= $self->port;
>> my $name= $self->name;
>>
>> print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
>> TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
>> $port,
>> '-x', '--no-sync');
>> print "# Backup finished\n";
>> }
>
>
> Oh bleh. That's what I get for just running the tests for pg_basebackup
> itself.
>
> I removed it completely in this patch, making it use streaming. Or is there
> a particular reason we want it to use fetch, that I'm not aware of?

Not really. Both should be equivalent in the current tests. It may
actually be a good idea to add an argument in PostgresNode->backup to
define the WAL fetching method.

> Attached is a new patch with this fix, and those suggested by Fujii as well.

-the backup. If this option is specified, it is possible to start
-a postmaster directly in the extracted directory without the need
-to consult the log archive, thus making this a completely standalone
-backup.
+the backup. Unless the method none is specified,
+it is possible to start a postmaster directly in the extracted
+directory without the need to consult the log archive, thus
+making this a completely standalone backup.

I find a bit weird to mention a method value in a paragraph before
listing them. Perhaps it should be a separate paragraph after the list
of methods available?

-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
Two booleans are used to define 3 states. It may be cleaner to use an
enum instead. The important point is that streaming WAL is enabled
only if "stream" method is used.

Other than that the patch looks good to me. Tests pass.
-- 
Michael


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

[HACKERS] Fixing pgbench's logging of transaction timestamps

[Tom Lane]

pgbench's -l option is coded using inappropriate familiarity with the
contents of struct instr_time.  I recall complaining about that when
the code went in, but to little avail.  However, it needs to be fixed
if we're to switch over to using clock_gettime() as discussed in
the gettimeofday thread,
https://www.postgresql.org/message-id/17524.1483063...@sss.pgh.pa.us
And really it's unacceptably bad code even without that consideration,
because it doesn't work on Windows.

There are at least three ways we could fix it:

1. Switch over to printing the timestamp in the form of elapsed seconds
since the pgbench run start, as in the attached draft patch (which is
code-complete but lacks necessary documentation changes).  You could make
an argument that this is a better definition than what's there: in most
situations, people are going to want the elapsed time, and right now they
have to do painful manual arithmetic to get it.  About the only reason
I can see for liking the current definition is that it makes it possible
to correlate the pgbench log with external events, and I'm not sure
whether that's especially useful.

2. Have pgbench save both the INSTR_TIME_SET_CURRENT() and gettimeofday()
results for the run start instant.  In doLog(), compute the elapsed time
from run start much as in the attached patch, but then add it to the saved
gettimeofday() result and print that in the existing format.  This would
preserve the existing output format at the cost of a very small amount
of extra arithmetic per log line.  However, it's got a nasty problem if
we use clock_gettime(CLOCK_MONOTONIC) in instr_time, which I think would
be the typical case.  To the extent that CLOCK_MONOTONIC diverges from
CLOCK_REALTIME, which it would in case of external adjustments to the
system clock, the printed timestamps would no longer match the actual
system clock, which destroys the argument that you could correlate the
pgbench log with other events.  (I imagine the same problem would apply
in the Windows implementation.)

3. Forget about using the instr_time result and just have doLog() execute
gettimeofday() to obtain the timestamp to print.  This is kind of
conceptually ugly, but realistically the added overhead is probably
insignificant.  A larger objection might be that on Windows, the result
of gettimeofday() isn't very high precision ... but it'd still be a huge
improvement over the non-answer you get now.

I'm inclined to think that #2 isn't a very good choice; it appears to
preserve the current behavior but really doesn't.  So we should either
change the behavior as in #1 or expend an extra system call as in #3.
Preferences?

regards, tom lane

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index fbb0c2b..ca75900 100644
*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
*** static int64 total_weight = 0;
*** 398,403 
--- 398,405 
  
  static int	debug = 0;			/* debug flag */
  
+ static instr_time start_time;	/* overall run start time */
+ 
  /* Builtin test scripts */
  typedef struct BuiltinScript
  {
*** doLog(TState *thread, CState *st, instr_
*** 2483,2509 
  	else
  	{
  		/* no, print raw transactions */
! #ifndef WIN32
  
! 		/* This is more than we really ought to know about instr_time */
  		if (skipped)
! 			fprintf(logfile, "%d " INT64_FORMAT " skipped %d %ld %ld",
  	st->id, st->cnt, st->use_file,
! 	(long) now->tv_sec, (long) now->tv_usec);
  		else
! 			fprintf(logfile, "%d " INT64_FORMAT " %.0f %d %ld %ld",
  	st->id, st->cnt, latency, st->use_file,
! 	(long) now->tv_sec, (long) now->tv_usec);
! #else
! 
! 		/* On Windows, instr_time doesn't provide a timestamp anyway */
! 		if (skipped)
! 			fprintf(logfile, "%d " INT64_FORMAT " skipped %d 0 0",
! 	st->id, st->cnt, st->use_file);
! 		else
! 			fprintf(logfile, "%d " INT64_FORMAT " %.0f %d 0 0",
! 	st->id, st->cnt, latency, st->use_file);
! #endif
  		if (throttle_delay)
  			fprintf(logfile, " %.0f", lag);
  		fputc('\n', logfile);
--- 2485,2501 
  	else
  	{
  		/* no, print raw transactions */
! 		instr_time	elapsed = *now;
  
! 		INSTR_TIME_SUBTRACT(elapsed, start_time);
  		if (skipped)
! 			fprintf(logfile, "%d " INT64_FORMAT " skipped %d %.6f",
  	st->id, st->cnt, st->use_file,
! 	INSTR_TIME_GET_DOUBLE(elapsed));
  		else
! 			fprintf(logfile, "%d " INT64_FORMAT " %.0f %d %.6f",
  	st->id, st->cnt, latency, st->use_file,
! 	INSTR_TIME_GET_DOUBLE(elapsed));
  		if (throttle_delay)
  			fprintf(logfile, " %.0f", lag);
  		fputc('\n', logfile);
*** main(int argc, char **argv)
*** 3664,3670 
  	CState	   *state;			/* status of clients */
  	TState	   *threads;		/* array of thread */
  
- 	instr_time	start_time;		/* start up time */
  	instr_time	total_time;
  	instr_time	conn_total_time;
  	int64		latency_late = 0;
--- 3656,3661 

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

Re: [HACKERS] safer node casting

[Tom Lane]

I wrote:
> Just to bikeshed a bit ... would "castNode" be a better name?

Um ... -ENOCAFFEINE.  Never mind that bit.

regards, tom lane


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

Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

2016-12-31 18:46 GMT+01:00 Fabien COELHO :

>
>DROP VARIABLE super_secret;
>>>CREATE VARIABLE super_secret ...;
>>>
>>
>> But you don't do it in functions - these variables are persistent - you
>> don't create it or drop inside functions. The content is secure, so you
>> don't need to hide this variable against other.
>>
>
> ISTM that you are still missing my point.
>
> I understood that you want a static analysis tool to re-assure you about
> how your session variables are manipulated. I do not see how such a tool
> can give any assurance without checking that the variable meta-data are not
> changed by some malicious code inserted in a function.


if you afraid this, then just use grep to verify functions that have this
code. It is same like tables - you can generate it dynamicly, but is risks
- similar to use dynamic SQL. Sure, there is a exceptions - but there are
rules for PL - don't use dynamic SQL if it is not deadly necessary, use SQL
security, not own, ...



>
>
>
>>> I'm not sure that I understand these sentences.
>>>
>>
>>
>> so I don't prefer any design that increase a area where plpgsql_check
>> should not work.
>>
>
> My assumption is that plpgsql_check can be improved. For instance, I
> assume that if "secure session variables" are added, then it will be
> enhanced to do some checking about these and take them into account. If
> "simple session variables" are added, I assume that it would also be
> updated accordingly.


in simple session variables there are not any safe point - any
authoritative point. Sure I can do some - I can introduce some hints, etc -
but it is workaround - nothing more - it like C development without header
files.


>
>
> I wrote my notes there.
>>>


>>> Great! I restructured a little bit and tried to improve the English. I
>>> also added questions when some statement that I think are too optimistic,
>>> or are unclear to me.
>>>
>>
>> we have just different perspectives
>>
>
> I'm trying to have sentences that are both clear and true. If I think that
> a sentence is imprecise because it is missing a key hypothesis, then I try
> to improve it, whether it is mine or someone else.




>
>
> --
> Fabien.
>

Re: [HACKERS] safer node casting

[Tom Lane]

Peter Eisentraut  writes:
> I propose a macro castNode() that combines the assertion and the cast,
> so this would become
> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Seems like an OK idea, but I'm concerned by the implied multiple
evaluations, particularly if you're going to apply this to function
results --- as the above example does.  I think you need to go the
extra mile to make it single-evaluation.  See newNode() for ideas.

Just to bikeshed a bit ... would "castNode" be a better name?
Seems like a closer analogy to makeNode(), for instance.

regards, tom lane


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

Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

>
> If you do not have expectations, then all is fine.
>
> (1) Having some kind of variable, especially in interactive mode, allows to
>>> manipulate previous results and reuse them later, without having to
>>> resort
>>> to repeated sub-queries or to retype non trivial values.
>>>
>>> Client side psql :-variables are untyped and unescaped, thus not very
>>> convenient for this purpose.
>>>
>>
>> You can currently (ab)use user defined GUCs for this.
>>
>
> How? It seems that I have missed the syntax to assign the result of a
> query to a user-defined guc, and to reuse it simply in a query.
>
>
 postgres=# select set_config('myvar.text', (select
current_timestamp::text), false);
+---+
|  set_config   |
+---+
| 2016-12-31 18:56:42.894246+01 |
+---+
(1 row)

Time: 0,448 ms
postgres=# select current_setting('myvar.text');
+---+
|current_setting|
+---+
| 2016-12-31 18:56:42.894246+01 |
+---+
(1 row)


-- 
> Fabien.
>

Re: [HACKERS] proposal: session server side variables

[Fabien COELHO]

   DROP VARIABLE super_secret;
   CREATE VARIABLE super_secret ...;


But you don't do it in functions - these variables are persistent - you
don't create it or drop inside functions. The content is secure, so you
don't need to hide this variable against other.


ISTM that you are still missing my point.

I understood that you want a static analysis tool to re-assure you about 
how your session variables are manipulated. I do not see how such a tool 
can give any assurance without checking that the variable meta-data are 
not changed by some malicious code inserted in a function.




I'm not sure that I understand these sentences.



so I don't prefer any design that increase a area where plpgsql_check
should not work.


My assumption is that plpgsql_check can be improved. For instance, I 
assume that if "secure session variables" are added, then it will be 
enhanced to do some checking about these and take them into account. If 
"simple session variables" are added, I assume that it would also be 
updated accordingly.



I wrote my notes there.




Great! I restructured a little bit and tried to improve the English. I
also added questions when some statement that I think are too optimistic,
or are unclear to me.


we have just different perspectives


I'm trying to have sentences that are both clear and true. If I think that 
a sentence is imprecise because it is missing a key hypothesis, then I try 
to improve it, whether it is mine or someone else.


--
Fabien.


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

Re: [HACKERS] proposal: session server side variables

[Fabien COELHO]

Hello Craig,


As for "slow", I have just tested overheads with pgbench, comparing a direct
arithmetic operation (as a proxy to a fast session variable consultation) to
constant returning plpgsql functions with security definer and security
invoker, on a direct socket connection, with prepared statements:

  select 1 + 0: 0.020 ms
  select one_sd() : 0.024 ms
  select one_si() : 0.024 ms


That's one call per executor run. Not really an effective test.


I really did 10 calls per transaction. For one call it was 24 ms vs 28 ms. 
However I'm not sure of the respective overheads of the protocol, planer 
and executor, though.



Consider cases like row security where you're testing 1 rows.


Another test: calling 1,000,000 times one_sd() or one_si() in a plpgsql 
loops seems to cost about 1.1 seconds on my laptop. I'd say that the 
function call is about 2/3 of that time, the rest is on the loop and exit 
test.


  SELECT NOW();
  DO LANGUAGE plpgsql $$
DECLARE count INT DEFAULT 0;
BEGIN
  LOOP count := count + ONE_SD() ;
  EXIT WHEN count = 100;
END LOOP;
  END; $$;
  SELECT NOW();

Based on these evidences, I continue to think that there is no significant 
performance issue with calling simple security definer functions.




Hopefully the planner will inline the test if it's a function declared
stable, but it may not.


Indeed they are, so the planner should factor out the test when possible.



* On what basis do you _oppose_ persistently defining variables in the
catalogs as their own entities?


In understand that you are speaking of "persistent session variables".

For me a database is about persistence (metadata & data) with safety
(transactions) and security (permissions)... and maybe performance:-)

Pavel's proposal creates a new object with 2 (secure metadata-persistence)
out of 4 properties... I'm not a ease with introducting a new half-database
concept in a database.


I strongly disagree. If you want "all-database" properties ... use tables.


Sure. I am not sure about what are you disagreeing with, as I'm just 
describing Pavel's proposal...



We generally add new features when that's not sufficient to achieve
something. Most notably SEQUENCEs, which deliberately violate
transaction isolation and atomicity in order to deliver a compelling
benefit not otherwise achieveable.


Yes, sure.


On the other hand there are dynamic session variables (mysql, mssql, oracle
have some variants) which are useful on their own without pretending to be
database objects (no CREATE/ALTER/DROP, GRANT/REVOKE).


We have precent here for sequences. Yes, they do confuse users, but
they're also VERY useful, and the properties of variables would be
clearer IMO.


Yep. But my point is that before adding a new strange object type I would 
prefer that there is no other solution.



I'm not especially attached to doing them as database objects; I'm
just as happy with something declared at session start by some
function that then intends to set and use the variable. But I don't
think your argument against a DDL-like approach holds water.


I have expectations about objects hold by a database, and these new object 
fails them.


If you do not have expectations, then all is fine.


(1) Having some kind of variable, especially in interactive mode, allows to
manipulate previous results and reuse them later, without having to resort
to repeated sub-queries or to retype non trivial values.

Client side psql :-variables are untyped and unescaped, thus not very
convenient for this purpose.


You can currently (ab)use user defined GUCs for this.


How? It seems that I have missed the syntax to assign the result of a 
query to a user-defined guc, and to reuse it simply in a query.


--
Fabien.


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

Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

2016-12-31 17:51 GMT+01:00 Fabien COELHO :

>
> unexpectedly, that would be missed by a PL/pgSQL analysis tool... There is
>>> no miracle.
>>>
>>
>> No - metadata, in my design, are persistent - like tables - so you don't
>> calculate so any functions can drop a variables. The deployment of secure
>> variables is same like table deployment. No dynamic there.
>>
>
> You are missing my point: Static analysis is about proving properties. If
> you need metadata to be persistent, then you should check that it is the
> case, i.e. the static analysis must check that there is no metadata changes
> anywhere. For instance, an analysis tool should reject a function which
> contains:
>
>GRANT UPDATE ON VARIABLE super_secret_variable TO PUBLIC;
>

It doesn't need to reject this functions - but this information is not
visible in any other functions. But this tasks you are do in deployment
scripts - not in functions.


>
> Or does:
>
>DROP VARIABLE super_secret;
>CREATE VARIABLE super_secret ...;
>

But you don't do it in functions - these variables are persistent - you
don't create it or drop inside functions. The content is secure, so you
don't need to hide this variable against other.


>
> If a static analysis tool is specific to one language, then it can only
> checks that all is well in functions in those languages, but as there may
> be functions written in other languages as well then the check is somehow
> partial. This is not a bad thing, it just illustrate that you cannot check
> everything. That is quality ensurance.
>
> [...] Indeed, probably there exists some class of typos that may not be
>>> found by some static analysis implementations on PL/pgSQL functions which
>>> uses basic session variables.
>>>
>>
>> yes, and I would not to append any new case that cannot be covered by
>> plpgsql check. Dynamic SQL and our temporal tables are enough issues
>> already.
>>
>
> I'm not sure that I understand these sentences.


so I don't prefer any design that increase a area where plpgsql_check
should not work.


>
>
> I wrote my notes there.
>>
>
> Great! I restructured a little bit and tried to improve the English. I
> also added questions when some statement that I think are too optimistic,
> or are unclear to me.


we have just different perspectives

Regards

Pavel

>
>
> --
> Fabien.
>

[HACKERS] safer node casting

[Peter Eisentraut]

There is a common coding pattern that goes like this:

RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
Assert(IsA(rinfo, RestrictInfo));

(Arguably, the Assert should come before the cast, but I guess it's done
this way out of convenience.)

(Not to mention the other common coding pattern of just doing the cast
and hoping for the best.)

I propose a macro castNode() that combines the assertion and the cast,
so this would become

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

This is inspired by the dynamic_cast operator in C++, but follows the
syntax of the well-known makeNode() macro.

Besides saving a bunch of code and making things safer, the function
syntax also makes some code easier to read by saving levels of
parentheses, for example:

-   Assert(IsA(sstate->testexpr, BoolExprState));
-   oplist = ((BoolExprState *) sstate->testexpr)->args;
+   oplist = castNode(BoolExprState, sstate->testexpr)->args;

Attached is a patch that shows how this would work.  There is a lot more
that can be done, but I just stopped after a while for now.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From fd1e766ff765ecc58380d09c657ca254a6bf5674 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 28 Dec 2016 12:00:00 -0500
Subject: [PATCH] Add castNode() macro

---
 contrib/pg_stat_statements/pg_stat_statements.c | 10 --
 contrib/postgres_fdw/deparse.c  |  5 +
 contrib/postgres_fdw/postgres_fdw.c | 12 
 src/backend/catalog/pg_proc.c   |  3 +--
 src/backend/commands/aggregatecmds.c|  4 ++--
 src/backend/commands/analyze.c  |  6 +++---
 src/backend/commands/async.c|  8 
 src/backend/commands/collationcmds.c|  2 +-
 src/backend/commands/constraint.c   |  2 +-
 src/backend/commands/copy.c | 14 +++---
 src/backend/commands/createas.c |  5 ++---
 src/backend/commands/dropcmds.c |  4 +---
 src/backend/commands/explain.c  | 16 +++-
 src/backend/commands/functioncmds.c |  7 ++-
 src/backend/commands/matview.c  |  3 +--
 src/backend/commands/opclasscmds.c  |  9 +++--
 src/backend/commands/tablecmds.c| 14 --
 src/backend/commands/trigger.c  |  4 +---
 src/backend/commands/user.c |  4 +---
 src/backend/commands/view.c |  3 +--
 src/backend/executor/execAmi.c  |  3 +--
 src/backend/executor/execQual.c |  6 ++
 src/backend/executor/execTuples.c   |  5 +
 src/backend/executor/functions.c|  6 ++
 src/backend/executor/nodeAgg.c  |  6 ++
 src/backend/executor/nodeCtescan.c  |  3 +--
 src/backend/executor/nodeCustom.c   |  3 +--
 src/backend/executor/nodeHashjoin.c |  7 ++-
 src/backend/executor/nodeLockRows.c |  4 +---
 src/backend/executor/nodeModifyTable.c  |  4 +---
 src/backend/executor/nodeSubplan.c  | 15 +--
 src/backend/executor/nodeWorktablescan.c|  4 ++--
 src/include/nodes/nodes.h   |  2 ++
 33 files changed, 74 insertions(+), 129 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8ce24e0401..d21f1d044c 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2336,9 +2336,8 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
 
 	foreach(lc, rtable)
 	{
-		RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+		RangeTblEntry *rte = castNode(RangeTblEntry, lfirst(lc));
 
-		Assert(IsA(rte, RangeTblEntry));
 		APP_JUMB(rte->rtekind);
 		switch (rte->rtekind)
 		{
@@ -2524,7 +2523,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
 APP_JUMB(sublink->subLinkType);
 APP_JUMB(sublink->subLinkId);
 JumbleExpr(jstate, (Node *) sublink->testexpr);
-JumbleQuery(jstate, (Query *) sublink->subselect);
+JumbleQuery(jstate, castNode(Query, sublink->subselect));
 			}
 			break;
 		case T_FieldSelect:
@@ -2590,9 +2589,8 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
 JumbleExpr(jstate, (Node *) caseexpr->arg);
 foreach(temp, caseexpr->args)
 {
-	CaseWhen   *when = (CaseWhen *) lfirst(temp);
+	CaseWhen   *when = castNode(CaseWhen, lfirst(temp));
 
-	Assert(IsA(when, CaseWhen));
 	JumbleExpr(jstate, (Node *) when->expr);
 	JumbleExpr(jstate, (Node *) when->result);
 }
@@ -2804,7 +2802,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
 
 /* we store the string name because RTE_CTE RTEs need it */
 

Re: [HACKERS] cast result of copyNode()

[Tom Lane]

Peter Eisentraut  writes:
> In order to reduce the number of useless casts and make the useful casts
> more interesting, here is a patch that automatically casts the result of
> copyNode() back to the input type, if the compiler supports something
> like typeof(), which most current compilers appear to.  That gets us
> some more type safety and we only need to retain the casts that actually
> do change the type.

But doesn't this result in a boatload of warnings on compilers that
don't have typeof()?

If typeof() were in C99 I might figure that is an okay tradeoff,
but it isn't.  I'm not sure that this is the most useful place
to be moving our C standards compliance expectations by 20 years
in one jump.

Also, if your answer is "you shouldn't get any warnings because
copyObject is already declared to return void *", then why aren't
we just relying on that today?

regards, tom lane


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

Re: [HACKERS] proposal: session server side variables

[Fabien COELHO]

unexpectedly, that would be missed by a PL/pgSQL analysis tool... There is
no miracle.


No - metadata, in my design, are persistent - like tables - so you don't
calculate so any functions can drop a variables. The deployment of secure
variables is same like table deployment. No dynamic there.


You are missing my point: Static analysis is about proving properties. If 
you need metadata to be persistent, then you should check that it is the 
case, i.e. the static analysis must check that there is no metadata 
changes anywhere. For instance, an analysis tool should reject a function 
which contains:


   GRANT UPDATE ON VARIABLE super_secret_variable TO PUBLIC;

Or does:

   DROP VARIABLE super_secret;
   CREATE VARIABLE super_secret ...;

If a static analysis tool is specific to one language, then it can only 
checks that all is well in functions in those languages, but as there may 
be functions written in other languages as well then the check is somehow 
partial. This is not a bad thing, it just illustrate that you cannot check 
everything. That is quality ensurance.


[...] Indeed, probably there exists some class of typos that may not be 
found by some static analysis implementations on PL/pgSQL functions 
which uses basic session variables.


yes, and I would not to append any new case that cannot be covered by 
plpgsql check. Dynamic SQL and our temporal tables are enough issues 
already.


I'm not sure that I understand these sentences.


I wrote my notes there.


Great! I restructured a little bit and tried to improve the English. I 
also added questions when some statement that I think are too optimistic, 
or are unclear to me.


--
Fabien.


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

Re: [HACKERS] Add doc advice about systemd RemoveIPC

[Tom Lane]

Magnus Hagander  writes:
> I still think that some wording in the direction of the fact that the
> majority of all users won't actually have this problem is the right thing
> to do (regardless of our previous history in the area as pointed out by
> Craig)

+1.  The use-a-system-user solution is the one that's in place on the
ground for most current PG users on affected platforms.  We should explain
that one first and make clear that platform-specific packages attempt to
set it up that way for you.  The RemoveIPC technique should be documented
as a fallback to be used if you can't/won't use a system userid.

regards, tom lane


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

Re: [HACKERS] tab complete regress tests

[Peter Eisentraut]

On 12/31/16 4:09 AM, Pavel Stehule wrote:
> now the code in tabcomplete become large part of psql. Is there some
> possibility to write regress tests? 

I started on that a while ago with some Perl Expect module.  The use of
the module was a bit cumbersome, but it worked OK.

The problem is that you end up just writing out another copy of
tab-complete.c in a different language.  So, for example, to test

else if (Matches4("ALTER", "DOMAIN", MatchAny, "SET"))
COMPLETE_WITH_LIST3("DEFAULT", "NOT NULL", "SCHEMA");

the test code would effectively look like

test_completion(["ALTER", "DOMAIN", random_string(), "SET"],
["DEFAULT, "NOT NULL", "SCHEMA"]);

That's not very interesting, and the regressions are more likely in
keeping the test code up to date than in actual behavior changes.

I do agree that having some tests would be good, because we're now so
used to having tests that reviewing tab completion changes becomes
strangely manual.  But I don't have a good idea how to structure those
tests efficiently.

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


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

Re: [HACKERS] tab complete regress tests

[Fabrízio de Royes Mello]

Em sáb, 31 de dez de 2016 às 07:11, Pavel Stehule 
escreveu:

> Hi
>
> now the code in tabcomplete become large part of psql. Is there some
> possibility to write regress tests?
>
> I found only this link
>
>
> http://stackoverflow.com/questions/22795767/how-to-test-python-readline-completion
>
> Isn't possible implement it using our current perl TAP infrastructure?

Regards,

[HACKERS] Replication/backup defaults

[Magnus Hagander]

Cycling back to this topic again, but this time at the beginning of a CF.

Here's an actual patch to change:


wal_level=replica
max_wal_senders=10
max_replication_slots=20

Based on feedback from last year (
https://www.postgresql.org/message-id/CABUevEwfV7zDutescm2PHGvsJdYA0RWHFMTRGhwrJPGgSbzZDQ%40mail.gmail.com
):


There were requests for benchmarks of performance difference. Tomas has
promised to run a couple of benchmarks on his standard benchmarking setups
to give numbers on that. Thanks Tomas, please pipe in with your results
when you have them!


Security considerations about pg_hba.conf -- I avoided those by not
actually changing pg_hba.conf. Since pg_hba.conf can be changed on a reload
instead of a restart it's a lot easier to deal with. I still think changing
it to allow "postgres" the same type of connections  as it does for regular
users would not be a security problem, but again thanks to it only needing
a reload it's not as big an issue.


There was the idea to have multiple sets of defaults to choose from at
initdb time. I don't see a problem having that, but it's been another year
and nobody built it. I don't think not having that is an excuse for the
current defaults. And implementing something like that is in no way
hindered by
changing the current defaults.

We were too close to beta1 -- this is why I'm sending it earlier this time
:) (Even though I intended to do it already back in September, better now
than even later)

Finally, there's the argument that we're already shaking up a number of
other things with version 10, so this is a good time to do this one as well.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 6eaed1e..d7df910 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1420,7 +1420,8 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'
  
   If more flexibility in copying the backup files is needed, a lower
   level process can be used for standalone hot backups as well.
-  To prepare for low level standalone hot backups, set wal_level to
+  To prepare for low level standalone hot backups, make sure
+  wal_level is set to
   replica or higher, archive_mode to
   on, and set up an archive_command that performs
   archiving only when a switch file exists.  For example:
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8d7b3bf..94cec33 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2169,12 +2169,12 @@ include_dir 'conf.d'
   
   

-wal_level determines how much information is written
-to the WAL. The default value is minimal, which writes
-only the information needed to recover from a crash or immediate
-shutdown. replica adds logging required for WAL
-archiving as well as information required to run
-read-only queries on a standby server.  Finally,
+wal_level determines how much information is written to
+the WAL. The default value is replica, which writes enough
+data to support WAL archiving and replication, including running
+read-only queries on a standby server. minimal removes all
+logging except the information required to recover from a crash or
+immediate shutdown.  Finally,
 logical adds information necessary to support logical
 decoding.  Each level includes the information logged at all lower
 levels.  This parameter can only be set at server start.
@@ -2912,7 +2912,7 @@ include_dir 'conf.d'
 Specifies the maximum number of concurrent connections from
 standby servers or streaming base backup clients (i.e., the
 maximum number of simultaneously running WAL sender
-processes). The default is zero, meaning replication is
+processes). The default is 10. The value 0 means replication is
 disabled. WAL sender processes count towards the total number
 of connections, so the parameter cannot be set higher than
 .  Abrupt streaming client
@@ -2937,7 +2937,7 @@ include_dir 'conf.d'
 
  Specifies the maximum number of replication slots
  (see ) that the server
- can support. The default is zero.  This parameter can only be set at
+ can support. The default is 20.  This parameter can only be set at
  server start.
  wal_level must be set
  to replica or higher to allow replication slots to
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 946ba9e..a31347b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2315,7 +2315,7 @@ static struct config_int ConfigureNamesInt[] =
 			NULL
 		},
 		_wal_senders,
-		0, 0, MAX_BACKENDS,
+		10, 0, MAX_BACKENDS,
 		NULL, NULL, NULL
 	},
 
@@ -2326,7 +2326,7 @@ static struct config_int ConfigureNamesInt[] =

Re: [HACKERS] Add doc advice about systemd RemoveIPC

[Magnus Hagander]

On Sat, Dec 31, 2016 at 6:30 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 12/30/16 3:59 AM, Magnus Hagander wrote:
> > I wonder if I missed part of the discussions around this, so maybe my
> > understanding of the cases where this occurs is wrong, but isn't it the
> > case of pretty much all (or actually) all the packaged versions of
> > postgresql out there (debian, redhat etc) that they do the right thing,
> > as in that they create "postgres" as a system user?
>
> If you install a package but the user already exists, then the package
> will just use that user.  So just using a package is not a guarantee
> that everything will be alright.
>
>
Good point.

I still think that some wording in the direction of the fact that the
majority of all users won't actually have this problem is the right thing
to do (regardless of our previous history in the area as pointed out by
Craig)

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

Re: [HACKERS] identity columns

[Peter Eisentraut]

Updated patch with some merge conflicts resolved and some minor bug
fixes.  Vitaly's earlier reviews were very comprehensive, and I believe
I have fixed all the issues that have been pointed out, so just
double-checking that would be helpful at this point.  I still don't have
a solution for managing access to the implicit sequences without
permission checking, but I have an idea, so I might send an update sometime.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From eee7443a04676a2b22892fb8114d2814e720b8ed Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 31 Dec 2016 12:00:00 -0500
Subject: [PATCH v3] Identity columns

This is the SQL standard-conforming variant of PostgreSQL's serial
columns.  It also fixes a few usability issues that serial columns
have:

- CREATE TABLE / LIKE copies default but refers to same sequence
- cannot add/drop serialness with ALTER TABLE
- dropping default does not drop sequence
- slight weirdnesses because serial is some kind of special macro
---
 doc/src/sgml/catalogs.sgml  |  11 +
 doc/src/sgml/information_schema.sgml|  11 +-
 doc/src/sgml/ref/alter_table.sgml   |  29 ++
 doc/src/sgml/ref/create_table.sgml  |  58 +++-
 doc/src/sgml/ref/insert.sgml|  41 +++
 src/backend/access/common/tupdesc.c |   5 +
 src/backend/catalog/genbki.pl   |   2 +
 src/backend/catalog/heap.c  |  15 +-
 src/backend/catalog/index.c |   1 +
 src/backend/catalog/information_schema.sql  |  19 +-
 src/backend/catalog/pg_depend.c |  17 +-
 src/backend/catalog/sql_features.txt|  12 +-
 src/backend/commands/sequence.c | 108 ++-
 src/backend/commands/tablecmds.c| 287 -
 src/backend/nodes/copyfuncs.c   |   4 +
 src/backend/nodes/equalfuncs.c  |   4 +
 src/backend/nodes/makefuncs.c   |   1 +
 src/backend/nodes/outfuncs.c|   9 +
 src/backend/nodes/readfuncs.c   |   1 +
 src/backend/parser/analyze.c|   2 +
 src/backend/parser/gram.y   | 126 +++-
 src/backend/parser/parse_utilcmd.c  | 396 ++--
 src/backend/rewrite/rewriteHandler.c|  43 ++-
 src/backend/utils/adt/ruleutils.c   |   8 +
 src/backend/utils/cache/lsyscache.c |  21 ++
 src/backend/utils/cache/relcache.c  |   1 +
 src/backend/utils/errcodes.txt  |   1 +
 src/bin/pg_dump/pg_dump.c   |  87 +-
 src/bin/pg_dump/pg_dump.h   |   3 +
 src/bin/psql/describe.c |  26 +-
 src/bin/psql/tab-complete.c |  18 +-
 src/include/catalog/catversion.h|   2 +-
 src/include/catalog/dependency.h|   2 +-
 src/include/catalog/pg_attribute.h  |  20 +-
 src/include/catalog/pg_class.h  |   2 +-
 src/include/commands/sequence.h |   1 +
 src/include/nodes/parsenodes.h  |  28 +-
 src/include/parser/kwlist.h |   2 +
 src/include/utils/lsyscache.h   |   1 +
 src/test/regress/expected/create_table_like.out |  47 +++
 src/test/regress/expected/identity.out  | 274 
 src/test/regress/expected/truncate.out  |  30 ++
 src/test/regress/parallel_schedule  |   5 +
 src/test/regress/serial_schedule|   1 +
 src/test/regress/sql/create_table_like.sql  |  14 +
 src/test/regress/sql/identity.sql   | 162 ++
 src/test/regress/sql/truncate.sql   |  18 ++
 47 files changed, 1800 insertions(+), 176 deletions(-)
 create mode 100644 src/test/regress/expected/identity.out
 create mode 100644 src/test/regress/sql/identity.sql

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 493050618d..3c18fffc1c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1105,6 +1105,17 @@ pg_attribute Columns
  
 
  
+  attidentity
+  char
+  
+  
+   If a space character, then not an identity column.  Otherwise,
+   a = generated always, d =
+   generated by default.
+  
+ 
+
+ 
   attisdropped
   bool
   
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325d06..8ece4398ba 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -1583,13 +1583,20 @@ columns Columns
  
   is_identity
   yes_or_no
-  Applies to a feature not available in PostgreSQL
+  
+   If the column is an identity column, then YES,
+   else NO.
+  
  
 
  
   

Re: [HACKERS] Supporting huge pages on Windows

[Magnus Hagander]

On Wed, Sep 28, 2016 at 2:26 AM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> > From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> > On Sun, Sep 25, 2016 at 10:45 PM, Tsunakawa, Takayuki
> >  wrote:
> > > Credit: This patch is based on Thomas Munro's one.
> >
> > How are they different?
>
> As Thomas mentioned, his patch (only win32_shmem.c) might not have been
> able to compile (though I didn't try.)  And it didn't have error processing
> or documentation.  I added error handling, documentation, comments, and a
> little bit of structural change.  The possibly biggest change, though it's
> only one-liner in pg_ctl.c, is additionally required.  I failed to include
> it in the first patch.  The attached patch includes that.
>
>

For the pg_ctl changes, we're going from removing all privilieges from the
token, to removing none. Are there any other privileges that we should be
worried about? I think you may be correct in that it's overkill to do it,
but I think we need some more specifics to decide that.

Also, what happens with privileges that were granted to the groups that
were removed? Are they retained or lost?

Should we perhaps consider having pg_ctl instead *disable* all the
privileges (rather than removing them), using AdjustTokenPrivileges? As a
middle ground?




Taking a look at the actual code here, and a few small nitpicks:

+errdetail("Failed system call was %s,
error code %lu", "LookupPrivilegeValue", GetLastError(;

this seems to be a new pattern of code -- for other similar cases it just
writes LookupPrivilegeValue inside the patch itself. I'm guessing the idea
was for translatability, but I think it's better we stick to the existing
pattern.


When AdjustTokenPrivileges() returns, you explicitly check for
ERROR_NOT_ALL_ASSIGNED, which is good. But we should probably also
explicitly check for ERROR_SUCCESS for that case. Right now that's the only
two possible options that can be returned, but in a future version other
result codes could be added and we'd miss them. Basically, "there should be
an else branch".


Is there a reason the error messages for AdjustTokenPrivileges() returning
false and ERROR_NOT_ALL_ASSIGNED is different?


There are three repeated blocks of
+   if (huge_pages == HUGE_PAGES_ON || huge_pages == HUGE_PAGES_TRY)

It threw me off in the initial reading, until I realized the upper levels
of them can change the value of huge_pages.

I don't think changing the global variable huge_pages like that is a very
good idea. Wouldn't something like the attached be cleaner (not tested)? At
least I find that one easier to read.



-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8d7b3bf..e1c3c66 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1335,17 +1335,23 @@ include_dir 'conf.d'

 

-At present, this feature is supported only on Linux. The setting is
-ignored on other systems when set to try.
+At present, this feature is supported only on Linux and Windows. The
+setting is ignored on other systems when set to try.

 

 The use of huge pages results in smaller page tables and less CPU time
-spent on memory management, increasing performance. For more details,
+spent on memory management, increasing performance. For more details on Linux,
 see .

 

+This feature uses the large-page support on Windows. To use the large-page
+support, you need to assign Lock page in memory user right to the Windows
+user account which runs PostgreSQL.
+   
+
+   
 With huge_pages set to try,
 the server will try to use huge pages, but fall back to using
 normal allocation if that fails. With on, failure
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 0ff2c7e..88b195a 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -21,6 +21,7 @@ HANDLE		UsedShmemSegID = INVALID_HANDLE_VALUE;
 void	   *UsedShmemSegAddr = NULL;
 static Size UsedShmemSegSize = 0;
 
+static bool EnableLockPagesPrivilege(int elevel);
 static void pgwin32_SharedMemoryDelete(int status, Datum shmId);
 
 /*
@@ -103,6 +104,61 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
 	return true;
 }
 
+/*
+ * EnableLockPagesPrivilege
+ *
+ * Try to acquire SeLockMemoryPrivilege so we can use large pages.
+ */
+static bool
+EnableLockPagesPrivilege(int elevel)
+{
+	HANDLE hToken;
+	TOKEN_PRIVILEGES tp;
+	LUID luid;
+
+	if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, ))
+	{
+		ereport(elevel,
+(errmsg("could not enable Lock pages in memory user right"),
+ errdetail("Failed 

Re: [HACKERS] port of INSTALL file generation to XSLT

[Magnus Hagander]

On Sat, Dec 31, 2016 at 6:57 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> A further step toward getting rid of the DSSSL tool chain requirement,
> here is a patch to change the generation of the text INSTALL file to use
> XLST and Pandoc.
>
> The change to Pandoc is not essential to this change, but it creates
> much better looking output and simplifies the locale/encoding handling
> over using Lynx.
>

Yeah, that seems a lot more clean than using Lynx.



>
> We'll need to get Pandoc installed on borka and check that that version
> works as well as the one I have been using.
>

Borka being a standard debian jessie install has Pandoc
1.12.4.2~dfsg-1+b14. Should be no problem installing that.

I ran a "make INSTALL" on a jessie box, and it comes out with some
formatting still in the file, see attachment. That seems incorrect.

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


INSTALL
Description: Binary data

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

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

[Magnus Hagander]

On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier  wrote:

> On Tue, Dec 20, 2016 at 10:38 PM, Fujii Masao 
> wrote:
> > On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov 
> wrote:
> >> The server must also be configured with max_wal_senders set high
> >> enough to leave at least one session available for the backup.
> >
> > I think that it's better to explain explicitly here that max_wal_senders
> > should be higher than one by default.
>
> Recovery tests are broken by this patch, the backup() method in
> PostgresNode.pm uses pg_basebackup -x:
> sub backup
> {
> my ($self, $backup_name) = @_;
> my $backup_path = $self->backup_dir . '/' . $backup_name;
> my $port= $self->port;
> my $name= $self->name;
>
> print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
> TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
> $port,
> '-x', '--no-sync');
> print "# Backup finished\n";
> }
>

Oh bleh. That's what I get for just running the tests for pg_basebackup
itself.

I removed it completely in this patch, making it use streaming. Or is there
a particular reason we want it to use fetch, that I'm not aware of?

Attached is a new patch with this fix, and those suggested by Fujii as well.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..b22b410 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -56,7 +56,7 @@ PostgreSQL documentation
and pg_hba.conf must explicitly permit the replication
connection. The server must also be configured
with  set high enough to leave at least
-   one session available for the backup.
+   one session available for the backup and one for WAL streaming (if used).
   
 
   
@@ -88,7 +88,7 @@ PostgreSQL documentation
   There is no guarantee that all WAL files required for the backup are archived
   at the end of backup. If you are planning to use the backup for an archive
   recovery and want to ensure that all required files are available at that moment,
-  you need to include them into the backup by using the -x option.
+  you need to include them into the backup by using the -X option.
  
 
 
@@ -285,27 +285,16 @@ PostgreSQL documentation
  
 
  
-  -x
-  --xlog
-  
-   
-Using this option is equivalent of using -X with
-method fetch.
-   
-  
- 
-
- 
   -X method
   --xlog-method=method
   

 Includes the required transaction log files (WAL files) in the
 backup. This will include all transaction logs generated during
-the backup. If this option is specified, it is possible to start
-a postmaster directly in the extracted directory without the need
-to consult the log archive, thus making this a completely standalone
-backup.
+the backup. Unless the method none is specified,
+it is possible to start a postmaster directly in the extracted
+directory without the need to consult the log archive, thus
+making this a completely standalone backup.


 The following methods for collecting the transaction logs are
@@ -313,6 +302,16 @@ PostgreSQL documentation
 
 
  
+  n
+  none
+  
+   
+Don't include transaction log in the backup.
+   
+  
+ 
+
+ 
   f
   fetch
   
@@ -349,6 +348,9 @@ PostgreSQL documentation
 named pg_wal.tar (if the server is a version
 earlier than 10, the file will be named pg_xlog.tar).

+   
+This value is the default.
+   
   
  
 
@@ -699,7 +701,7 @@ PostgreSQL documentation
To create a backup of a single-tablespace local database and compress
this with bzip2:
 
-$ pg_basebackup -D - -Ft | bzip2  backup.tar.bz2
+$ pg_basebackup -D - -Ft -X fetch | bzip2  backup.tar.bz2
 
(This command will fail if there are multiple tablespaces in the
database.)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 79b899a..3f79f7b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -71,8 +71,8 @@ static bool noclean = false;
 static bool showprogress = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
 static bool fastcheckpoint = false;
 static bool writerecoveryconf = false;
 static bool do_sync = true;
@@ -325,8 +325,7 @@ usage(void)
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to 

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

[Magnus Hagander]

On Tue, Dec 20, 2016 at 2:38 PM, Fujii Masao  wrote:

> On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov 
> wrote:
> >
> > On Sat, Dec 17, 2016 at 2:37 PM, Magnus Hagander 
> > wrote:
> >>
> >> Attached is an updated patch that does this. As a bonus it simplifies
> the
> >> code a bit. I also fixed an error message that I missed updating in the
> >> previous patch.
> >
> >
> > looks good to me.
>
> Yep, basically looks good to me. Here are some small comments.
>
> > while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nNzZ:d:c:h:p:U:
> s:S:wWvP",
> > long_options, _index)) != -1)
>
> 'x' should be removed from the above code.
>

Could've sworn I did that, but clearly not. Thanks.



> > To create a backup of a single-tablespace local database and compress
> this with bzip2:
> >
> > $ pg_basebackup -D - -Ft | bzip2 > backup.tar.bz2
>
> The above example in the docs needs to be modified. For example,
> add "-X fetch" into the above command so that pg_basebackup
> should not exit with an error.
>

Ah yes, good point as well.



> > The server must also be configured with max_wal_senders set high
> > enough to leave at least one session available for the backup.
>
> I think that it's better to explain explicitly here that max_wal_senders
> should be higher than one by default.
>

Added:
The server must also be configured
   with  set high enough to leave at
least
   one session available for the backup and one for WAL streaming (if used).


Will post new patch shortly, once I've addressed Michaels comments as well.

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

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

[Thomas Munro]

On Sat, Dec 3, 2016 at 1:38 AM, Haribabu Kommi  wrote:
> Moved to next CF with "waiting on author" status.

Unfortunately it's been a bit trickier than I anticipated to get the
interprocess batch file sharing and hash table shrinking working
correctly and I don't yet have a new patch in good enough shape to
post in time for the January CF.  More soon.

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


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

[HACKERS] tab complete regress tests

[Pavel Stehule]

Hi

now the code in tabcomplete become large part of psql. Is there some
possibility to write regress tests?

I found only this link

http://stackoverflow.com/questions/22795767/how-to-test-python-readline-completion

Regards

Pavel

[HACKERS] logical decoding of two-phase transactions

[Stas Kelvich]

Here is resubmission of patch to implement logical decoding of two-phase 
transactions (instead of treating them
as usual transaction when commit) [1] I’ve slightly polished things and used 
test_decoding output plugin as client.

General idea quite simple here:

* Write gid along with commit/prepare records in case of 2pc
* Add several routines to decode prepare records in the same way as it already 
happens in logical decoding.

I’ve also added explicit LOCK statement in test_decoding regression suit to 
check that it doesn’t break thing. If
somebody can create scenario that will block decoding because of existing dummy 
backend lock that will be great
help. Right now all my tests passing (including TAP tests to check recovery of 
twophase tx in case of failures from 
adjacent mail thread).

If we will agree about current approach than I’m ready to add this stuff to 
proposed in-core logical replication.

[1] 
https://www.postgresql.org/message-id/EE7452CA-3C39-4A0E-97EC-17A414972884%40postgrespro.ru



logical_twophase.diff
Description: Binary data


-- 
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



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