Re: [HACKERS] JIT & function naming

[Andres Freund]

Hi,

On 2017-08-31 23:41:31 -0700, Andres Freund wrote:
> I previously had an early prototype of JITing [1] expression evaluation
> and tuple deforming.  I've since then worked a lot on this.
>
> Here's an initial, not really pretty but functional, submission.

One of the things I'm not really happy about yet is the naming of the
generated functions. Those primarily matter when doing profiling, where
the function name will show up when the profiler supports JIT stuff
(e.g. with a patch I proposed to LLVM that emits perf compatible output,
there's also existing LLVM support for a profiler by intel and
oprofile).

Currently there's essentially a per EState counter and the generated
functions get named deform$n and evalexpr$n. That allows for profiling
of a single query, because different compiled expressions are
disambiguated. It even allows to run the same query over and over, still
giving meaningful results.  But it breaks down when running multiple
queries while profiling - evalexpr0 can mean something entirely
different for different queries.

The best idea I have so far would be to name queries like
evalexpr_$fingerprint_$n, but for that we'd need fingerprinting support
outside of pg_stat_statement, which seems painful-ish.

Perhaps somebody has a better idea?

Regards,

Andres


-- 
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] Function to move the position of a replication slot

[Andres Freund]

On 2017-09-02 18:31:10 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I don't quite see how you'd get corruption from a physical slot being
> > forwarded? I mean you surely can get into the situation that there's
> > missing WAL from wherever a standby is receiving its WAL, but that'll
> > "just" break replication.
> 
> Um, doesn't advancing a slot correspond exactly to skipping some amount
> of WAL?

Not for physical ones, no. The slot is just a marker on the *upstream*
(or a potential upstream) that remembers a standby's current WAL replay
position and, if enabled, it's current xmin. The prevents the upstream
to remove the WAL that the standby still need and if applicable vacuum
from removing rows the standby needs.  If the slot is at the wrong
position exactly the same things that can happen if no slot were in use
can also happen, i.e. "ERROR: requested WAL segment %s has already been 
removed".

For logical replication such a forward operation would have to be *more*
complicated than for physical rep, because the state that's kept is more
complicated...

- Andres


-- 
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] Function to move the position of a replication slot

[Tom Lane]

Andres Freund  writes:
> I don't quite see how you'd get corruption from a physical slot being
> forwarded? I mean you surely can get into the situation that there's
> missing WAL from wherever a standby is receiving its WAL, but that'll
> "just" break replication.

Um, doesn't advancing a slot correspond exactly to skipping some amount
of WAL?

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] Function to move the position of a replication slot

[Andres Freund]

On 2017-09-01 23:37:07 -0400, Tom Lane wrote:
> Peter Eisentraut  writes:
> > On 8/31/17 08:19, Magnus Hagander wrote:
> >> I think that, in the end, covered all the comments?
> 
> > I didn't see any explanation of what this would actually be useful for.
> > I suppose you could skip over some changes you don't want replicated,
> > but how do you find to what position to skip?
> 
> Um ... I can see how you might expect to skip some events in a logical
> replication stream and have a chance of things not being utterly broken.
> But how can that work for physical replication?  Missed updates are
> normally spelled "unrecoverable data corruption" at that level.

Consider e.g. a standby that follows master, but isn't a target for a
failover. It can make a fair bit of sense to script things so that
there's also a slot on the standby that's marked to be the primary in
disaster cases. For that you might want to forward the slot on a regular
basis.

I don't quite see how you'd get corruption from a physical slot being
forwarded? I mean you surely can get into the situation that there's
missing WAL from wherever a standby is receiving its WAL, but that'll
"just" break replication.

- Andres


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

Re: [HACKERS] Parallel worker error

[Robert Haas]

On Sat, Sep 2, 2017 at 2:51 AM, Noah Misch  wrote:
> [Action required within three days.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 10 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

It's not really a valid v10 open item, because it's not new in v10.
But I agree it should be fixed soon.

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


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

Re: [HACKERS] Function to move the position of a replication slot

[Robert Haas]

On Fri, Sep 1, 2017 at 11:30 PM, Peter Eisentraut
 wrote:
> On 8/31/17 08:19, Magnus Hagander wrote:
>> Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
>> Forward only.
>>
>> I think that, in the end, covered all the comments?
>
> I didn't see any explanation of what this would actually be useful for.
> I suppose you could skip over some changes you don't want replicated,
> but how do you find to what position to skip?
>
> Logical replication has a similar mechanism, using the function
> pg_replication_origin_advance().  Any overlap there?  (Maybe the names
> could be aligned.)
> (https://www.postgresql.org/docs/devel/static/logical-replication-conflicts.html)

I think you can use this to work around the absence of failover slots.

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


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

Re: [HACKERS] adding the commit to a patch's thread

[Daniel Gustafsson]

> On 01 Sep 2017, at 15:40, Robert Haas  wrote:
> 
> On Fri, Sep 1, 2017 at 4:26 AM, Alvaro Herrera  
> wrote:
>> Erik Rijkers wrote:
>>> Would it be possible to change the commitfest a bit and make it possible to
>>> add the commit (or commit-message, or hash) to the thread in the
>>> commitfest-app.
>> 
>> +1 to add one or more commit hashes to CF entry metadata.
>> 
>> (Back-filling for old entries welcome)
> 
> Couldn't the CF app scrape the commit messages for references to
> threads, and if the commit message points to a thread with exactly 1
> patch record, associate the commit to that patch?

Since there is a Gitlink field in the patch metadata, we could start simple
with setting that to link to the commit on pg gitweb?  While adding the thread
of the commit from -committers would be great as well, this gets us off the
ground quickly.

Unless there are objections, I’ll ensure that this is maintained during this CF
as part of my commitfest tinkering.

cheers ./daniel

-- 
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] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

I wrote:
> My thought is that what we need to do is find a way for isTopLevel
> to be false if we're processing a multi-command string.

Nah, that's backwards, the problem is exactly that isTopLevel is
false if we're processing a multi-command string.  That allows
DefineSavepoint to think that it's inside a function, and we don't
disallow savepoints inside functions.  (Or at least, xact.c doesn't
enforce any such prohibition; it's up to spi.c and the individual PLs
to decide if they could support that.)

After contemplating my navel for awhile, I think that this case proves
that the quick hack embodied in commit 4f896dac1 is inadequate.  Rather
than piling another quick hack on top and hoping that the result is OK,
I think it's time to bite the bullet and represent the behavior we want
explicitly in the transaction machinery.  Accordingly, PFA a patch
that invents a notion of an "implicit" transaction block.

I also added a bunch of test cases exercising the behavior.  Except
for the problem of FATAL exits for savepoint commands, all these
cases work exactly like they do in unpatched code.  However, now that
we have an explicit representation, it'd be easy to tweak the behavior
if we want to.  For instance, I'm not entirely sure whether we want
the behavior that COMMIT and ROLLBACK in this state print warnings.
Good luck changing that before; but now it'd be a straightforward
adjustment.

I'm inclined to complete the reversion of 4f896dac1 by also undoing
its error message text change in PreventTransactionChain,

- errmsg("%s cannot be executed from a function", stmtType)));
+ errmsg("%s cannot be executed from a function or 
multi-command string",
+stmtType)));

but this patch doesn't include that change.

My feeling about this is that we don't need a back-patch.  Throwing
FATAL rather than ERROR for a misplaced savepoint command is a bit
unpleasant, but it doesn't break other sessions, and the upshot is
really the same: don't do that.

regards, tom lane

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5e7e812..ba4b2da 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** typedef enum TBlockState
*** 145,150 
--- 145,151 
  	/* transaction block states */
  	TBLOCK_BEGIN,/* starting transaction block */
  	TBLOCK_INPROGRESS,			/* live transaction */
+ 	TBLOCK_IMPLICIT_INPROGRESS, /* live transaction after implicit BEGIN */
  	TBLOCK_PARALLEL_INPROGRESS, /* live transaction inside parallel worker */
  	TBLOCK_END,	/* COMMIT received */
  	TBLOCK_ABORT,/* failed xact, awaiting ROLLBACK */
*** StartTransactionCommand(void)
*** 2700,2705 
--- 2701,2707 
  			 * previous CommitTransactionCommand.)
  			 */
  		case TBLOCK_INPROGRESS:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  		case TBLOCK_SUBINPROGRESS:
  			break;
  
*** CommitTransactionCommand(void)
*** 2790,2795 
--- 2792,2798 
  			 * counter and return.
  			 */
  		case TBLOCK_INPROGRESS:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  		case TBLOCK_SUBINPROGRESS:
  			CommandCounterIncrement();
  			break;
*** AbortCurrentTransaction(void)
*** 3014,3023 
  			break;
  
  			/*
! 			 * if we aren't in a transaction block, we just do the basic abort
! 			 * & cleanup transaction.
  			 */
  		case TBLOCK_STARTED:
  			AbortTransaction();
  			CleanupTransaction();
  			s->blockState = TBLOCK_DEFAULT;
--- 3017,3028 
  			break;
  
  			/*
! 			 * If we aren't in a transaction block, we just do the basic abort
! 			 * & cleanup transaction.  For this purpose, we treat an implicit
! 			 * transaction block as if it were a simple statement.
  			 */
  		case TBLOCK_STARTED:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  			AbortTransaction();
  			CleanupTransaction();
  			s->blockState = TBLOCK_DEFAULT;
*** BeginTransactionBlock(void)
*** 3429,3434 
--- 3434,3448 
  			break;
  
  			/*
+ 			 * BEGIN converts an implicit transaction block to a regular one.
+ 			 * (Note that we allow this even if we've already done some
+ 			 * commands, which is a bit odd but matches historical practice.)
+ 			 */
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
+ 			s->blockState = TBLOCK_BEGIN;
+ 			break;
+ 
+ 			/*
  			 * Already a transaction block in progress.
  			 */
  		case TBLOCK_INPROGRESS:
*** PrepareTransactionBlock(char *gid)
*** 3503,3509 
  			 * ignore case where we are not in a transaction;
  			 * EndTransactionBlock already issued a warning.
  			 */
! 			Assert(s->blockState == TBLOCK_STARTED);
  			/* Don't send back a PREPARE result tag... */
  			result = false;
  		}
--- 3517,3524 
  			 * ignore case where we are not in a transaction;
  			 * EndTransactionBlock already issued a warning.
  			 */
! 			Assert(s->blockState == TBLOCK_STARTED ||
!    s->blockState == 

Re: [HACKERS] [WIP] Zipfian distribution in pgbench

[Alik Khilazhev]

Hello Fabien,

Thank you for detailed review. I hope I have fixed all the issues you mentioned 
in your letter.



pgbench-zipf-08v.patch
Description: Binary data

—
Thanks and Regards,
Alik Khilazhev
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

Re: [HACKERS] GnuTLS support

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:

> >> There are currently two failing SSL tests which at least to me seems more
> >> like they test specific OpenSSL behaviors rather than something which need
> >> to be true for all SSL libraries.
> 
> > I don't know what we should do about these issues.
> 
> Maybe the SSL test suite needs to be implementation-specific as well.

If only two tests fail currently, I suggest that the thing to do is to
split it up in generic vs. library-specific test files.  It should be
easy to do with the TAP framework -- just move the library-specific
tests to their own file and mark it as skipped at the start of the file
when a different library is detected.

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


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

[HACKERS] Exclude pg_internal.init from base backup

[David Steele]

Hackers,

The cache in pg_internal.init was reused in days of yore but has been
rebuilt on postmaster startup since v8.1.  It appears there is no reason
for this file to be backed up.

I also moved the RELCACHE_INIT_FILENAME constant to relcache.h to avoid
duplicating the string.

I'll add this to the 2017-11 CF.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 95aeb35507..c3e6c30eba 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1130,6 +1130,12 @@ SELECT pg_stop_backup();

 

+The pg_internal.init file can be omitted from the
+backup no matter what directory it appears in.  This file contains a
+relation cache that is always rebuilt on startup.
+   
+
+   
 The backup label
 file includes the label string you gave to pg_start_backup,
 as well as the time at which pg_start_backup was run, and
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 2bb4e38a9d..46748bfad0 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2384,6 +2384,11 @@ The commands accepted in walsender mode are:


 
+ pg_internal.init
+
+   
+   
+
  Various temporary files and directories created during the operation
  of the PostgreSQL server, such as any file or directory beginning
  with pgsql_tmp.
diff --git a/src/backend/replication/basebackup.c 
b/src/backend/replication/basebackup.c
index 9776858f03..2518774fb7 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -36,6 +36,7 @@
 #include "utils/builtins.h"
 #include "utils/elog.h"
 #include "utils/ps_status.h"
+#include "utils/relcache.h"
 #include "utils/timestamp.h"
 
 
@@ -151,6 +152,9 @@ static const char *excludeFiles[] =
/* Skip current log file temporary file */
LOG_METAINFO_DATAFILE_TMP,
 
+   /* Skip relation cache because it is rebuilt on startup */
+   RELCACHE_INIT_FILENAME,
+
/*
 * If there's a backup_label or tablespace_map file, it belongs to a
 * backup started by the user with pg_start_backup().  It is *not* 
correct
diff --git a/src/backend/utils/cache/relcache.c 
b/src/backend/utils/cache/relcache.c
index b8e37809b0..5015719915 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -87,11 +87,6 @@
 #include "utils/tqual.h"
 
 
-/*
- * name of relcache init file(s), used to speed up backend startup
- */
-#define RELCACHE_INIT_FILENAME "pg_internal.init"
-
 #define RELCACHE_INIT_FILEMAGIC0x573266/* version ID 
value */
 
 /*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl 
b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index a00f7b0e1a..d95ea3e0d5 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -4,7 +4,7 @@ use Cwd;
 use Config;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 72;
+use Test::More tests => 73;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -61,6 +61,11 @@ foreach my $filename (
close $file;
 }
 
+# Connect to a database to create global/pg_internal.init.  If this is removed
+# the test to ensure global/pg_internal.init is not copied will return a false
+# positive.
+$node->safe_psql('postgres', 'SELECT 1;');
+
 $node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup", '-X', 'none' ],
'pg_basebackup runs');
 ok(-f "$tempdir/backup/PG_VERSION", 'backup was created');
@@ -84,7 +89,8 @@ foreach my $dirname (
 
 # These files should not be copied.
 foreach my $filename (
-   qw(postgresql.auto.conf.tmp postmaster.opts postmaster.pid 
tablespace_map current_logfiles.tmp)
+   qw(postgresql.auto.conf.tmp postmaster.opts postmaster.pid 
tablespace_map current_logfiles.tmp
+   global/pg_internal.init)
   )
 {
ok(!-f "$tempdir/backup/$filename", "$filename not copied");
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 3c53cefe4b..29c6d9bae3 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -18,6 +18,11 @@
 #include "nodes/bitmapset.h"
 
 
+/*
+ * Name of relcache init file(s), used to speed up backend startup
+ */
+#define RELCACHE_INIT_FILENAME "pg_internal.init"
+
 typedef struct RelationData *Relation;
 
 /* 

-- 
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] Secondary index access optimizations

[Konstantin Knizhnik]

On 09/02/2017 06:44 AM, Thomas Munro wrote:

On Wed, Aug 16, 2017 at 9:23 PM, Konstantin Knizhnik
 wrote:

postgres=# explain select * from bt where k between 1 and 2 and v = 100;
   QUERY PLAN
--
  Append  (cost=0.29..15.63 rows=2 width=8)
->  Index Scan using dti1 on dt1  (cost=0.29..8.30 rows=1 width=8)
  Index Cond: (v = 100)
->  Index Scan using dti2 on dt2  (cost=0.29..7.33 rows=1 width=8)
  Index Cond: (v = 100)
  Filter: (k <= 2)
(6 rows)

+1

This seems like a good feature to me: filtering stuff that is
obviously true is a waste of CPU cycles and may even require people to
add redundant stuff to indexes.  I was pondering something related to
this over in the partition-wise join thread (join quals that are
implied by partition constraints and should be discarded).

It'd be interesting to get Amit Langote's feedback, so I CC'd him.
I'd be surprised if he and others haven't got a plan or a patch for
this down the back of the sofa.

I might be missing some higher level architectural problems with the
patch, but for what it's worth here's some feedback after a first read
through:

--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1441,6 +1441,20 @@ relation_excluded_by_constraints(PlannerInfo *root,
  if (predicate_refuted_by(safe_constraints, rel->baserestrictinfo, false))
  return true;

+/*
+ * Remove from restrictions list items implied by table constraints
+ */
+safe_restrictions = NULL;
+foreach(lc, rel->baserestrictinfo)
+{
+RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);

I think the new way to write this is "RestrictInfo *rinfo =
lfirst_node(RestrictInfo, lc)".

+if (!predicate_implied_by(list_make1(rinfo->clause),
safe_constraints, false)) {
+safe_restrictions = lappend(safe_restrictions, rinfo);
+}
+}
+rel->baserestrictinfo = safe_restrictions;

It feels wrong to modify rel->baserestrictinfo in
relation_excluded_by_constraints().  I think there should probably be
a function with a name that more clearly indicates that it mutates its
input, and you should call that separately after
relation_excluded_by_constraints().  Something like
remove_restrictions_implied_by_constraints()?


It is because operator_predicate_proof is not able to understand that k <
20001 and k <= 2 are equivalent for integer type.

[...]

  /*
   * operator_predicate_proof
  if (clause_const->constisnull)
  return false;

+if (!refute_it
+&& ((pred_op == Int4LessOrEqualOperator && clause_op ==
Int4LessOperator)
+|| (pred_op == Int8LessOrEqualOperator && clause_op ==
Int8LessOperator)
+|| (pred_op == Int2LessOrEqualOperator && clause_op ==
Int2LessOperator))
+&& pred_const->constbyval && clause_const->constbyval
+&& pred_const->constvalue + 1 == clause_const->constvalue)
+{
+return true;
+}
+

I'm less sure about this part.  It seems like a slippery slope.

A couple of regression test failures:

  inherit  ... FAILED
  rowsecurity  ... FAILED

  2 of 179 tests failed.


I didn't try to understand the rowsecurity one, but at first glance
all the differences reported in the inherit test are in fact cases
where your patch is doing the right thing and removing redundant
filters from scans.  Nice!


Thank you for review.
I attached new version of the patch with 
remove_restrictions_implied_by_constraints() function.
Concerning failed tests - this is actually result of this optimization: extra 
filter conditions are removed from query plans.
Sorry, I have not included updated version of expected test output files to the 
patch.
Now I did it.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 2d7e1d8..5de67ce 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -344,6 +344,7 @@ set_rel_size(PlannerInfo *root, RelOptInfo *rel,
 		switch (rel->rtekind)
 		{
 			case RTE_RELATION:
+remove_restrictions_implied_by_constraints(root, rel, rte);
 if (rte->relkind == RELKIND_FOREIGN_TABLE)
 {
 	/* Foreign table */
@@ -1047,6 +1048,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
 			set_dummy_rel_pathlist(childrel);
 			continue;
 		}
+		remove_restrictions_implied_by_constraints(root, childrel, childRTE);
 
 		/*
 		 * CE failed, so finish copying/modifying targetlist and join quals.
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index a1ebd4a..fc663d6 100644
--- a/src/backend/optimizer/util/plancat.c
+++ 

Re: [HACKERS] Parallel worker error

[Amit Kapila]

On Sat, Sep 2, 2017 at 12:21 PM, Noah Misch  wrote:
> On Thu, Aug 31, 2017 at 03:11:10PM -0400, Robert Haas wrote:
>> On Wed, Aug 30, 2017 at 11:19 AM, Robert Haas  wrote:
>> > But since that's an established design fl^H^Hprinciple, maybe that
>> > means we should go with the approach of teaching SerializeGUCState()
>> > to ignore role altogether and instead have ParallelWorkerMain call
>> > SetCurrentRoleId using information passed via the FixedParallelState
>> > (not sure of the precise details here).
>>
>> Could I get some opinions on the virtues of this approach, vs. any of
>> the other suggestions at or near
>> http://postgr.es/m/ca+tgmoasp90e33-mu2ypgs73ttj37m5hv-xqhjc7tpqx9wx...@mail.gmail.com
>> ?
>
> It seems good to me, better than the other options in that mail.  This does
> rely on "role" being on the only vulnerable GUC.  Looking at callers of
> GUC_check_errmsg(), I don't expect trouble in any other GUC.  (I suspect one
> can hit the "permission denied to set role" during parallel initialization
> after a concurrent ALTER ROLE removes a membership.)
>

I think that error won't happen during parallel initialization because
of 'InitializingParallelWorker' check.


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


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

Re: [HACKERS] Parallel worker error

[Amit Kapila]

On Wed, Aug 30, 2017 at 8:49 PM, Robert Haas  wrote:
> On Wed, Aug 30, 2017 at 9:58 AM, Tom Lane  wrote:
>> The problem here is exactly that we cannot transmit the leader's
>> state to the worker.  You can't blame it on SET ROLE, because
>> I didn't do one.
>
> Hmm, that's a good reason for holding it blameless.  In this case,
> I'll blame the fact that we allow a role to be dropped while there are
> users connected using that role.
>

The similar could happen with schema as well.  For example, even if
you have set the schema name in the search_path of session-1, it will
still be allowed to drop the schema from another session.  Now, we
will pass the dropped schema name as part of search_path to the
parallel worker.  It won't lead to any error because we don't check
catalog while setting the value of search_path GUC.

I am not sure whether we need to bother about this, but I thought it
might help in choosing the approach to fix this problem.


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


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

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

On Fri, Sep 1, 2017 at 7:17 PM, Thomas Munro
 wrote:
> On Sat, Aug 26, 2017 at 7:32 AM, Robert Haas  wrote:
>> On Sat, Apr 8, 2017 at 10:05 AM, David Steele  wrote:
>>> On 3/28/17 1:21 AM, Tsunakawa, Takayuki wrote:
 Thanks, marked as ready for committer, as the code is good and Alexander 
 reported the test success.
>>>
>>> This bug has been moved to CF 2017-07.
>>
>> This bug fix has been pending in "Ready for Committer" state for about
>> 4.5 months.  Three committers (Magnus, Heikki, Tom) have contributed
>> to the thread to date.  Maybe one of them would like to commit this?
>
> In the meantime its bits have begun to rot.  Michael, could you please rebase?

Thanks for the reminder, Thomas. The 2PC code triggered during
recovery has been largely reworked in PG10, explaining the conflicts.
As this has been some time since I touched this patch, I had again a
look at its logic and could not find any problems around it. So
attached are a rebased versions for both 0001 and 0002, I have updated
the messages as well to be more in-line with what is in HEAD for
corrupted entries.
-- 
Michael
From 2f3a16c0c0cb12f8bfef2d58656352c46a681393 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sat, 2 Sep 2017 21:15:04 +0900
Subject: [PATCH 2/2] Minimize window between history file and end-of-recovery
 record

Once a standby node is promoted, this makes the assignment of the new
timeline number booked earlier as the history file gets archived immediately.
This way the other nodes are aware that this new timeline number is taken
and should not be assigned to other nodes.

The window between which the history file is archived and the end-of-recovery
record is written cannot be zeroed, but this way it is minimized as much as
possible. The new order of actions prevents as well a corrupted data directory
on failure.
---
 src/backend/access/transam/xlog.c | 27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 80fe2c60e6..abc0cec3d2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7449,6 +7449,24 @@ StartupXLOG(void)
 		else
 			snprintf(reason, sizeof(reason), "no recovery target specified");
 
+		/*
+		 * We are now done reading the old WAL.  Turn off archive fetching
+		 * if it was active, and make a writable copy of the last WAL segment.
+		 * (Note that we also have a copy of the last block of the old WAL
+		 * in readBuf; we will use that below.)
+		 */
+		exitArchiveRecovery(EndOfLogTLI, EndOfLog);
+
+		/*
+		 * Write the timeline history file, and have it archived. After this
+		 * point (or rather, as soon as the file is archived), the timeline
+		 * will appear as "taken" in the WAL archive and to any standby servers.
+		 * If we crash before actually switching to the new timeline, standby
+		 * servers will nevertheless think that we switched to the new timeline,
+		 * and will try to connect to the new timeline. To minimize the window
+		 * for that, try to do as little as possible between here and writing the
+		 * end-of-recovery record.
+		 */
 		writeTimeLineHistory(ThisTimeLineID, recoveryTargetTLI,
 			 EndRecPtr, reason);
 	}
@@ -7457,15 +7475,6 @@ StartupXLOG(void)
 	XLogCtl->ThisTimeLineID = ThisTimeLineID;
 	XLogCtl->PrevTimeLineID = PrevTimeLineID;
 
-	/*
-	 * We are now done reading the old WAL.  Turn off archive fetching if it
-	 * was active, and make a writable copy of the last WAL segment. (Note
-	 * that we also have a copy of the last block of the old WAL in readBuf;
-	 * we will use that below.)
-	 */
-	if (ArchiveRecoveryRequested)
-		exitArchiveRecovery(EndOfLogTLI, EndOfLog);
-
 	/*
 	 * Prepare to write WAL starting at EndOfLog location, and init xlog
 	 * buffer cache using the block containing the last record from the
-- 
2.14.1

From 12ab1765f7ea032db217f2081f9561c00d1d96df Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sat, 2 Sep 2017 21:07:21 +0900
Subject: [PATCH 1/2] Change detection of corrupted 2PC files as FATAL

When scanning 2PC files, be it for initializing a hot standby node or
finding the XID range at the end of recovery, bumping on a corrupted
2PC file is reported as a WARNING and are blindly corrupted. If it
happened that the corrupted file was actually legit, there is actually
a risk of corruption, so switch that to a hard failure.
---
 src/backend/access/transam/twophase.c | 27 +++
 src/backend/access/transam/xlog.c | 10 +++---
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index ba03d9687e..4b4f2449f8 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1778,6 +1778,10 @@ restoreTwoPhaseData(void)
  * 

Re: [HACKERS] Rename RECOVERYXLOG to RECOVERYWAL?

[David Steele]

On 9/1/17 7:53 PM, Michael Paquier wrote:
> On Sat, Sep 2, 2017 at 3:06 AM, Robert Haas  wrote:
>> I don't think this really buys us anything.  If we'd applied it to v10
>> maybe, but what do we get out of whacking it around now?
>>
>> "Consistency", I hear you cry!  Fair point.  But we never had a goal
>> of eliminating all internal references to "xlog", just the user-facing
>> ones.  And since RECOVERYXLOG is not documented, I think there's a
>> good argument that it's not user-facing.  You could argue that since
>> it shows up in the file system it's implicitly user-facing, and maybe
>> you're right; if some other committer really wants to make this
>> change, I won't grouse much.  But personally I'd favor leaving it
>> alone to avoid having the behavior change a little bit in every new
>> release.
> 
> I may be wrong, but I would suspect that some backup tools doing
> FS-level backup are checking on the existence of this file and skip
> it. From the point of view of operations, it does not matter much as
> any existing RECOVERYXLOG is unlinked before being replaced by a new
> one, but that would not be nice to add silently 16MB in each backup.

Yes, pgBackRest does have an "offline" mode that can be used (when the
database is shutdown) to do an FS-level backup.

It never occurred to me to exclude RECOVERYXLOG but with 1GB WAL
segments coming in v11 it might be a good idea.

-- 
-David
da...@pgmasters.net


-- 
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] configure issue - warnings sort: No such file or directory

[Pavel Stehule]

2017-09-02 13:28 GMT+02:00 Devrim Gündüz :

>
> Hi,
>
> On Sat, 2017-09-02 at 06:52 +0200, Pavel Stehule wrote:
> > but looks so it is Fedora26 issue - I see these lines elsewhere too.
>
> I cannot reproduce it on my F26 box.
>

I have to do deep research

Thank you for info

Pavel

>
> Regards,
> --
> Devrim Gündüz
> EnterpriseDB: https://www.enterprisedb.com
> PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
> Twitter: @DevrimGunduz , @DevrimGunduzTR
>

Re: [HACKERS] configure issue - warnings sort: No such file or directory

[Devrim Gündüz]

Hi,

On Sat, 2017-09-02 at 06:52 +0200, Pavel Stehule wrote:
> but looks so it is Fedora26 issue - I see these lines elsewhere too.

I cannot reproduce it on my F26 box.

Regards,
-- 
Devrim Gündüz
EnterpriseDB: https://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR


signature.asc
Description: This is a digitally signed message part

Re: [HACKERS] pg_basebackup throttling doesn't throttle as promised

[Antonin Houska]

Jeff Janes  wrote:

> On Fri, Sep 1, 2017 at 1:32 PM, Jeff Janes  wrote:
> 
>  The "-r" option to pg_basebackup is supposed to throttle the rate of the
>  backup. But it only works properly if the server is mostly idle.
> 
>  Every non-trivial call to XLogFlush or XLogBackgroundFlush will wake up the
>  wal sender (the one which is not really sending wal, but base files), and
>  the throttling routine doesn't go back to sleep after being awoke
>  early. Rather, it releases another 32kb of data.

Sorry, I missed this fact when coding the feature.

>  Should the basebackup.c throttle sleep in a loop until its time has
>  expired?

I think this is the correct approach because latch can be set for unrelated
reasons.

The patch makes sense to me. I just recommend moving this part in front of the
loop because elapsed_min does not have to be re-computed each time:

/* How much should have elapsed at minimum? */
elapsed_min = elapsed_min_unit * (throttling_counter / throttling_sample);

And also a comment explaining the purpose of the loop would be
appreciated. Thanks.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.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] Parallel worker error

[Noah Misch]

On Thu, Aug 31, 2017 at 03:11:10PM -0400, Robert Haas wrote:
> On Wed, Aug 30, 2017 at 11:19 AM, Robert Haas  wrote:
> > But since that's an established design fl^H^Hprinciple, maybe that
> > means we should go with the approach of teaching SerializeGUCState()
> > to ignore role altogether and instead have ParallelWorkerMain call
> > SetCurrentRoleId using information passed via the FixedParallelState
> > (not sure of the precise details here).
> 
> Could I get some opinions on the virtues of this approach, vs. any of
> the other suggestions at or near
> http://postgr.es/m/ca+tgmoasp90e33-mu2ypgs73ttj37m5hv-xqhjc7tpqx9wx...@mail.gmail.com
> ?

It seems good to me, better than the other options in that mail.  This does
rely on "role" being on the only vulnerable GUC.  Looking at callers of
GUC_check_errmsg(), I don't expect trouble in any other GUC.  (I suspect one
can hit the "permission denied to set role" during parallel initialization
after a concurrent ALTER ROLE removes a membership.)


[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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