Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-09-27 Thread Michael Banck
Hi,

Am Mittwoch, den 27.09.2017, 10:10 -0400 schrieb Peter Eisentraut:
> On 9/11/17 03:11, Michael Banck wrote:
> > So my patch only moves the slot creation slightly further forward,
> > AFAICT.
> 
> I have committed this patch, along with some associated cleanup.

Thanks!

> > AIUI, wal streaming always begins at last checkpoint and from my tests
> > the restart_lsn of the created replication slot is also before that
> > checkpoint's lsn. However, I hope somebody more familiar with the
> > WAL/replication slot code could comment on that.  What I dropped in the
> > refactoring is the RESERVE_WAL that used to be there when the temporary
> > slot gets created, I have readded that now.
> 
> I had to make some changes to that, because the way your patch was
> written it would use RESERVE_WAL also for the calls from pg_receivewal
> --create-slot, which would have been a behavior change.  So I added
> another argument to CreateReplicationSlot() to control that.

Euh right, thanks for catching that.

> > I also added a TAP test case that tries to check that the restart_lsn is
> > lower than the checkpoint_lsn, which appears to be the case.
> 
> I think that test was written incorrectly, because it didn't actually
> check the $checkpoint_lsn that it read.  I have not included that test
> in the committed patch.  Feel free to send another patch if you want to
> add more or different tests.

I will take a look at that.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

2017-09-13 Thread Michael Banck
On Tue, Sep 12, 2017 at 07:38:40PM -0400, Stephen Frost wrote:
> Further, really, I think we should provide a utility to do all of the
> above instead of using rsync- and that utility should do some additional
> things, such as:
> 
> - Check that the control file on the primary and replica show that they
>   reached the same point prior to the pg_upgrade.  If they didn't, then
>   things could go badly as there's unplayed WAL that the primary got
>   through and the replica didn't.
> 
> - Not copy over unlogged data, or any other information that shouldn't
>   be copied across.
> 
> - Allow the directory structures to be more different between the
>   primary and the replica than rsync allows (wouldn't have to have a
>   common subdirectory on the replica).
> 
> - Perhaps other validation checks or similar.
> 
> Unfortunately, this is a bit annoying as it necessairly involves running
> things on both the primary and the replica from the same tool, without
> access to PG, meaning we'd have to work through something else (such as
> SSH, like rsync does, but then what would we do for Windows...?).

Maybe pg_rewind's mechanism could be partially reused for this as it
seems to accomplish something vaguely similar AIUI?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-09-12 Thread Michael Banck
Hi,

Am Dienstag, den 12.09.2017, 08:53 -0400 schrieb Peter Eisentraut:
> On 9/11/17 03:11, Michael Banck wrote:
> > > Is there a race condition here?  The slot is created after the checkpoint
> > > is completed.  But you have to start streaming from the LSN where the
> > > checkpoint started, so shouldn't the slot be created before the checkpoint
> > > is started?
> > 
> > So my patch only moves the slot creation slightly further forward,
> > AFAICT.
> > 
> > AIUI, wal streaming always begins at last checkpoint and from my tests
> > the restart_lsn of the created replication slot is also before that
> > checkpoint's lsn. However, I hope somebody more familiar with the
> > WAL/replication slot code could comment on that.  What I dropped in the
> > refactoring is the RESERVE_WAL that used to be there when the temporary
> > slot gets created, I have readded that now.
> 
> Maybe there is an argument to be made here about whether this is correct
> or not, but why bother and risk the fragility?  Why not create the
> replication slot first thing.  I would put it after the server version
> checks and before we write recovery.conf.

The replication slots are created via the replication protocol through
the second background connection that is used for WAL streaming in
StartLogStreamer().

By their nature temporary replication slots must be created by that WAL
streamer using them and cannot be created by the main connection which
initiates the snapshot (or else you get a "replication slot
"pg_basebackup_XXX" is active for PID XXX" error in the WAL streamer).
So ISTM we cannot rip out CreateReplicationSlot() (or the manual
CREATE_REPLICATION_SLOT that is currently in master) from
StartLogStreamer() at least for temporary slots.

We could split up the logic here and create the optional physical
replication slot in the main connection and the temporary one in the WAL
streamer connection, but this would keep any fragility around for
(likely more frequently used) temporary replication slots. It would make
the patch much smaller though if I revert touching temporary slots at
all.

The alternative would be to call StartLogStreamer() earlier, but it
requires xlogstart as argument so we cannot move its call before the
checkpoint is taken and xlogstart is determined, the earliest I managed
was when starttli is determined, which is just few instructions earlier
than now.

Thoughts?


Michael 

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-09-11 Thread Michael Banck
Hi,


On Fri, Sep 08, 2017 at 08:41:56AM +0200, Michael Banck wrote:
> Am Mittwoch, den 06.09.2017, 12:22 -0400 schrieb Peter Eisentraut:
> > On 8/18/17 05:28, Michael Banck wrote:
> > > > > Rebased, squashed and slighly edited version attached. I've added this
> > > > > to the 2017-07 commitfest now as well:
> > > > > 
> > > > > https://commitfest.postgresql.org/14/1112/
> > > > 
> > > > Can you rebase this past some conflicting changes?
> > > 
> > > Thanks for letting me know, PFA a rebased version.
> > 
> > I have reviewed the thread so far.  I think there is general agreement
> > that something like this would be good to have.
> > 
> > I have not found any explanation, however, why the "if not exists"
> > behavior is desirable, let alone as the default.  I can only think of
> > two workflows here:  Either you have scripts for previous PG versions
> > that create the slot externally, in which can you omit --create, or you
> > use the new functionality to have pg_basebackup create the slot.  I
> > don't see any use for pg_basebackup to opportunistically use a slot if
> > it happens to exist.  Even if there is one, it should not be the
> > default.  So please change that.
> 
> Ok, I tried to research why that was the case and couldn't find any
> trace of a discussion either.
> 
> So we should just error out in CreateReplicationSlot() in case a slot
> exists, right? I think having yet another option like --create-if-not-
> exists does not sound needed from what you wrote above.
> 
> > A minor point, I suggest to print the message about the replication slot
> > being created *after* the slot has been created.  This aligns with how
> > logical replication subscriptions work.  
> 
> Ok.
> 
> > I don't see the need for printing a message about temporary slots. 
> > Since they are temporary, they will go away automatically, so there is
> > nothing the user needs to know there.
> 
> Ok. I thought I'd remembered some request around having this reported
> always (maybe from Magnus), but I couldn't find anything in the prior
> discussions either.
> 
> If we don't print the message for temporary slots, then the
> CreateReplicationSlot() refactoring and the addition of the
> temp_replication_slot argument would be no longer needed, or is this
> something useful on its own?

New reworked and rebased patch attached, including setting RESERVE_WAL
for physical replication slots and the additional TAP test comparing the
restart_lsn with the checkpoint_lsn.

The only thing from the above I did not change yet is the removal of the
temporary slots creation message in verbose mode.  I have the feeling
knowing the temporary slot name could be helpful for debugging and
pg_basebackup is already pretty chatty in verbose mode so I preferred to
keep it, but I can remove it of course if that is the consensus.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From fca622f6f7c6ef9e566c5a1b72044b824e7e02cc Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 10 Sep 2017 18:07:33 +0200
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it unless
it exists already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 56 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 15 +---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 34 +++--
 8 files changed, 107

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-09-11 Thread Michael Banck
Hi,

On Fri, Sep 08, 2017 at 10:30:20AM -0700, Jeff Janes wrote:
> On Wed, Sep 6, 2017 at 9:22 AM, Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com> wrote:
> 
> > On 8/18/17 05:28, Michael Banck wrote:
> > >>> Rebased, squashed and slighly edited version attached. I've added this
> > >>> to the 2017-07 commitfest now as well:
> > >>>
> > >>> https://commitfest.postgresql.org/14/1112/
> > >> Can you rebase this past some conflicting changes?
> > > Thanks for letting me know, PFA a rebased version.
> >
> > I have reviewed the thread so far.  I think there is general agreement
> > that something like this would be good to have.
> >
> > I have not found any explanation, however, why the "if not exists"
> > behavior is desirable, let alone as the default.  I can only think of
> > two workflows here:  Either you have scripts for previous PG versions
> > that create the slot externally, in which can you omit --create, or you
> > use the new functionality to have pg_basebackup create the slot.  I
> > don't see any use for pg_basebackup to opportunistically use a slot if
> > it happens to exist.  Even if there is one, it should not be the
> > default.  So please change that.
> 
> +1.  I'd rather just get an error and re-run without the --create switch.
> That way you are forced to think about what you are doing.

OK.
 
> Is there a race condition here?  The slot is created after the checkpoint
> is completed.  But you have to start streaming from the LSN where the
> checkpoint started, so shouldn't the slot be created before the checkpoint
> is started?

So my patch only moves the slot creation slightly further forward,
AFAICT.

AIUI, wal streaming always begins at last checkpoint and from my tests
the restart_lsn of the created replication slot is also before that
checkpoint's lsn. However, I hope somebody more familiar with the
WAL/replication slot code could comment on that.  What I dropped in the
refactoring is the RESERVE_WAL that used to be there when the temporary
slot gets created, I have readded that now.

I also added a TAP test case that tries to check that the restart_lsn is
lower than the checkpoint_lsn, which appears to be the case.

If there is still a race condition here, do you have a suggestion in how
to try to trigger it?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From fca622f6f7c6ef9e566c5a1b72044b824e7e02cc Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 10 Sep 2017 18:07:33 +0200
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it unless
it exists already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 56 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 15 +---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 34 +++--
 8 files changed, 107 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 2454d35af3..57086295d3 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot
+option.  It causes the specified slot name to be created unless it
+exists already.
+   
+  
+ 
+
+ 
   -T olddir=newdir
   --tablespace-mapping=olddir=newdir
   
diff --git a/src/bin/pg_basebackup/pg_basebac

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-09-07 Thread Michael Banck
Hi,

Am Mittwoch, den 06.09.2017, 12:22 -0400 schrieb Peter Eisentraut:
> On 8/18/17 05:28, Michael Banck wrote:
> > > > Rebased, squashed and slighly edited version attached. I've added this
> > > > to the 2017-07 commitfest now as well:
> > > > 
> > > > https://commitfest.postgresql.org/14/1112/
> > > 
> > > Can you rebase this past some conflicting changes?
> > 
> > Thanks for letting me know, PFA a rebased version.
> 
> I have reviewed the thread so far.  I think there is general agreement
> that something like this would be good to have.
> 
> I have not found any explanation, however, why the "if not exists"
> behavior is desirable, let alone as the default.  I can only think of
> two workflows here:  Either you have scripts for previous PG versions
> that create the slot externally, in which can you omit --create, or you
> use the new functionality to have pg_basebackup create the slot.  I
> don't see any use for pg_basebackup to opportunistically use a slot if
> it happens to exist.  Even if there is one, it should not be the
> default.  So please change that.

Ok, I tried to research why that was the case and couldn't find any
trace of a discussion either.

So we should just error out in CreateReplicationSlot() in case a slot
exists, right? I think having yet another option like --create-if-not-
exists does not sound needed from what you wrote above.

> A minor point, I suggest to print the message about the replication slot
> being created *after* the slot has been created.  This aligns with how
> logical replication subscriptions work.  

Ok.

> I don't see the need for printing a message about temporary slots. 
> Since they are temporary, they will go away automatically, so there is
> nothing the user needs to know there.

Ok. I thought I'd remembered some request around having this reported
always (maybe from Magnus), but I couldn't find anything in the prior
discussions either.

If we don't print the message for temporary slots, then the
CreateReplicationSlot() refactoring and the addition of the
temp_replication_slot argument would be no longer needed, or is this
something useful on its own?


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-09-07 Thread Michael Banck
Hi,

not directly related to the topic, but:

On Tue, Mar 21, 2017 at 03:34:00PM -0400, Robert Haas wrote:
> For example, somebody creates a replica using the new super-easy
> method, and then blows it away without dropping the slot from the
> master, 

Just a thought, but maybe there should be some pg_dropstandby command or
similar, which cleans everything (what exactly?) up? That might go some
way to ensure this does not happen.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

2017-08-24 Thread Michael Banck
Hi,

On Wed, May 24, 2017 at 07:16:06AM +, Tsunakawa, Takayuki wrote:
> I confirmed that the attached patch successfully provides:

I was going to take a look at this, but the patch no longer applies
cleanly for me:

Hunk #1 succeeded at 1474 (offset 49 lines).
Hunk #2 succeeded at 1762 (offset 49 lines).
Hunk #3 succeeded at 1773 (offset 49 lines).
patching file doc/src/sgml/protocol.sgml
patching file src/backend/access/transam/xlog.c
Hunk #1 succeeded at 7909 (offset 5 lines).
patching file src/backend/tcop/postgres.c
Hunk #1 succeeded at 3737 (offset 15 lines).
patching file src/backend/utils/misc/check_guc
patching file src/backend/utils/misc/guc.c
Hunk #1 succeeded at 147 with fuzz 1.
Hunk #5 succeeded at 10062 (offset 11 lines).
patching file src/interfaces/libpq/fe-connect.c
Hunk #1 succeeded at 1178 (offset 112 lines).
Hunk #2 succeeded at 2973 (offset 124 lines).
Hunk #3 succeeded at 3003 (offset 124 lines).
Hunk #4 succeeded at 3067 (offset 124 lines).
Hunk #5 FAILED at 3022.
Hunk #6 succeeded at 3375 (offset 144 lines).
1 out of 6 hunks FAILED -- saving rejects to file
src/interfaces/libpq/fe-connect.c.rej
patching file src/interfaces/libpq/fe-exec.c
patching file src/interfaces/libpq/libpq-int.h
Hunk #1 succeeded at 361 (offset 1 line).
Hunk #2 succeeded at 421 (offset -1 lines).

Can you rebase it, please?


Best

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-08-18 Thread Michael Banck
Hi,

On Tue, Aug 15, 2017 at 02:14:58PM -0700, Jeff Janes wrote:
> On Sun, Apr 9, 2017 at 4:22 AM, Michael Banck 
> wrote:
> > Rebased, squashed and slighly edited version attached. I've added this
> > to the 2017-07 commitfest now as well:
> >
> > https://commitfest.postgresql.org/14/1112/
> 
> Can you rebase this past some conflicting changes?

Thanks for letting me know, PFA a rebased version.

I also adressed the following message which appeared when starting the
TAP tests:

't/010_pg_basebackup.pl ... "my" variable $lsn masks earlier declaration
in same scope at t/010_pg_basebackup.pl line 287.'


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From 62973bba3338abfbf4d9e2f0f05a338eb283b4b8 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 18 Aug 2017 11:28:26 +0200
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it in case
it does not exist already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 58 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 18 ++---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 25 ++--
 8 files changed, 103 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 2454d35af3..5397a185d2 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot 
+option.  It causes the specified slot name to be created if it does not
+exist already.  
+   
+  
+ 
+
+ 
   -T olddir=newdir
   --tablespace-mapping=olddir=newdir
   
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index dfb9b5ddcb..2bc7e868e0 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -92,6 +92,8 @@ static pg_time_t last_progress_report = 0;
 static int32 maxrate = 0;		/* no limit by default */
 static char *replication_slot = NULL;
 static bool temp_replication_slot = true;
+static bool create_slot = false;
+static bool no_slot = false;
 
 static bool success = false;
 static bool made_new_pgdata = false;
@@ -337,6 +339,7 @@ usage(void)
 			 " write recovery.conf for replication\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
+	printf(_("  -C, --create-slot  create replication slot if not present already\n"));
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 			 " relocate tablespace in OLDDIR to NEWDIR\n"));
 	printf(_("  -X, --wal-method=none|fetch|stream\n"
@@ -492,8 +495,6 @@ LogStreamerMain(logstreamer_param *param)
 	stream.partial_suffix = NULL;
 	stream.replication_slot = replication_slot;
 	stream.temp_slot = param->temp_slot;
-	if (stream.temp_slot && !stream.replication_slot)
-		stream.replication_slot = psprintf("pg_basebackup_%d", (int) PQbackendPID(param->bgconn));
 
 	if (format == 'p')
 		stream.walmethod = CreateWalDirectoryMethod(param->xlog, 0, do_sync);
@@ -586,6 +587,29 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Create replication slot if one is needed.
+	 */
+	if (!no_slot && (temp_replication_slot || create

Re: [HACKERS] Change in "policy" on dump ordering?

2017-08-03 Thread Michael Banck
Am Donnerstag, den 27.07.2017, 15:52 -0400 schrieb Tom Lane:
> So I'm thinking we should consider the multi-pass patch I posted as
> a short-term, backpatchable workaround, which we could hope to
> replace with dependency logic later.

+1, it would be really nice if this could be fixed in the back-branches 
before v11.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] New partitioning - some feedback

2017-07-07 Thread Michael Banck
On Fri, Jul 07, 2017 at 07:40:55PM +1200, Mark Kirkwood wrote:
> On 07/07/17 13:29, Amit Langote wrote:
> >Someone complained about this awhile back [1].  And then it came up again
> >[2], where Noah appeared to take a stance that partitions should be
> >visible in views / output of commands that list "tables".
> >
> >Although I too tend to prefer not filling up the \d output space by
> >listing partitions (pg_class.relispartition = true relations), there
> >wasn't perhaps enough push for creating a patch for that.  If some
> >committer is willing to consider such a patch, I can make one.
> 
> Yeah, me too (clearly). However if the consensus is that all these partition
> tables *must* be shown in \d output, then I'd be happy if they were
> identified as such rather than just 'table' (e.g 'partition table').

+1.

Or maybe just 'partition' is enough if 'partition table' would widen the
column output unnecessarily.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] [PostgreSQL 10] default of hot_standby should be "on"?

2017-05-03 Thread Michael Banck
Hi,

On Tue, May 02, 2017 at 11:13:58AM +0200, Magnus Hagander wrote:
> Looks good to me as well. Applied, with only a minor further docs addition
> saying that this is the default also on the high availability page.

I understand this is late, but a colleague alerted me to the following
behaviour change: If you recover a server with default settings, it is
our understanding that pg_isready will now return true immediately after
the consistent state is reached and possibly well before recovery had
actually ended (depending on the amount of outstanding wal). As hot
standby works with log shipping, this is independent of the
recovery.conf settings, i.e. even if standby_mode and primary_conninfo
are not set. So if one was monitoring recovery like that before and
expects pg_isready to only return true once the recovery is fully
complete, this will now have to be adjusted. Also, if the recovered
server is to be used for transactions, there will now be a window where
the server accepts connections, but is in read-only mode.

Before, one had the make the concious choice to set hot_standby to get
the behaviour, now it might be surprising to users, or maybe I'm
overthinking this?

If that is indeed the case, maybe it should be mentioned more
prominently in the documentation and/or get highlighted in the release
notes?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] A note about debugging TAP failures

2017-04-23 Thread Michael Banck
On Sat, Apr 22, 2017 at 02:05:13PM -0700, Andres Freund wrote:
> On 2017-04-22 16:22:59 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2017-04-22 13:51:30 -0400, Tom Lane wrote:
> > >> I think we need to fix TestLib and/or PostgresNode so that there's a way
> > >> to make TAP tests not auto-clean their data directories at end of run,
> > >> without having to resort to editing the script like this.
> > 
> > > I think leaving the directory around in case of failures would be a
> > > pretty bare minimum.  It's sometimes also useful to keep the remains if
> > > everything was apparently successfull, but it's far less important imo.
> > 
> > In the particular case I was interested in here, the test script thought
> > everything was successful :-(.  I'm working on fixing that little problem,
> > but I do not believe that the TAP scripts are so bulletproof that there
> > will never be a need to override their judgment.
> 
> Agreed.  If paths are reproducible and cleaned up on next run it's also
> much less of an issue to just leave them around till the next run.
> Which we imo also should do for non-failing tmp_check directories.

In general yes, but I think it should be checked what the overall 
storage requirements will be if one set of all data directories is kept
around.

I was surprised the src/bin/pg_basebackup/t TAP tests took up several
hundred megabytes (IIRC) when I ran them, so if other tests are similar,
it might make a few animals run out of diskspace as soon as this is
deployed without a heads-up to the operators.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-04-09 Thread Michael Banck
Hi,

Am Freitag, den 24.03.2017, 19:32 +0100 schrieb Michael Banck:
> On Thu, Mar 23, 2017 at 12:41:54PM +0100, Magnus Hagander wrote:
> > On Tue, Mar 21, 2017 at 8:34 PM, Robert Haas  wrote:
> > > So I tend to think that there should always be some explicit user
> > > action to cause the creation of a slot, like --create-slot-if-needed
> > > or --create-slot=name.  That still won't prevent careless use of that
> > > option but it's less dangerous than assuming that a user who refers to
> > > a nonexistent slot intended to create it when, perhaps, they just
> > > typo'd it.
> > 
> > Well, the explicit user action would be "--slot". But sure, I can
> > definitely live with a --create-if-not-exists. 
> 
> Can we just make that option create slot and don't worry if one exists
> already? CreateReplicationSlot() can be told to not mind about already
> existing slots, and I don't see a huge point in erroring out if the slot
> exists already, unless somebody can show that this leads to data loss
> somehow.
> 
> > The important thing, to me, is that you can run it as a single
> > command, which makes it a lot easier to work with. (And not like we
> > currently have for pg_receivewal which requires a separate command to
> > create the slot)
> 
> Oh, that is how it works with pg_receivewal, I have to admit I've never
> used it so was a bit confused about this when I read its code.
> 
> So in that case I think we don't necessarily need to have the same user
> interface at all. I first thought about just adding "-C, --create" (as
> in "--create --slot=foo"), but this on second thought this looked a bit
> shortsighted - who knows what flashy thing pg_basebackup might create in
> 5 years... So I settled on --create-slot, which is only slightly more to
> type (albeit repetive, "--create-slot --slot=foo"), but adding a short
> option "-C" would be fine I thinkg "-C -S foo".
> 
> So attached is a patch with adds that option. If people really think it
> should be --create-slot-if-not-exists instead I can update the patch, of
> course.
> 
> I again added a second patch with some further refactoring which makes
> it print a message on temporary slot creation in verbose mode.

Rebased, squashed and slighly edited version attached. I've added this
to the 2017-07 commitfest now as well:

https://commitfest.postgresql.org/14/1112/


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From 47fc0d543a43441f239f109d08bfc7c082f4c091 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 24 Mar 2017 18:27:47 +0100
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it in case
it does not exist already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 58 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 18 ++---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 23 +--
 8 files changed, 102 insertions(+), 33 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index e1cec9d..65ca8dc 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot 
+option.  It causes the specified slot name to be created if it does not
+exist already.  
+   
+  
+ 
+
+   

Re: [HACKERS] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-04-03 Thread Michael Banck
Am Samstag, den 01.04.2017, 17:29 +0200 schrieb Magnus Hagander:

> I've applied a backpatch to 9.4. Prior to that pretty much the entire
> patch is a conflict, so it would need a full rewrite.

Thanks!


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




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


Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-03-31 Thread Michael Banck
On Fri, Mar 31, 2017 at 02:11:44PM +0900, Kyotaro HORIGUCHI wrote:
> At Fri, 31 Mar 2017 13:37:38 +0900, Michael Paquier 
>  wrote in 
> 
> > In my first reviews of the patch, I completely forgot the fact that
> > BASE_BACKUP does send the start LSN of the backup in the first result
> > set, so the patch proposed is actually rather useless because the data
> > you are looking for is already at hand. If more data would be
> > interesting to have, like the start timestamp number, we could just
> > extend the first result set a bit as Fujii-san is coming at. Let's
> > drop this patch and move on.
> 
> +1 for dropping this.

I've marked it as "Rejected" now, as that is clearly the consensus.
 
> But I think we should edit the documentation a bit.
> 
> I don't fully understand those who want to handle it by a script,
> but the documentation seems to be suggesting that something like
> is possible. So it might be better add a description like that or
> just remove the example.

The documentation says "For the purpose of testing replication
commands", one could use psql and "IDENTIFY_SYSTEM", it doesn't exactly
suggest to run BASE_BACKUP this way.

Which, by the way, appears to be working totally fine from psql, modulo
the problem that you won't get to the starting transaction location.

> "psql doesn't handle this protocol properly. The instances of the
>  usage of these protocols are found in the source code of
>  walreceiver and pg_basebackup."

Something along those lines makes sense I think, yeah.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] REINDEX CONCURRENTLY 2.0

2017-03-29 Thread Michael Banck
et constraint violations from the

"we only get"

> + /*
> +  * Phase 5 of REINDEX CONCURRENTLY
> +  *
> +  * The indexes hold now a fresh relfilenode of their respective 
> concurrent

I'd write "now hold" instead of "hold now".

> +  * entries indexes. It is time to mark the now-useless concurrent 
> entries
> +  * as not ready so as they can be safely discarded from write operations
> +  * that may occur on them.

So the "concurrent entries" is the original index, as that one should be
now-useless?  If so, that's a bit confusing terminology to me and it was
called "old index" in the previous phases.

> + /*
> +  * Phase 6 of REINDEX CONCURRENTLY
> +  *
> +  * Drop the concurrent indexes, with actually the same code path as

Again, I'd have written "Drop the old indexes". Also, "with actually the
same" sounds a bit awkward, maybe "actually using the same" would be
better.

> + /*
> +  * Last thing to do is to release the session-level lock on the parent 
> table
> +  * and the indexes of table.

"and on the indexes of the table"?  Or what exactly is meant with the
last bit?

> diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
> index 20b5273405..c76dacc44a 100644
> --- a/src/backend/tcop/utility.c
> +++ b/src/backend/tcop/utility.c
> @@ -773,16 +773,20 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> - ReindexIndex(stmt->relation, 
> stmt->options);
> + ReindexIndex(stmt->relation, 
> stmt->options, stmt->concurrent);
> - ReindexTable(stmt->relation, 
> stmt->options);
> + ReindexTable(stmt->relation, 
> stmt->options, stmt->concurrent);
> - 
> ReindexMultipleTables(stmt->name, stmt->kind, stmt->options);
> + 
> ReindexMultipleTables(stmt->name, stmt->kind, stmt->options, 
> stmt->concurrent);

Those lines are now in excess of 80 chars.


Cheers,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-03-29 Thread Michael Banck
Hi,

Am Montag, den 27.02.2017, 16:20 +0100 schrieb Magnus Hagander:
> On Sun, Feb 26, 2017 at 9:59 PM, Tom Lane  wrote:
> Is there an argument for back-patching this?
> 
> 
> Seems you were typing that at the same time as we did.
> 
> 
> I'm considering it, but not swayed in either direction. Should I take
> your comment as a vote that we should back-patch it? 

I've checked back into this thread, and there seems to be a +1 from Tom
and a +(0.5-1) from Simon for backpatching, and no obvious -1s. Did you
decide against it in the end, or is this still an open item?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




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


Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-03-29 Thread Michael Banck
Hi,

Am Mittwoch, den 29.03.2017, 15:22 +0900 schrieb Michael Paquier:
> On Wed, Mar 29, 2017 at 3:56 AM, Fujii Masao  wrote:
> > If your need other information except START WAL LOCATION at the beginning of
> > base backup and they are very useful for many third-party softwares,
> > you can add them into that first result set. If you do this, you can
> > retrieve them
> > at the beginning even when WAL files are included in the backup.
> 
> You mean in the result tuple of pg_start_backup(), right? Why not.

The replication protocol chapter says: "When the backup is started, the
server will first send two ordinary result sets, followed by one or more
CopyResponse results. The first ordinary result set contains the
starting position of the backup, in a single row with two columns."

However, I don't think it is very obvious to users (or at least it is
not to me) how to get at this from psql, if you want to script it.  If I
run something like 'psql "dbname=postgres replication=database" -c
"BASE_BACKUP" "> foo', I seem to only get the tar file(s), unless I am
missing something. 

The reason one might not want to run pg_basebackup directly is that this
way one can pipe the output to an external program, e.g. to better
compress it; this is not possible with pg_basebackup if you additional
tablespaces.

So I think that has some merit on its own.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer



-- 
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] Logical replication existing data copy

2017-03-24 Thread Michael Banck
Am Freitag, den 24.03.2017, 14:57 -0400 schrieb Peter Eisentraut:
> On 3/24/17 05:22, Michael Banck wrote:
> > However, replication also seems to not work, I'm using the following
> > script right now:
> 
> The problem is that your publication does not include any tables.

Oops, of course. That part must've got lost in a copy-paste or when I
tried to dig further why the CREATE SUBSCRIPTION segfaulted, sorry for
the noise.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-03-24 Thread Michael Banck
Hi,

On Thu, Mar 23, 2017 at 12:41:54PM +0100, Magnus Hagander wrote:
> On Tue, Mar 21, 2017 at 8:34 PM, Robert Haas  wrote:
> > So I tend to think that there should always be some explicit user
> > action to cause the creation of a slot, like --create-slot-if-needed
> > or --create-slot=name.  That still won't prevent careless use of that
> > option but it's less dangerous than assuming that a user who refers to
> > a nonexistent slot intended to create it when, perhaps, they just
> > typo'd it.
> 
> Well, the explicit user action would be "--slot". But sure, I can
> definitely live with a --create-if-not-exists. 

Can we just make that option create slot and don't worry if one exists
already? CreateReplicationSlot() can be told to not mind about already
existing slots, and I don't see a huge point in erroring out if the slot
exists already, unless somebody can show that this leads to data loss
somehow.

> The important thing, to me, is that you can run it as a single
> command, which makes it a lot easier to work with. (And not like we
> currently have for pg_receivewal which requires a separate command to
> create the slot)

Oh, that is how it works with pg_receivewal, I have to admit I've never
used it so was a bit confused about this when I read its code.

So in that case I think we don't necessarily need to have the same user
interface at all. I first thought about just adding "-C, --create" (as
in "--create --slot=foo"), but this on second thought this looked a bit
shortsighted - who knows what flashy thing pg_basebackup might create in
5 years... So I settled on --create-slot, which is only slightly more to
type (albeit repetive, "--create-slot --slot=foo"), but adding a short
option "-C" would be fine I thinkg "-C -S foo".

So attached is a patch with adds that option. If people really think it
should be --create-slot-if-not-exists instead I can update the patch, of
course.

I again added a second patch with some further refactoring which makes
it print a message on temporary slot creation in verbose mode.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From 4e37e110ac402e67874f729832b330a837284d4b Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 24 Mar 2017 18:27:47 +0100
Subject: [PATCH 1/2] Add option to create a replication slot in pg_basebackup
 if not yet present.

When reqeusting a particular replication slot, the new option --create tries to
create it before starting to replicate from it in case it does not exist
already.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 
 src/bin/pg_basebackup/pg_basebackup.c| 44 +++-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 23 +--
 3 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index e1cec9d..789f649 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot 
+	option.  It causes the specified slot name to be created if it does not
+exist already.  
+   
+  
+ 
+
+ 
   -T olddir=newdir
   --tablespace-mapping=olddir=newdir
   
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 0a4944d..2af2e22 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -92,6 +92,7 @@ static pg_time_t last_progress_report = 0;
 static int32 maxrate = 0;		/* no limit by default */
 static char *replication_slot = NULL;
 static bool temp_replication_slot = true;
+static bool create_slot = false;
 
 static bool success = false;
 static bool made_new_pgdata = false;
@@ -337,6 +338,7 @@ usage(void)
 			 " write recovery.conf for replication\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
+	printf(_("  -C, --create-slot  create replication slot if not present already\n"));
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 	  " relocate tablespace in OLDDIR to NEWDIR\n"));
 	printf(_("  -X, --wal-method=none|fetch|stream\n"
@@ -581,6 +583,19 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->

Re: [HACKERS] Logical replication existing data copy

2017-03-24 Thread Michael Banck
Hi,

On Fri, Mar 24, 2017 at 12:32:28AM +0100, Petr Jelinek wrote:
> Yes, I also forgot to check if the table actually exists on subscriber
> when fetching them in CREATE SUBSCRIPTION (we have check during
> replication but not there).
> 
> Attached patches should fix both issues.

I no longer get a segfault, thanks.

However, replication also seems to not work, I'm using the following
script right now:

--8<--
#!/bin/sh

set -x

#rm -rf data_* pg*.log

initdb --pgdata=data_pg1 1> /dev/null 2>&1
sed -i -e 's/^#wal_level.=.replica/wal_level = logical/'
data_pg1/postgresql.conf
pg_ctl --pgdata=data_pg1 -l pg1.log start 1> /dev/null
psql -c "CREATE TABLE t1(id int);"
pg_basebackup --pgdata=data_pg2
sed -i -e 's/^#port.=.5432/port = 5433/' data_pg2/postgresql.conf
psql -c "INSERT INTO t1 VALUES(1);"
pg_ctl --pgdata=data_pg2 -l pg2.log start 1> /dev/null
psql -c "CREATE PUBLICATION pub1;"
psql --port=5433 -c "CREATE SUBSCRIPTION sub1 CONNECTION
'dbname=postgres' PUBLICATION pub1 WITH (COPY DATA);"
sleep 6
psql -c 'SELECT application_name, state FROM pg_stat_replication;'
psql --port=5433 -c "SELECT COUNT(*) FROM t1;"
--8<--

(I had to add the sleep 6 - 5 wasn't always enough - in order to get the
subcription from 'catchup' to 'streaming' which was a bit surprising to
me)

This is the output:

--8<--
+ initdb --pgdata=data_pg1
+ sed -i -e s/^#wal_level.=.replica/wal_level = logical/
data_pg1/postgresql.conf
+ pg_ctl --pgdata=data_pg1 -l pg1.log start
+ psql -c CREATE TABLE t1(id int);
CREATE TABLE
+ pg_basebackup --pgdata=data_pg2
+ sed -i -e s/^#port.=.5432/port = 5433/ data_pg2/postgresql.conf
+ psql -c INSERT INTO t1 VALUES(1);
INSERT 0 1
+ pg_ctl --pgdata=data_pg2 -l pg2.log start
+ psql -c CREATE PUBLICATION pub1;
CREATE PUBLICATION
+ psql --port=5433 -c CREATE SUBSCRIPTION sub1 CONNECTION
'dbname=postgres' PUBLICATION pub1 WITH (COPY DATA);
NOTICE:  created replication slot "sub1" on publisher
NOTICE:  synchronized table states
CREATE SUBSCRIPTION
+ sleep 6
+ psql -c SELECT application_name, state FROM pg_stat_replication;
 application_name |   state   
--+---
 sub1 | streaming
(1 row)

+ psql --port=5433 -c SELECT COUNT(*) FROM t1;
 count 
---
 0
(1 row)
--8<--

I would have assumed the inserted value to be visible on the standby.
If I insert further values, don't show up, either.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Logical replication existing data copy

2017-03-23 Thread Michael Banck
it/postgresql/build/../src/backend/postmaster/postmaster.c:4317
#12 BackendStartup (port=0xfc2970)
at 
/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/backend/postmaster/postmaster.c:3989
#13 ServerLoop ()
at 
/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/backend/postmaster/postmaster.c:1729
#14 0x0068819c in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0xf9c420)
at 
/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/backend/postmaster/postmaster.c:1337
#15 0x00477f27 in main (argc=3, argv=0xf9c420)
at 
/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/backend/main/main.c:228

The TAP tests in src/test/subscriber pass for me, going back to
707576b571f05ec5b89adb65964d55f3bd1b also makes the above chain of
commands run fine.

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-03-21 Thread Michael Banck
Hi,

Am Mittwoch, den 22.03.2017, 00:40 +0100 schrieb Michael Banck:
> I guess if we decide (physical) slots should not be created implicitly,
> then using the same UI as pg_receivewal makes sense for the sake of
> consistency, i.e. "--slot=name --create-slot [--if-not-exists]". That is
> rather verbose though and I don't think the --if-not-exists is great UX,
> but maybe there is some use-case for erroring out if a slot already
> exists that I haven't thought of.

Thinking about this some more, there's the case of somebody accidentally
using an already existing slot that was meant for another standby which
happens to be down just at that moment.

>From some quick testing, that makes replication fail once the other
standby is started up again, but soon after the new standby is taken
down, replication picks up without apparent problems for the other
standby.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-03-21 Thread Michael Banck
Am Dienstag, den 21.03.2017, 15:34 -0400 schrieb Robert Haas:
> On Sun, Mar 19, 2017 at 12:01 PM, Magnus Hagander  wrote:
> > I think maybe we should output a message when the slot is created, at least
> > in verbose mode, to make sure people realize that happened. Does that seem
> > reasonable?
> 
> Slots are great until you leave one lying around by accident.  I'm
> afraid that no matter what we do, we're going to start getting
> complaints from people who mess that up.  For example, somebody
> creates a replica using the new super-easy method, and then blows it
> away without dropping the slot from the master, and then days or weeks
> later pg_wal fills up and takes the server down.  The user says, oh,
> these old write-ahead log files should have gotten removed, and
> removes them all.  Oops.

Hrm. 

Maybe it would be useful to log a warning like "slot XY has not been
active for X minutes", based on some threshold, though possibly the
people not keeping an eye on their replication slots won't spot that in
the logfiles, either.

> So I tend to think that there should always be some explicit user
> action to cause the creation of a slot, like --create-slot-if-needed
> or --create-slot=name.  That still won't prevent careless use of that
> option but it's less dangerous than assuming that a user who refers to
> a nonexistent slot intended to create it when, perhaps, they just
> typo'd it.

I guess if we decide (physical) slots should not be created implicitly,
then using the same UI as pg_receivewal makes sense for the sake of
consistency, i.e. "--slot=name --create-slot [--if-not-exists]". That is
rather verbose though and I don't think the --if-not-exists is great UX,
but maybe there is some use-case for erroring out if a slot already
exists that I haven't thought of.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-03-21 Thread Michael Banck
Am Dienstag, den 21.03.2017, 12:52 +0100 schrieb Michael Banck:
> New patches attached.

On second though, there was a superfluous whitespace change in
t/010_pg_basebackup.pl, and I've moved the check-for-hex regex fix to
the second patch as it's cosmetic and not related to changing the --slot
creation behaviour.


Michael
 
-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

From 2ab1be27a341ecdcd2d6a3dd5ce535aba5e16b03 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH 01/29] Create replication slot in pg_basebackup if requested
 and not yet present.

If a replication slot is explicitly requested, try to create it before
starting to replicate from it.
---
 src/bin/pg_basebackup/pg_basebackup.c| 15 +++
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 15 ++-
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 0a4944d..69ca4be 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -581,6 +581,21 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Try to create a permanent replication slot if one is specified. Do
+	 * not error out if the slot already exists, other errors are already
+	 * reported by CreateReplicationSlot().
+	 */
+	if (!temp_replication_slot && replication_slot)
+	{
+		if (verbose)
+			fprintf(stderr, _("%s: creating replication slot \"%s\"\n"),
+	progname, replication_slot);
+
+		if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true))
+			return 1;
+	}
+
 	if (format == 'p')
 	{
 		/*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 14bd813..70c3284 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');
@@ -246,14 +246,19 @@ $node->command_ok(
 	'pg_basebackup -X stream runs with --no-slot');
 
 $node->command_fails(
-	[ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+	[ 'pg_basebackup', '-D', "$tempdir/fail", '-X', 'none', '-S', 'slot0' ],
 	'pg_basebackup with replication slot fails without -X stream');
-$node->command_fails(
+$node->command_ok(
 	[   'pg_basebackup', '-D',
 		"$tempdir/backupxs_sl_fail", '-X',
 		'stream','-S',
-		'slot1' ],
-	'pg_basebackup fails with nonexistent replication slot');
+		'slot0' ],
+	'pg_basebackup -S creates previously nonexistent replication slot');
+
+my $lsn = $node->safe_psql('postgres',
+	q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot0'}
+);
+like($lsn, qr!^0/[0-9A-F]{7,8}$!, 'slot is present');
 
 $node->safe_psql('postgres',
 	q{SELECT * FROM pg_create_physical_replication_slot('slot1')});
-- 
2.1.4

From 5f0f31f61bf668de937868840a97c0a56dc2cd5d Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Tue, 21 Mar 2017 12:50:22 +0100
Subject: [PATCH 02/29] Refactor replication slot creation in pg_basebackup et
 al.

Add new argument `is_temporary' to CreateReplicationSlot() in streamutil.c
which specifies whether a physical replication slot is temporary or not. Update
all callers.

Move the creation of the temporary replication slot for pg_basebackup from
receivelog.c to pg_basebackup.c. At the same time, also create the temporary
slot via CreateReplicationSlot() instead of creating the temporary slot with an
explicit SQL command.
---
 src/bin/pg_basebackup/pg_basebackup.c| 28 +---
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 --
 src/bin/pg_basebackup/streamutil.c   | 18 +-
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl |  2 +-
 7 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_baseba

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-03-21 Thread Michael Banck
Hi,

Am Dienstag, den 21.03.2017, 11:03 +0900 schrieb Michael Paquier:
> On Tue, Mar 21, 2017 at 1:32 AM, Michael Banck
>  wrote:
>  /*
> + * Try to create a permanent replication slot if one is specified. Do
> + * not error out if the slot already exists, other errors are already
> + * reported by CreateReplicationSlot().
> + */
> +if (!stream->temp_slot && stream->replication_slot)
> +{
> +if (!CreateReplicationSlot(conn, stream->replication_slot,
> NULL, true, true))
> +return false;
> +}
> This could be part of an else taken from the previous if where
> temp_slot is checked.

Not sure how this would work, as ISTM the if clause above only sets the
name for param->temp_slot (if supported), but doesn't distinguish which
kind of slot (if any) will actually be created. 

I've done it for the second (refactoring) patch though, but I had to
make `no_slot' a global variable to not have the logic be too
complicated.

> There should be some TAP tests included. And +1 for sharing the same
> behavior as pg_receivewal.

Well, I've adjusted the TAP tests in so far as it's now checking that
the physical slot got created, previously it checked for the opposite:

|-$node->command_fails(
|+$node->command_ok(
|[   'pg_basebackup', '-D',
|"$tempdir/backupxs_sl_fail", '-X',
|'stream','-S',
|-   'slot1' ],
|-   'pg_basebackup fails with nonexistent replication slot');
|+   'slot0' ],
|+   'pg_basebackup runs with nonexistent replication slot');
|+
|+my $lsn = $node->safe_psql('postgres',
|+   q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name
|= 'slot0'}
|+);
|+like($lsn, qr!^0/[0-9A-F]{7,8}$!, 'slot is present');

I have now made the message a bit clearer by saying "pg_basebackup -S
creates previously nonexistent replication slot".

Probably there could be additional TAP tests, but off the top of my head
I can't think of any?

New patches attached.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From 93337fe0ef320fe8ef68a9b64c4df85a63c4346c Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH 1/2] Create replication slot in pg_basebackup if requested and
 not yet present.

If a replication slot is explicitly requested, try to create it before
starting to replicate from it.
---
 src/bin/pg_basebackup/pg_basebackup.c| 15 +++
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 19 ---
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 0a4944d..69ca4be 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -581,6 +581,21 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Try to create a permanent replication slot if one is specified. Do
+	 * not error out if the slot already exists, other errors are already
+	 * reported by CreateReplicationSlot().
+	 */
+	if (!temp_replication_slot && replication_slot)
+	{
+		if (verbose)
+			fprintf(stderr, _("%s: creating replication slot \"%s\"\n"),
+	progname, replication_slot);
+
+		if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true))
+			return 1;
+	}
+
 	if (format == 'p')
 	{
 		/*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 14bd813..f50005f 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');
@@ -246,17 +246,22 @@ $node->command_ok(
 	'pg_basebackup -X stream runs with --no-slot');
 
 $node->command_fails(
-	[ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+	[ 'pg_basebackup', '-D', "$tempdir/fail", '-X', 'none', '-S', 'slot0' ],
 	'pg_basebackup with replication slot fails without -X stream');
-$node->command_fails(
+$node->c

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-03-20 Thread Michael Banck
Hi,

On Mon, Mar 20, 2017 at 02:42:32PM +0300, Arthur Zakirov wrote:
> Also maybe it would be good if pg_basebackup had a way to drop created slot.
> Although "drop slot" is not related with concept of automatically created
> slots, it will good if user will have a way to drop slots.

If you want to drop the slot after basebackup finishes you'd just use a
temporary slot (i.e. the default), or am I understanding your use-case
incorrectly?

I assumed the primary use-case for creating a non-temporary slot is to
keep it around while the standby is active.


Michael


-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

2017-03-19 Thread Michael Banck
Hi,

On Sun, Mar 19, 2017 at 05:01:23PM +0100, Magnus Hagander wrote:
> On Sun, Mar 19, 2017 at 11:21 AM, Michael Banck 
> wrote:
> > So I propose the attached tiny patch to just create the slot (if it does
> > not exist already) in pg_basebackup, somewhat similar to what
> > pg_receivewal does, albeit unconditionally, if the user explicitly
> > requested a slot:
> >
> > $ pg_basebackup --pgdata=data2 --write-recovery-conf --slot=pg2
> > $ echo $?
> > 0
> > $ psql -c "DROP_REPLICATION_SLOT pg2" "host=127.0.0.1 port=5432
> > replication=database user=mba dbname=postgres"
> > SELECT
> > $
> >
> > This would get us somewhat closer to near zero-config replication, in my
> > opinion. Pardon me if that was discussed already and shot down
> >
> > Comments?
> 
> I think this is a good idea, since it makes replication slots easier to
> use. The change to use temporary slots was good for backups, but didn't
> help people setting up replicas.
> 
> I've been annoyed for a while we didn't have a "create slot" mode in
> pg_basebackup, but doing it integrated like this is definitely a step even
> better than that.

Great!
 
> I think maybe we should output a message when the slot is created, at least
> in verbose mode, to make sure people realize that happened. Does that seem
> reasonable?

So the patch I sent earlier creates the slot in ReceiveXlogStream() in
receivewal.c, as that's where the temp slot gets created as well, but
now I wonder whether that is maybe not the best place, as pg_receivewal
also calls that function. The other problem with receivewal.c is that
`verbose' isn't around in it so I don't how I'd print out a message
there.

So probably it is better to create the slot in pg_basebackup.c's
StartLogStreamer(), see the attached first patch, that one also adds
a verbose message.

I also now realized I didn't ran the TAP tests and they need updating,
this has been done in the first attached patch as well. Independently of
this patch series I think those two hunks from the third patch are
applicable to current master as well:

 $node->command_fails(
-   [ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+   [ 'pg_basebackup', '-D', "$tempdir/fail", '-X', 'none', '-S', 'slot0' ],
'pg_basebackup with replication slot fails without -X stream');

as '-X stream' is now the default, and (more cosmetically)

-like($lsn, qr!^0/[0-9A-Z]{7,8}$!, 'restart LSN of slot has advanced');
+like($lsn, qr!^0/[0-9A-F]{7,8}$!, 'restart LSN of slot has advanced');

as we are looking for hex values.

However, the other thing to ponder is that we don't really know whether
the user maybe setup a replication slot on the primary in preparation
already as there seems to be no way to query the list of current slots
via the replication protocal, and setting up another normal connection
just to query pg_replication_slots seems to be overkill. So maybe the
user would be confused then why the slot is created, but IDK. 

If there are other uses for querying the available replication slots,
maybe the protocol could be extended to that end for 11.

Finally, it is a bit inconsitent that we'd report the creation of the
permanent slot, but not the temporary one. 

I took a look and it seems the main reason why ReceiveXlogStream() does
not call streamutil,c's CreateReplicationSlot() seems to be that the
latter has no notion of temporary slots yet.  I'm attaching a second
patch which refactors things a bit more, adding a `is_temporary'
argument to CreateReplicationSlot() and moving the creation of the
temporary slot to pg_basebackup.c's StartLogStreamer() as well (as
pg_recvlogical and pg_receivewal do not deal with temp slots anyway).
This way, the creation of the temporary slot can be mention on --verbose
as well.

Personally, I think it'd be good to be able to have the --slot option
just work in 10, so if the first patch is still acceptable (pending
review) for 10 but not the second, that'd be entirely fine.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From 835e6fe3e30d534bc5918d1d6c399074a9a13626 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH 1/2] Create replication slot in pg_basebackup if requested and
 not yet present.

If a replication slot is explicitly requested, try to create 

[HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-03-19 Thread Michael Banck
Hi,

with the default configuration improvements so far for 10, it seems to
be very easy to setup streaming replication (at least locally):

$ initdb --pgdata=data1
$ pg_ctl --pgdata=data1 start
$ pg_basebackup --pgdata=data2 --write-recovery-conf
$ sed -i -e 's/^#port.=.5432/port = 5433/' \
> -e 's/^#hot_standby.=.off/hot_standby = on/' \
> data2/postgresql.conf
$ pg_ctl --pgdata=data2 start

(there might be a case for having hot_standby=on by default, but I think
that got discussed elsewhere and is anyway a different thread).

However, the above does not use replication slots, and if you want to do
so, you get:

$ pg_basebackup --pgdata=data2 --write-recovery-conf --slot=pg2 
2017-03-19 11:04:37.978 CET [25362] ERROR:  replication slot "pg2" does
not exist
pg_basebackup: could not send replication command "START_REPLICATION":
ERROR:  replication slot "pg2" does not exist
pg_basebackup: child process exited with error 1
pg_basebackup: removing data directory "data2"

The error message is clear enough, but I wonder whether people will
start writing streaming replication tutorials just glossing over this
because it's one more step to run CREATE_REPLICATION_SLOT manually.

So I propose the attached tiny patch to just create the slot (if it does
not exist already) in pg_basebackup, somewhat similar to what
pg_receivewal does, albeit unconditionally, if the user explicitly
requested a slot:

$ pg_basebackup --pgdata=data2 --write-recovery-conf --slot=pg2 
$ echo $?
0
$ psql -c "DROP_REPLICATION_SLOT pg2" "host=127.0.0.1 port=5432 
replication=database user=mba dbname=postgres" 
SELECT
$

This would get us somewhat closer to near zero-config replication, in my
opinion. Pardon me if that was discussed already and shot down

Comments?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From 8a52b7d8cc6f956fba47465d280581e200ec5239 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH] Create replication slot in pg_basebackup if requested and not
 yet present.

If a replication slot is explicitly requested, try to create it before
starting to replicate from it.
---
 src/bin/pg_basebackup/receivelog.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index f415135..0ddb7a6 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -527,6 +527,17 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
 	}
 
 	/*
+	 * Try to create a permanent replication slot if one is specified. Do
+	 * not error out if the slot already exists, other errors are already
+	 * reported by CreateReplicationSlot().
+	 */
+	if (!stream->temp_slot && stream->replication_slot)
+	{
+		if (!CreateReplicationSlot(conn, stream->replication_slot, NULL, true, true))
+			return false;
+	}
+
+	/*
 	 * initialize flush position to starting point, it's the caller's
 	 * responsibility that that's sane.
 	 */
-- 
2.1.4


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


Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-03-17 Thread Michael Banck
Hi,

Am Freitag, den 17.03.2017, 20:46 +0900 schrieb Michael Paquier:
> On Fri, Mar 17, 2017 at 7:18 PM, Michael Banck
>  wrote:
> > Am Freitag, den 17.03.2017, 10:50 +0900 schrieb Michael Paquier:
> >> The comment block format is incorrect. I would think as well that this
> >> comment should say it is important to have the main tablespace listed
> >> last it includes the WAL segments, and those need to contain all the
> >> latest WAL segments for a consistent backup.
> >
> > How about the attached? The comment now reads as follows:
> >
> > |Add a node for the base directory. If WAL is included, the base
> > |directory has to be last as the WAL files get appended to it. If WAL
> > |is not included, send the base directory first, so that the
> > |backup_label file is the first file to be sent.
> 
> Close enough, still not that:
> https://www.postgresql.org/docs/current/static/source-format.html

Ouch, three times a charm then I guess. I hope the attached is finally
correct, thanks for bearing with me.

> >> FWIW, I have no issue with changing the ordering of backups the way
> >> you are proposing here as long as the comment of this code path is
> >> clear.
> >
> > OK, great, let's see what the committers think then.
> 
> Still that's a minor point, so I am making that ready for committer.

Thanks!


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

From c982aa133ab4d982c1a6a267c67d7ca89811d024 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 17 Mar 2017 11:10:53 +0100
Subject: [PATCH] Reorder tablespaces for non-WAL streaming basebackups.

Currently, the main tablespace is the last to be sent, in order for the WAL
files to be appended to it. This makes the backup_label file (which gets
prepended to the main tablespace) inconveniently end up in the middle of the
basebackup stream if other tablespaces are present.

Change this so that the main tablespace is the first to be sent in case
no WAL files are requested, ensuring that backup_label is the first file
in the stream in this case.
---
 src/backend/replication/basebackup.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index e3a7ad5..520628f 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -233,10 +233,18 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		else
 			statrelpath = pgstat_stat_directory;
 
-		/* Add a node for the base directory at the end */
+		/*
+		 * Add a node for the base directory. If WAL is included, the base
+		 * directory has to be last as the WAL files get appended to it. If WAL
+		 * is not included, send the base directory first, so that the
+		 * backup_label file is the first file to be sent.
+		 */
 		ti = palloc0(sizeof(tablespaceinfo));
 		ti->size = opt->progress ? sendDir(".", 1, true, tablespaces, true) : -1;
-		tablespaces = lappend(tablespaces, ti);
+		if (opt->includewal)
+			tablespaces = lappend(tablespaces, ti);
+		else
+			tablespaces = lcons(ti, tablespaces);
 
 		/* Send tablespace header */
 		SendBackupHeader(tablespaces);
-- 
2.1.4


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


Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-03-17 Thread Michael Banck
Hi,

Am Freitag, den 17.03.2017, 10:50 +0900 schrieb Michael Paquier:
> The comment block format is incorrect. I would think as well that this
> comment should say it is important to have the main tablespace listed
> last it includes the WAL segments, and those need to contain all the
> latest WAL segments for a consistent backup.

How about the attached? The comment now reads as follows:

|Add a node for the base directory. If WAL is included, the base
|directory has to be last as the WAL files get appended to it. If WAL
|is not included, send the base directory first, so that the
|backup_label file is the first file to be sent.

> FWIW, I have no issue with changing the ordering of backups the way
> you are proposing here as long as the comment of this code path is
> clear.

OK, great, let's see what the committers think then.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

From 608ba9113e6d8f5752690c526051f61909c2e982 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 17 Mar 2017 11:10:53 +0100
Subject: [PATCH] Reorder tablespaces for non-WAL streaming basebackups.

Currently, the main tablespace is the last to be sent, in order for the WAL
files to be appended to it. This makes the backup_label file (which gets
prepended to the main tablespace) inconveniently end up in the middle of the
basebackup stream if other tablespaces are present.

Change this so that the main tablespace is the first to be sent in case
no WAL files are requested, ensuring that backup_label is the first file
in the stream in this case.
---
 src/backend/replication/basebackup.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index e3a7ad5..e03e902 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -233,10 +233,17 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		else
 			statrelpath = pgstat_stat_directory;
 
-		/* Add a node for the base directory at the end */
+		/* Add a node for the base directory. If WAL is included, the base
+		 * directory has to be last as the WAL files get appended to it. If WAL
+		 * is not included, send the base directory first, so that the
+		 * backup_label file is the first file to be sent.
+		 */
 		ti = palloc0(sizeof(tablespaceinfo));
 		ti->size = opt->progress ? sendDir(".", 1, true, tablespaces, true) : -1;
-		tablespaces = lappend(tablespaces, ti);
+		if (opt->includewal)
+			tablespaces = lappend(tablespaces, ti);
+		else
+			tablespaces = lcons(ti, tablespaces);
 
 		/* Send tablespace header */
 		SendBackupHeader(tablespaces);
-- 
2.1.4


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


Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-03-16 Thread Michael Banck
Hi,

sorry, it took me a while to get back to this.

Am Freitag, den 03.03.2017, 15:44 +0900 schrieb Michael Paquier:
> On Wed, Feb 22, 2017 at 9:23 PM, Bernd Helmle  wrote:
> > The comment in the code says explicitely to add the base directory to
> > the end of the list, not sure if that is out of a certain reason.
> >
> > I'd say this is an oversight in the implementation. I'm currently
> > working on a tool using the streaming protocol directly and i've
> > understood it exactly the way, that the default tablespace is the first
> > one in the stream.
> >
> > So +1 for the patch.
> 
> Commit 507069de has switched the main directory from the beginning to
> the end of the list, and the thread about this commit is here:
> https://www.postgresql.org/message-id/AANLkTikgmZRkBuQ%2B_hcwPBv7Cd7xW48Ev%3DUBHA-k4v0W%40mail.gmail.com
> 
> +   /* Add a node for the base directory at the beginning.  This way, the
> +* backup_label file is always the first file to be sent. */
> ti = palloc0(sizeof(tablespaceinfo));
> ti->size = opt->progress ? sendDir(".", 1, true, tablespaces,
> true) : -1;
> -   tablespaces = lappend(tablespaces, ti);
> +   tablespaces = lcons(ti, tablespaces);
> So, the main directory is located at the end on purpose. When using
> --wal-method=fetch the WAL segments are part of the main tarball, so
> if you send the main tarball first you would need to generate a second
> tarball with the WAL segments that have been generated between the
> moment the main tarball has finished until the end of the last
> tablespace taken if you want to have a consistent backup. 

Ah, thanks for pointing that out, I've missed that in my testing.

> Your patch would work with the stream mode though.

Or, if not requesting the "WAL" option of the replication protocol's
BASE_BACKUP command.

I agree it doesn't make sense to start messing with fetch mode, but I
don't think we guarantee any ordering of tablespaces (to wit, Bernd was
pretty sure it was the other way around all the time), neither do I
think having the main tablespace be the first for non-WAL/stream and the
last for WAL/fetch would be confusing personally, though I understand
this is debatable.

So I've updated the patch to only switch the main tablespace to be first
in case WAL isn't included, please find it attached.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

From 2532c4a659eb32527c489d1a65caa080e181dbd0 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 26 Feb 2017 17:59:38 +0100
Subject: [PATCH] Reorder tablespaces for non-WAL streaming basebackups.

The replication protocol documentation appears to express that the main
tablespace is the first to be sent, however, it is actually the last
one in order for the WAL files to be appended to it. This makes the
backup_label file (which gets prepended to the main tablespace)
inconveniently end up in the middle of the basebackup stream if other
tablespaces are present.

Change this so that the main tablespace is the first to be sent in case
no WAL files are requested, ensuring that backup_label is the first file
in the stream in this case.
---
 src/backend/replication/basebackup.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 09ecc15..ef3c115 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -230,10 +230,16 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		else
 			statrelpath = pgstat_stat_directory;
 
-		/* Add a node for the base directory at the end */
+		/* Add a node for the base directory, either at the end or (if
+		 * WAL is not included) at the beginning (if WAL is not
+		 * included).  This means the backup_label file is the first
+		 * file to be sent in the latter case. */
 		ti = palloc0(sizeof(tablespaceinfo));
 		ti->size = opt->progress ? sendDir(".", 1, true, tablespaces, true) : -1;
-		tablespaces = lappend(tablespaces, ti);
+		if (opt->includewal)
+			tablespaces = lappend(tablespaces, ti);
+		else
+			tablespaces = lcons(ti, tablespaces);
 
 		/* Send tablespace header */
 		SendBackupHeader(tablespaces);
-- 
2.1.4


-- 
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] Statement-level rollback

2017-03-07 Thread Michael Banck
On Tue, Mar 07, 2017 at 01:49:29PM -0700, legrand legrand wrote:
> JDBC has nothing and developers has to play with savepoint as described 
> http://blog.endpoint.com/2015/02/postgres-onerrorrollback-explained.html

JDBC has it since 9.4.1210 (2016-09-07), unless I am mistaken:

https://github.com/pgjdbc/pgjdbc/commit/adc08d57d2a9726309ea80d574b1db835396c1c8


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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 Index Scans

2017-03-06 Thread Michael Banck
Hi,

On Thu, Feb 16, 2017 at 08:14:28AM +0530, Amit Kapila wrote:
> On Thu, Feb 16, 2017 at 12:27 AM, Robert Haas  wrote:
> > On Wed, Feb 15, 2017 at 1:39 PM, Robert Haas  wrote:
> >> On Wed, Feb 15, 2017 at 7:11 AM, Amit Kapila  
> >> wrote:>
> >>> support related patch.  In anycase, to avoid confusion I am attaching
> >>> all the three patches with this e-mail.
> >>
> >> Committed guc_parallel_index_scan_v1.patch, with changes to the
> >> documentation and GUC descriptions.
> >
> > And committed parallel_index_opt_exec_support_v10.patch as well, with
> > a couple of minor tweaks.
> 
> Thanks a lot!  I think this is a big step forward for parallelism in
> PostgreSQL.  Now, we have another way to drive parallel scans and I
> hope many more queries can use parallelism.

Shouldn't the chapter 15.3 "Parallel Plans" in the documentation be
updated for this as well, or is this going to be taken care as a batch
at the ned of the development cycle, pending other added parallization
features?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Change in "policy" on dump ordering?

2017-03-06 Thread Michael Banck
Hi,

On Sat, Mar 04, 2017 at 02:49:36PM -0500, Peter Eisentraut wrote:
> On 3/1/17 08:36, Peter Eisentraut wrote:
> > On 2/22/17 18:24, Jim Nasby wrote:
> >>> Yes, by that logic matview refresh should always be last.
> >>
> >> Patches for head attached.
> >>
> >> RLS was the first item added after DO_REFRESH_MATVIEW, which was added 
> >> in 9.5. So if we want to treat this as a bug, they'd need to be patched 
> >> as well, which is a simple matter of swapping 33 and 34.
> > 
> > I wonder whether we should emphasize this change by assigning
> > DO_REFRESH_MATVIEW a higher number, like 100?
> 
> Since there wasn't any interest in that idea, I have committed Jim's
> patch as is.

Would this be a candidate for backpatching, or is the behaviour change
in pg_dump trumping the issues it solves?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Change in "policy" on dump ordering?

2017-02-27 Thread Michael Banck
Hi,

I've found the (AIUI) previous discussion about this, it's Bug #13907:
https://www.postgresql.org/message-id/flat/20160202161407.2778.24659%40wrigleys.postgresql.org#20160202161407.2778.24...@wrigleys.postgresql.org

On Wed, Feb 22, 2017 at 05:24:49PM -0600, Jim Nasby wrote:
> On 2/22/17 12:29 PM, Peter Eisentraut wrote:
> >On 2/22/17 10:14, Jim Nasby wrote:
> >>CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
> >>SELECT 0
> >>
> >>IOW, you can create matviews that depend on any other
> >>table/view/matview, but right now if the matview includes certain items
> >>it will mysteriously end up empty post-restore.
> >
> >Yes, by that logic matview refresh should always be last.

In 
https://www.postgresql.org/message-id/9af4bc32-7e55-a21d-47e7-608582a8c48d%402ndquadrant.com
you (Peter) wrote: 

"The reason that ACLs are restored last is that they could contain owner
self-revokes.  So anything that you run after the ACLs could fail
because of that.  I think a more complete fix would be to split up the
ACL restores into the general part, which you would run right after the
object is restored, and the owner revokes, which you would run last."
 
> Patches for head attached.

FWIW, Keven Grittner had proposed a more involved patch in 
https://www.postgresql.org/message-id/CACjxUsNmpQDL58zRm3EFS9atqGT8%2BX_2%2BFOCXpYBwWZw5wgi-A%40mail.gmail.com


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-02-26 Thread Michael Banck
Hi,

Am Sonntag, den 26.02.2017, 21:32 +0100 schrieb Magnus Hagander:

> On Sun, Feb 26, 2017 at 8:27 PM, Michael Banck
>  wrote:

> Agreed, and applied as one patch. Except I noticed you also fixed a
> couple of entries which were missing the progname in the messages -- I
> broke those out to a separate patch instead.

Thanks!

> Made a small change to "using as much I/O as available" rather than
> "as possible", which I think is a better wording, along with the
> change of the idle wording I suggested before. (but feel free to point
> it out to me if that's wrong). 

LGTM, I apparently missed your suggestion when I re-read the thread.

I am just wondering whether this could/should be back-patched, maybe? It
is not a bug fix, of course, but OTOH is rather small and probably
helpful to some users on current releases.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-02-26 Thread Michael Banck
Hi,

Am Dienstag, den 14.02.2017, 18:18 -0500 schrieb Robert Haas:
> On Tue, Feb 14, 2017 at 4:06 PM, Alvaro Herrera
>  wrote:
> > I'd rather have a --quiet mode instead.  If you're running it by hand,
> > you're likely to omit the switch, whereas when writing the cron job
> > you're going to notice lack of switch even before you let the job run
> > once.
> 
> Well, that might've been a better way to design it, but changing it
> now would break backward compatibility and I'm not really sure that's
> a good idea.  Even if it is, it's a separate concern from whether or
> not in the less-quiet mode we should point out that we're waiting for
> a checkpoint on the server side.

ISTM the consensus is that there should be no output in regular mode,
but a message should be displayed in verbose and progress mode.

So I went forth and also added a message in progress mode (unless
verbose messages are requested anyway).

Regarding the documentation, I tried to clarify the difference between
the checkpoint types a bit more, but I think any further action is
probably a larger rewrite of the documentation on this topic.

So attached are two patches, I've split it up in the documentation and
the code output part. I'll add it as one commitfest entry in the
"Clients" section though, as it's not really a big patch, unless
somebody thinks it should have a secon entry in "Documentation"?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From bcbe19855f9f94eadf9e47a7f3b9a920a7f2a616 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 26 Feb 2017 18:06:40 +0100
Subject: [PATCH 1/2] Documentation updates regarding checkpoints for
 basebackups.

Mention that fast and immediate checkpoints are the same, and add a paragraph to
the pg_basebackup documentation about the checkpoint taken on the remote server.
---
 doc/src/sgml/backup.sgml|  3 ++-
 doc/src/sgml/ref/pg_basebackup.sgml | 10 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 5f009ee..9485d87 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -862,7 +862,8 @@ SELECT pg_start_backup('label', false, false);
  ).  This is
  usually what you want, because it minimizes the impact on query
  processing.  If you want to start the backup as soon as
- possible, change the second parameter to true.
+ possible, change the second parameter to true, which will
+ issue an immediate checkpoint using as much I/O as possible.
 
 
 
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index c9dd62c..c197630 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -419,7 +419,7 @@ PostgreSQL documentation
   --checkpoint=fast|spread
   

-Sets checkpoint mode to fast or spread (default) (see ).
+Sets checkpoint mode to fast (immediate) or spread (default) (see ).

   
  
@@ -660,6 +660,14 @@ PostgreSQL documentation
   Notes
 
   
+   At the beginning of the backup, a checkpoint needs to be written on the
+   server the backup is taken from.  Especially if the option
+   --checkpoint=fast is not used, this can take some time
+   during which pg_basebackup will be idle on the
+   server it is running on.
+  
+
+  
The backup will include all files in the data directory and tablespaces,
including the configuration files and any additional files placed in the
directory by third parties, except certain temporary files managed by
-- 
2.1.4

From 1e4051dff9710382b6b4f63373a304c6ce70c4ac Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 26 Feb 2017 20:23:21 +0100
Subject: [PATCH 2/2] Mention initial checkpoint in pg_basebackup for
 verbose/progess output.

Before the actual data directory contents are streamed, a checkpoint is
taken on the remote server. Especially if no fast checkpoint is
requested, this can take quite a while during which the pg_basebackup
command apparently sits idle doing nothing.

To alert the user that work is being done on the remote server, mention
the checkpoint if verbose or progress output has been requested.  As
pg_basebackup does not output anything during regular operation, no
additional output is printed in this case.

Also harmonize some other verbose messages in passing.
---
 src/bin/pg_basebackup/pg_basebackup.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c

Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-02-26 Thread Michael Banck
Hi,

Am Sonntag, den 26.02.2017, 22:25 +0530 schrieb Robert Haas:
> On Tue, Feb 21, 2017 at 3:47 PM, Michael Banck
>  wrote:
> > So I am proposing the attached patch, which sends the base tablespace
> > first, and then all the other external tablespaces afterwards, thus
> > having base_backup be the first file in the tar in all cases. Does
> > anybody see a problem with that?
> 
> Please add this to commitfest.postgresql.org so it doesn't get forgotten.

Right, I was going to do that and have done so now, thanks for the
reminder.

Also, attached is a slightly changed patch which expands on the reason
of the change in the comment.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From 9829ad0da1950b0c928c8b2adfc93a98271930d1 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 26 Feb 2017 17:59:38 +0100
Subject: [PATCH] Reorder tablespaces for streaming basebackups.

The replication protocol documentation appears to express that the main
tablespace is th first to be sent, however, it is actually that last one. This
makes the backup_label file (which gets prepended to the main tablespace)
inconveniently end up in the middle of the basebackup stream if other
tablespaces are present.

Change this so that the main tablespace is the first to be sent, ensuring that
backup_label is the first file in the stream.
---
 src/backend/replication/basebackup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 09ecc15..b806205 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -230,10 +230,11 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		else
 			statrelpath = pgstat_stat_directory;
 
-		/* Add a node for the base directory at the end */
+		/* Add a node for the base directory at the beginning.  This way, the
+		 * backup_label file is always the first file to be sent. */
 		ti = palloc0(sizeof(tablespaceinfo));
 		ti->size = opt->progress ? sendDir(".", 1, true, tablespaces, true) : -1;
-		tablespaces = lappend(tablespaces, ti);
+		tablespaces = lcons(ti, tablespaces);
 
 		/* Send tablespace header */
 		SendBackupHeader(tablespaces);
-- 
2.1.4


-- 
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] Change in "policy" on dump ordering?

2017-02-22 Thread Michael Banck
Hi,

On Wed, Feb 22, 2017 at 05:24:49PM -0600, Jim Nasby wrote:
> On 2/22/17 12:29 PM, Peter Eisentraut wrote:
> >On 2/22/17 10:14, Jim Nasby wrote:
> >>CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
> >>SELECT 0
> >>
> >>IOW, you can create matviews that depend on any other
> >>table/view/matview, but right now if the matview includes certain items
> >>it will mysteriously end up empty post-restore.
> >
> >Yes, by that logic matview refresh should always be last.

Glad to hear - I vaguely remember this coming up in a different thread
some time ago, and I though you (Peter) had reservations about moving
things past after the ACL step, but I couldn't find this thread now
anymore, only
https://www.postgresql.org/message-id/11166.1424357659%40sss.pgh.pa.us

> Patches for head attached.

Yay.

> diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
> index ea643397ba..708a47f3cb 100644
> --- a/src/bin/pg_dump/pg_dump_sort.c
> +++ b/src/bin/pg_dump/pg_dump_sort.c
> @@ -26,6 +26,9 @@ static const char *modulename = gettext_noop("sorter");
>   * Sort priority for database object types.
>   * Objects are sorted by type, and within a type by name.
>   *
> + * Because materialized views can potentially reference system views,
> + * DO_REFRESH_MATVIEW should always be the last thing on the list.
> + *

I think this comment is overly specific: any materialized view that
references a view or table in a different schema (pg_catalog or not)
will likely not refresh on pg_restore AIUI, so singling out system views
doesn't look right to me.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


[HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

2017-02-21 Thread Michael Banck
Hi,

currently, the backup_label and (I think) the tablespace_map files are
(by design) conveniently located at the beginning of the main tablespace
tarball when making a basebackup. However, (by accident or also by
design?) the main tablespace is the last tarball[1] to be streamed via
the BASE_BACKUP replication protocol command. 

For pg_basebackup, this is not a real problem, as either each tablespace
is its own tarfile (in tar format mode), or the files are extracted
anyway (in plain mode).

However, third party tools using the BASE_BACKUP command might want to
extract the backup_label, e.g. in order to figure out the START WAL
LOCATION. If they make a big tarball for the whole cluster potentially
including all external tablespaces, then the backup_label file is
somewhere in the middle of it and it takes a long time for tar to
extract it.

So I am proposing the attached patch, which sends the base tablespace
first, and then all the other external tablespaces afterwards, thus
having base_backup be the first file in the tar in all cases. Does
anybody see a problem with that?


Michael

[1] Chapter 52.3 of the documentation says "one or more CopyResponse
results will be sent, one for the main data directory and one for each
additional tablespace other than pg_default and pg_global.", which makes
it sound like the main data directory is first, but in my testing, this
is not the case.

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 09ecc15..a5a96b7 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -230,10 +230,10 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		else
 			statrelpath = pgstat_stat_directory;
 
-		/* Add a node for the base directory at the end */
+		/* Add a node for the base directory at the beginning */
 		ti = palloc0(sizeof(tablespaceinfo));
 		ti->size = opt->progress ? sendDir(".", 1, true, tablespaces, true) : -1;
-		tablespaces = lappend(tablespaces, ti);
+		tablespaces = lcons(ti, tablespaces);
 
 		/* Send tablespace header */
 		SendBackupHeader(tablespaces);

-- 
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] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-02-13 Thread Michael Banck
Hi,

Am Montag, den 13.02.2017, 09:31 +0100 schrieb Magnus Hagander:
> On Mon, Feb 13, 2017 at 3:29 AM, Jim Nasby 
> wrote:
> On 2/11/17 4:36 AM, Michael Banck wrote:
> I guess you're right, I've moved it further down.
> There is in fact a
> message about the xlog location (unless you switch off
> wal entirely),
> but having another one right before that mentioning
> the completed
> checkpoint sounds ok to me.
> 
> 1) I don't think this should be verbose output. Having a
> program sit there "doing nothing" for no apparent reason is
> just horrible UI design.
> 
> 
> That would include much of Unix then.. For example if I run "cp" on a
> large file it sits around "doing nothing". Same if I do "tar". No?

The expectation for all three commands is that, even if there is no
output on stdout, they will write data to the local machine. So you can
easily monitor the progress of cp and tar by running du or something in
a different terminal.

With pg_basebackup, nothing is happening on the local machine until the
checkpoint on the remote is finished; while this is obvious to somebody
familiar with how basebackups work internally, it appears to be not
clear at all to some users.

So I think notifying the user that something is happening remotely while
the local process waits would be useful, but on the other hand,
pg_basebackup does not print anything unless (i) --verbose is set or
(ii) there is an error, so I think having it mention the checkpoint in
--verbose mode only is acceptable.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-02-11 Thread Michael Banck
Hi,

Am Samstag, den 11.02.2017, 11:25 +0100 schrieb Michael Banck:
> Am Samstag, den 11.02.2017, 11:07 +0100 schrieb Magnus Hagander:
> > As for the code, while I haven't tested it, isn't the "checkpoint
> > completed" message in the wrong place? Doesn't PQsendQuery() complete
> > immediately, and the check needs to be put *after* the PQgetResult()
> > call?
> 
> I guess you're right, I've moved it further down. There is in fact a
> message about the xlog location (unless you switch off wal entirely),
> but having another one right before that mentioning the completed
> checkpoint sounds ok to me.
> 
> There's also some inconsistencies around which messages are prepended
> with "pg_basebackup: " and which are translatable; I guess all messages
> printed on --verbose should be translatable? Also, as almost all
> messages have a "pg_basebackup: " prefix, I've added it to the rest.

Sorry, there were two typoes in the last patch, I've attached a fixed
one.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index c9dd62c..a298e5c 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -660,6 +660,14 @@ PostgreSQL documentation
   Notes
 
   
+   At the beginning of the backup, a checkpoint needs to be written on the
+   server the backup is taken from.  Especially if the option
+   --checkpoint=fast is not used, this can take some time
+   during which pg_basebackup will be idle on the
+   server it is running on.
+  
+
+  
The backup will include all files in the data directory and tablespaces,
including the configuration files and any additional files placed in the
directory by third parties, except certain temporary files managed by
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index b6463fa..60200a9 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1754,6 +1754,11 @@ BaseBackup(void)
 	if (maxrate > 0)
 		maxrate_clause = psprintf("MAX_RATE %u", maxrate);
 
+	if (verbose)
+		fprintf(stderr,
+_("%s: initiating base backup, waiting for checkpoint to complete\n"),
+progname);
+
 	basebkp =
 		psprintf("BASE_BACKUP LABEL '%s' %s %s %s %s %s %s",
  escaped_label,
@@ -1791,6 +1796,9 @@ BaseBackup(void)
 
 	strlcpy(xlogstart, PQgetvalue(res, 0, 0), sizeof(xlogstart));
 
+	if (verbose)
+		fprintf(stderr, _("%s: checkpoint completed\n"), progname);
+
 	/*
 	 * 9.3 and later sends the TLI of the starting point. With older servers,
 	 * assume it's the same as the latest timeline reported by
@@ -1804,8 +1812,8 @@ BaseBackup(void)
 	MemSet(xlogend, 0, sizeof(xlogend));
 
 	if (verbose && includewal != NO_WAL)
-		fprintf(stderr, _("transaction log start point: %s on timeline %u\n"),
-xlogstart, starttli);
+		fprintf(stderr, _("%s: transaction log start point: %s on timeline %u\n"),
+progname, xlogstart, starttli);
 
 	/*
 	 * Get the header
@@ -1907,7 +1915,7 @@ BaseBackup(void)
 	}
 	strlcpy(xlogend, PQgetvalue(res, 0, 0), sizeof(xlogend));
 	if (verbose && includewal != NO_WAL)
-		fprintf(stderr, "transaction log end point: %s\n", xlogend);
+		fprintf(stderr, _("%s: transaction log end point: %s\n"), progname, xlogend);
 	PQclear(res);
 
 	res = PQgetResult(conn);
@@ -2048,7 +2056,7 @@ BaseBackup(void)
 	}
 
 	if (verbose)
-		fprintf(stderr, "%s: base backup completed\n", progname);
+		fprintf(stderr, _("%s: base backup completed\n"), progname);
 }
 
 

-- 
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] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-02-11 Thread Michael Banck
Hi,


Am Samstag, den 11.02.2017, 11:07 +0100 schrieb Magnus Hagander:


> As for the code, while I haven't tested it, isn't the "checkpoint
> completed" message in the wrong place? Doesn't PQsendQuery() complete
> immediately, and the check needs to be put *after* the PQgetResult()
> call?

I guess you're right, I've moved it further down. There is in fact a
message about the xlog location (unless you switch off wal entirely),
but having another one right before that mentioning the completed
checkpoint sounds ok to me.

There's also some inconsistencies around which messages are prepended
with "pg_basebackup: " and which are translatable; I guess all messages
printed on --verbose should be translatable? Also, as almost all
messages have a "pg_basebackup: " prefix, I've added it to the rest.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index c9dd62c..a298e5c 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -660,6 +660,14 @@ PostgreSQL documentation
   Notes
 
   
+   At the beginning of the backup, a checkpoint needs to be written on the
+   server the backup is taken from.  Especially if the option
+   --checkpoint=fast is not used, this can take some time
+   during which pg_basebackup will be idle on the
+   server it is running on.
+  
+
+  
The backup will include all files in the data directory and tablespaces,
including the configuration files and any additional files placed in the
directory by third parties, except certain temporary files managed by
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index b6463fa..874b6d6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1754,6 +1754,11 @@ BaseBackup(void)
 	if (maxrate > 0)
 		maxrate_clause = psprintf("MAX_RATE %u", maxrate);
 
+	if (verbose)
+		fprintf(stderr,
+_("%s: initiating base backup, waiting for checkpoint to complete\n"),
+progname);
+
 	basebkp =
 		psprintf("BASE_BACKUP LABEL '%s' %s %s %s %s %s %s",
  escaped_label,
@@ -1791,6 +1796,9 @@ BaseBackup(void)
 
 	strlcpy(xlogstart, PQgetvalue(res, 0, 0), sizeof(xlogstart));
 
+	if (verbose)
+		fprintf(stderr, _("%s: checkpoint completed\n"), progname);
+
 	/*
 	 * 9.3 and later sends the TLI of the starting point. With older servers,
 	 * assume it's the same as the latest timeline reported by
@@ -1804,8 +1812,8 @@ BaseBackup(void)
 	MemSet(xlogend, 0, sizeof(xlogend));
 
 	if (verbose && includewal != NO_WAL)
-		fprintf(stderr, _("transaction log start point: %s on timeline %u\n"),
-xlogstart, starttli);
+		fprintf(stderr, _("%s: transaction log start point: %s on timeline %u\n"),
+progname, xlogstart, starttli);
 
 	/*
 	 * Get the header
@@ -1907,7 +1915,7 @@ BaseBackup(void)
 	}
 	strlcpy(xlogend, PQgetvalue(res, 0, 0), sizeof(xlogend));
 	if (verbose && includewal != NO_WAL)
-		fprintf(stderr, "transaction log end point: %s\n", xlogend);
+		fprintf(stderr, _("%s: transaction log end point: %s\n", progname, xlogend);
 	PQclear(res);
 
 	res = PQgetResult(conn);
@@ -2048,7 +2056,7 @@ BaseBackup(void)
 	}
 
 	if (verbose)
-		fprintf(stderr, "%s: base backup completed\n", progname);
+		fprintf(stderr, _("%s: base backup completed\n)", progname);
 }
 
 

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


[HACKERS] gitlab post-mortem: pg_basebackup waiting for checkpoint

2017-02-11 Thread Michael Banck
Hi,

one take-away from the Gitlab Post-Mortem[1] appears to be that after
their secondary lost replication, they were confused about what
pg_basebackup was doing when they tried to rebuild it. It just sat there
and did nothing (even with --verbose), so they assumed something was
wrong with either the primary or the connection, and restarted it
several times.

AFAICT, it turns out the checkpoint was written on the master (they
probably did not use -c fast), but this wasn't obvious to them:

"One of the engineers went to the secondary and wiped the data
directory, then ran pg_basebackup. Unfortunately pg_basebackup would
hang, producing no meaningful output, despite the --verbose option being
set."

[...]

"Unfortunately this did not resolve the problem of pg_basebackup not
starting replication immediately. One of the engineers decided to run it
with strace to see what it was blocking on. strace showed that
pg_basebackup was hanging in a poll call, but that did not provide any
other meaningful information that might have explained why."

[...]

"It would later be revealed by another engineer (who wasn't around at
the time) that this is normal behavior: pg_basebackup will wait for the
primary to start sending over replication data and it will sit and wait
silently until that time. Unfortunately this was not clearly documented
in our engineering runbooks nor in the official pg_basebackup document."

ISTM that even with WAL streaming, nothing would be written on the
client server until the checkpoint is complete, as do_pg_start_backup()
runs the checkpoint and only returns the starting WAL location
afterwards.

The attached (untested) patch is to kick of a discussion on how to
improve the situation, it is supposed to mention the checkpoint when
--verbose is used and adds a paragraph about the checkpoint being run to
the Notes section of the documentation.


Michael

[1]https://about.gitlab.com/2017/02/10/postmortem-of-database-outage-of-january-31/

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index c9dd62c..a298e5c 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -660,6 +660,14 @@ PostgreSQL documentation
   Notes
 
   
+   At the beginning of the backup, a checkpoint needs to be written on the
+   server the backup is taken from.  Especially if the option
+   --checkpoint=fast is not used, this can take some time
+   during which pg_basebackup will be idle on the
+   server it is running on.
+  
+
+  
The backup will include all files in the data directory and tablespaces,
including the configuration files and any additional files placed in the
directory by third parties, except certain temporary files managed by
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index b6463fa..ae18c16 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1754,6 +1754,9 @@ BaseBackup(void)
 	if (maxrate > 0)
 		maxrate_clause = psprintf("MAX_RATE %u", maxrate);
 
+	if (verbose)
+		fprintf(stderr, "%s: initiating base backup, waiting for checkpoint to complete\n", progname);
+
 	basebkp =
 		psprintf("BASE_BACKUP LABEL '%s' %s %s %s %s %s %s",
  escaped_label,
@@ -1771,6 +1774,9 @@ BaseBackup(void)
 		disconnect_and_exit(1);
 	}
 
+	if (verbose)
+		fprintf(stderr, "%s: checkpoint completed\n", progname);
+
 	/*
 	 * Get the starting xlog position
 	 */

-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

2017-02-07 Thread Michael Banck
Hi,

Am Dienstag, den 07.02.2017, 15:58 -0500 schrieb Jonathan S. Katz:


> https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.txt;h=f90d4716f240dbea4cca647b099f79865545b633;hb=d85498c284275bcab4752b91476834de780648b8

It says "[...]then rows that were updated by transactions running at the
same time as the CREATE INDEX CONCURRENTLY command could have been index
incorrectly."

That sounds off to me, shouldn't it be "indexed incorrectly" or
something?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

2017-02-07 Thread Michael Banck
Hi,

Am Dienstag, den 07.02.2017, 10:37 -0500 schrieb Jonathan S. Katz:

> Below is the draft of the press release for the update this Thursday:

About the CREATE INDEX CONCURRENTLY issue, I wonder whether Peter's
amcheck extension[1] would catch that (for B-Tree indexes at least), and
if that is the case, whether we could mention that to our users as
guidance for how to check for index corruption?


Michael

[1] https://github.com/petergeoghegan/amcheck
-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




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

2017-01-26 Thread Michael Banck
On Thu, Jan 26, 2017 at 12:37:44PM -0500, Peter Eisentraut wrote:
> On 1/24/17 8:19 AM, Tom Lane wrote:
> > What about just saying that the database owner has those privileges?
> > After all, the ultimate privilege of an owner is to drop the object
> > (and then remake it as she pleases), and the DB owner has that option
> > w.r.t. the whole database.  So I'm not sure we need to invent a new
> > concept.
> 
> A database owner does not necessarily have the permission to create a
> new database.

Right.

Would a "TRUNCATE ;" (i.e. empty the database, but don't
delete it) make sense/be useful for that maybe?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Checksums by default?

2017-01-21 Thread Michael Banck
On Sat, Jan 21, 2017 at 09:02:25PM +0200, Ants Aasma wrote:
> On Sat, Jan 21, 2017 at 6:41 PM, Andreas Karlsson  wrote:
> > It might be worth looking into using the CRC CPU instruction to reduce this
> > overhead, like we do for the WAL checksums. Since that is a different
> > algorithm it would be a compatibility break and we would need to support the
> > old algorithm for upgraded clusters..
> 
> We looked at that when picking the algorithm. At that point it seemed
> that CRC CPU instructions were not universal enough to rely on them.
> The algorithm we ended up on was designed to be fast on SIMD hardware.
> Unfortunately on x86-64 that required SSE4.1 integer instructions, so
> with default compiles there is a lot of performance left on table. A
> low hanging fruit would be to do CPU detection like the CRC case and
> enable a SSE4.1 optimized variant when those instructions are
> available. IIRC it was actually a lot faster than the naive hardware
> CRC that is used for WAL and about on par with interleaved CRC.

I am afraid that won't fly with most end-user packages, cause
distributions can't just built packages on their newest machine and then
users get SIGILL or whatever cause their 2014 server doesn't have that
instruction, or would they still work?

So you would have to do runtime detection of the CPU features, and use
the appropriate code if they are available.  Which also makes regression
testing harder, as not all codepaths would get exercised all the time.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


Re: [HACKERS] Replication/backup defaults

2017-01-05 Thread Michael Banck
On Mon, Jan 02, 2017 at 10:21:41AM +0100, Magnus Hagander wrote:
> On Mon, Jan 2, 2017 at 10:17 AM, Simon Riggs  wrote:
> > On 31 December 2016 at 15:00, Magnus Hagander  wrote:
> > > max_wal_senders=10
> > > max_replication_slots=20

[...]

> > > wal_level=replica
> >
> > This is more problematic because it changes behaviours.
> 
> You can't actually change the other two without changing wal_level.

That actually goes both ways: I recently saw a server not start cause we
were experimenting with temporarily setting wal_level to minimal for
initial bulk loading, but did not reduce max_wal_senders back to zero.
So it failed at startup with 'FATAL:  WAL streaming (max_wal_senders >
0) requires wal_level "replica" or "logical"'.

I don't want to hijack this thread, but I wonder whether the name
"*max*_wal_senders" really conveys that dependence on wal_level (there's
no comment to that end in the postgresql.conf sample) and/or whether
maybe the admin should just be notified that WAL streaming is turned off
cause wal_level < 'replica'?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Reporting planning time with EXPLAIN

2016-12-28 Thread Michael Banck
On Wed, Dec 28, 2016 at 02:08:55PM +0100, Michael Banck wrote:
> On Tue, Dec 27, 2016 at 09:26:21AM -0500, Stephen Frost wrote:
> > * Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
> > > We report planning and execution time when EXPLAIN ANALYZE is issued.
> > > We do not have facility to report planning time as part EXPLAIN
> > > output. In order to get the planning time, one has to issue EXPLAIN
> > > ANALYZE which involves executing the plan, which is unnecessary.
> > 
> > After reading that, rather long, thread, I agree that just having it be
> > 'summary' is fine.  We don't really want to make it based off of
> > 'timing' or 'costs' or 'verbose', those are different things.
> 
> I'm wondering, why is 'timing' (in the EXPLAIN scope) a different thing
> to planning time?  It might be that people don't expect it at this point
> and external tools might break (dunno), but in oder to print the
> planning time, 'timing' sounds clearer than 'summary' to me.

OK, after also rereading the thread, this indeed seems to be very
complicated, and I don't want to reopen this can of worms, sorry.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Reporting planning time with EXPLAIN

2016-12-28 Thread Michael Banck
On Tue, Dec 27, 2016 at 09:26:21AM -0500, Stephen Frost wrote:
> * Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
> > We report planning and execution time when EXPLAIN ANALYZE is issued.
> > We do not have facility to report planning time as part EXPLAIN
> > output. In order to get the planning time, one has to issue EXPLAIN
> > ANALYZE which involves executing the plan, which is unnecessary.
> 
> After reading that, rather long, thread, I agree that just having it be
> 'summary' is fine.  We don't really want to make it based off of
> 'timing' or 'costs' or 'verbose', those are different things.

I'm wondering, why is 'timing' (in the EXPLAIN scope) a different thing
to planning time?  It might be that people don't expect it at this point
and external tools might break (dunno), but in oder to print the
planning time, 'timing' sounds clearer than 'summary' to me.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

2016-11-22 Thread Michael Banck
Hi,

On Fri, Nov 11, 2016 at 07:42:05PM +0100, Andreas Karlsson wrote:
> On 11/11/2016 07:40 PM, Andreas Karlsson wrote:
> >Here is a new version of the patch with the only differences;
> >
> >1) The SSL tests have been changed to use reload rather than restart
> >
> >2) Rebased on master
> 
> And here with a fix to a comment.

Michael, as you brought up the issues with the SSL tests, do you plan to
review the latest version (and add yourself as reviewer)?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

2016-11-08 Thread Michael Banck
Hi,

On Mon, Nov 07, 2016 at 02:36:00AM +0100, Andreas Karlsson wrote:
> Thanks for the review! I have fixed all your feedback in the attached
> patch.

Thanks! I couldn't find furhter faults in my testing. I guess the
question what to do about this on Windows is possibly still open, but as
I am not familiar with the Windows port at all I've marked it Ready for
Committer for now.


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

2016-11-03 Thread Michael Banck
>  extern bool secure_loaded_verify_locations(void);
>  extern void secure_destroy(void);
>  extern int   secure_open_server(Port *port);
> 

This hunk baffled me at first because two lines below your added
secure_destroy() declaration, the same line is already present.  I did
some digging and it turns out we had a secure_destroy() in the ancient
past, but its implementation got removed in 2008 in 4e8162865 as there
were no (more) users of it, however, the declaration was kept on until
now.

So this hunk should be removed I guess.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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


[HACKERS] pg_rewind: Should abort if both --source-pgdata and --source-server are specified

2016-10-07 Thread Michael Banck
Hi,

ISTM that pg_rewind's --source-pgdata and --source-server options are
mutually exclusive, and the synopsis in the documentation seems to
indicate that as well: 

|pg_rewind [option...] {-D | --target-pgdata} directory
|{--source-pgdata=directory | --source-server=connstr}

However, there is no such check in the code.

I've seen people assume --source-pgdata is supposed to be the data
directory location on the remote server if they specify --source-server
as well, and are then confused by error messages.

So I think pg_rewind should abort in this case.  Patch for that attached.

Thoughts?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 5fdd4c5..dd62dd0 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -162,6 +162,13 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
+	if (datadir_source != NULL && connstr_source != NULL)
+	{
+		fprintf(stderr, _("%s: only one of --source-pgdata or --source-server can be specified\n"), progname);
+		fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+		exit(1);
+	}
+
 	if (datadir_target == NULL)
 	{
 		fprintf(stderr, _("%s: no target data directory specified (--target-pgdata)\n"), progname);

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


Re: [HACKERS] pg_rewind hangs if --source-server is used and syncrep is enabled

2016-10-05 Thread Michael Banck
On Wed, Oct 05, 2016 at 04:39:39PM +0200, Michael Banck wrote:
> if pg_rewind is told to fetch data via a libpq connection
> (--source-server), synchronous replication is enabled and there is only
> one sync standby (pilot error, but sill); pg_rewinding the old master
> hangs at the CREATE TEMPORARY TABLE step (CREATE TABLE waiting for
> X/).  At least this happened to one of our clients while
> evaluating pg_rewind.
> 
> To the user, the last thing printed is "need to copy  MB [...]".  If
> the user cancels the pg_rewind command with ^C, the backend keeps
> hanging around even in --dry-run mode.  That won't hurt too much as it
> does not seem to block future pg_rewind runs after synchronous_commit
> has been set to a different value, but looks surprising to me.
> 
> Not sure whether pg_rewind could error out gracefully without hanging in
> this case, 

My colleague Christoph Berg pointed out that pg_rewind could just set
synchronous_commit = local before creating the temporary table, which
indeed works, proof-of-concept patch attached


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index 9239009..a5cb426 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -406,7 +406,7 @@ libpq_executeFileMap(filemap_t *map)
 	 * First create a temporary table, and load it with the blocks that we
 	 * need to fetch.
 	 */
-	sql = "CREATE TEMPORARY TABLE fetchchunks(path text, begin int4, len int4);";
+	sql = "SET synchronous_commit = local; CREATE TEMPORARY TABLE fetchchunks(path text, begin int4, len int4);";
 	res = PQexec(conn, sql);
 
 	if (PQresultStatus(res) != PGRES_COMMAND_OK)

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


[HACKERS] pg_rewind hangs if --source-server is used and syncrep is enabled

2016-10-05 Thread Michael Banck
Hi,

if pg_rewind is told to fetch data via a libpq connection
(--source-server), synchronous replication is enabled and there is only
one sync standby (pilot error, but sill); pg_rewinding the old master
hangs at the CREATE TEMPORARY TABLE step (CREATE TABLE waiting for
X/).  At least this happened to one of our clients while
evaluating pg_rewind.

To the user, the last thing printed is "need to copy  MB [...]".  If
the user cancels the pg_rewind command with ^C, the backend keeps
hanging around even in --dry-run mode.  That won't hurt too much as it
does not seem to block future pg_rewind runs after synchronous_commit
has been set to a different value, but looks surprising to me.

Not sure whether pg_rewind could error out gracefully without hanging in
this case, but maybe it could/should clean up the query on SIGTERM? And
at the least, maybe this caveat should be documented?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Exclude schema during pg_restore

2016-09-22 Thread Michael Banck
Hi,

On Tue, Sep 20, 2016 at 08:59:37PM -0400, Peter Eisentraut wrote:
> On 9/19/16 3:23 PM, Michael Banck wrote:
> > Version 2 attached.
> 
> Committed, thanks.
 
Thanks!

> I added the new option to the help output in pg_restore.

Oh, sorry I missed that.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Exclude schema during pg_restore

2016-09-19 Thread Michael Banck
Hi,

sorry, it took me a while to find time to look at this.

On Thu, Sep 01, 2016 at 09:39:56PM -0400, Peter Eisentraut wrote:
> On 8/31/16 4:10 AM, Michael Banck wrote:
> > attached is a small patch that adds an -N option to pg_restore, in order
> > to exclude a schema, in addition to -n for the restriction to a schema.
> 
> I think this is a good idea, and the approach looks sound.  However,
> something doesn't work right.  If I take an empty database and dump it,
> it will dump the plpgsql extension.  If I run pg_dump in plain-text mode
> with -N, then the plpgsql extension is also dumped (since it is not in
> the excluded schema).  But if I use the new pg_restore -N option, the
> plpgsql extension is not dumped.  Maybe this is because it doesn't have
> a schema, but I haven't checked.

I was afraid that this might need major code surgery, but in the end it
seems this was just a thinko on my part in tocEntryRequired(). For the
exclude-schema case, we shouldn't skip objects without a namespace (like
the plpgsql extension you mentioned above).
 
> pg_dump does not apply --strict-names to -N, but your patch for
> pg_restore does that.  I think that should be made the same as pg_dump.

Ok, I've removed that hunk.

Version 2 attached.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index c906919..e5eb18e 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -315,6 +315,17 @@
  
 
  
+  -N namespace
+  --exclude-schema=schema
+  
+   
+Do not restore objects that are in the named schema.  Multiple schemas
+to be excluded may be specified with multiple -N switches.
+   
+  
+ 
+
+ 
   -O
   --no-owner
   
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 4afa92f..0a28124 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -99,6 +99,7 @@ typedef struct _restoreOptions
 	SimpleStringList indexNames;
 	SimpleStringList functionNames;
 	SimpleStringList schemaNames;
+	SimpleStringList schemaExcludeNames;
 	SimpleStringList triggerNames;
 	SimpleStringList tableNames;
 
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 05bdbdb..0081d2f 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2751,6 +2751,9 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
 			return 0;
 	}
 
+	if ((ropt->schemaExcludeNames.head != NULL) && te->namespace && simple_string_list_member(&ropt->schemaExcludeNames, te->namespace))
+		return 0;
+
 	if (ropt->selTypes)
 	{
 		if (strcmp(te->desc, "TABLE") == 0 ||
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index fb08e6b..3be8654 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -85,6 +85,7 @@ main(int argc, char **argv)
 		{"data-only", 0, NULL, 'a'},
 		{"dbname", 1, NULL, 'd'},
 		{"exit-on-error", 0, NULL, 'e'},
+		{"exclude-schema", 1, NULL, 'N'},
 		{"file", 1, NULL, 'f'},
 		{"format", 1, NULL, 'F'},
 		{"function", 1, NULL, 'P'},
@@ -148,7 +149,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:Op:P:RsS:t:T:U:vwWx1",
+	while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
 			cmdopts, NULL)) != -1)
 	{
 		switch (c)
@@ -196,6 +197,10 @@ main(int argc, char **argv)
 simple_string_list_append(&opts->schemaNames, optarg);
 break;
 
+			case 'N':			/* Do not dump data for this schema */
+simple_string_list_append(&opts->schemaExcludeNames, optarg);
+break;
+
 			case 'O':
 opts->noOwner = 1;
 break;

-- 
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] Exclude schema during pg_restore

2016-09-01 Thread Michael Banck
Am Donnerstag, den 01.09.2016, 21:39 -0400 schrieb Peter Eisentraut:
> On 8/31/16 4:10 AM, Michael Banck wrote:
> > attached is a small patch that adds an -N option to pg_restore, in order
> > to exclude a schema, in addition to -n for the restriction to a schema.
> 
> I think this is a good idea, and the approach looks sound.  However,
> something doesn't work right.  If I take an empty database and dump it,
> it will dump the plpgsql extension.  If I run pg_dump in plain-text mode
> with -N, then the plpgsql extension is also dumped (since it is not in
> the excluded schema).  But if I use the new pg_restore -N option, the
> plpgsql extension is not dumped.  Maybe this is because it doesn't have
> a schema, but I haven't checked.

Thanks for the testing and feedback, I hadn't thought of issues with
extensions when I tested myself.  I will take a look.

> pg_dump does not apply --strict-names to -N, but your patch for
> pg_restore does that.  I think that should be made the same as pg_dump.

Aye.


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




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


Re: [HACKERS] Proposal for changes to recovery.conf API

2016-08-31 Thread Michael Banck
On Wed, Aug 31, 2016 at 05:15:59PM +0100, Simon Riggs wrote:
> Recover mode starts the server as a standby server which
> expects to receive changes from a primary/master server using physical
> streaming replication or is used for performing a recovery from
> backup. 

I understand where this is coming from, but wouldn't "standby",
"replication" or something be more generally understood than "recover"?
Streaming replication got bolted on to recovery, but that seems like an
implementation detail by now.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Exclude schema during pg_restore

2016-08-31 Thread Michael Banck
Hi,

Am Mittwoch, den 31.08.2016, 07:59 -0300 schrieb Fabrízio de Royes
Mello:

> Please add it to the next open commitfest.

I had done so already: https://commitfest.postgresql.org/10/762/


Regards,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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 schema during pg_restore

2016-08-31 Thread Michael Banck
Hi,

attached is a small patch that adds an -N option to pg_restore, in order
to exclude a schema, in addition to -n for the restriction to a schema.

In principle, this could be extended to -t etc., but I think having this
for schemas would be the most useful with the least effort.

One use case for this would be the need to restore one or more schemas
first (using -n foo), then all the others (now using -N foo) without (i)
having to specify them all with -n and (ii) getting errors due to
already restored objects from the initial schema. While users could be
told to just ignore the errors/warnings, it would be useful for
automation when you would like to check for zero errors/warning, for
example.

I have so far seen two reasons for this use case: (i) Add-ons that are
not yet an extension and install objects in public (e.g. ESRI ArcGIS),
requiring the public schema to be present already on restore of user
schemas and (ii) restoring materialized views that reference objects
from other schemas; as permissions are restored last, no permissions
have been granted for those other schemas yet.

Argueably, those reasons could be dealt with as well, but this seems to
be a generally useful addition to pg_restore, in my opinion.


Michael

-- 
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index c906919..e5eb18e 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -315,6 +315,17 @@
  
 
  
+  -N namespace
+  --exclude-schema=schema
+  
+   
+Do not restore objects that are in the named schema.  Multiple schemas
+to be excluded may be specified with multiple -N switches.
+   
+  
+ 
+
+ 
   -O
   --no-owner
   
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 4afa92f..0a28124 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -99,6 +99,7 @@ typedef struct _restoreOptions
 	SimpleStringList indexNames;
 	SimpleStringList functionNames;
 	SimpleStringList schemaNames;
+	SimpleStringList schemaExcludeNames;
 	SimpleStringList triggerNames;
 	SimpleStringList tableNames;
 
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 05bdbdb..37063ba 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2674,6 +2674,13 @@ StrictNamesCheck(RestoreOptions *ropt)
 			exit_horribly(modulename, "schema \"%s\" not found\n", missing_name);
 	}
 
+	if (ropt->schemaExcludeNames.head != NULL)
+	{
+		missing_name = simple_string_list_not_touched(&ropt->schemaExcludeNames);
+		if (missing_name != NULL)
+			exit_horribly(modulename, "schema \"%s\" not found\n", missing_name);
+	}
+
 	if (ropt->tableNames.head != NULL)
 	{
 		missing_name = simple_string_list_not_touched(&ropt->tableNames);
@@ -2751,6 +2758,15 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
 			return 0;
 	}
 
+	if (ropt->schemaExcludeNames.head != NULL)
+	{
+		/* If no namespace is specified, it means all. */
+		if (!te->namespace)
+			return 0;
+		if ((simple_string_list_member(&ropt->schemaExcludeNames, te->namespace)))
+			return 0;
+	}
+
 	if (ropt->selTypes)
 	{
 		if (strcmp(te->desc, "TABLE") == 0 ||
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index fb08e6b..3be8654 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -85,6 +85,7 @@ main(int argc, char **argv)
 		{"data-only", 0, NULL, 'a'},
 		{"dbname", 1, NULL, 'd'},
 		{"exit-on-error", 0, NULL, 'e'},
+		{"exclude-schema", 1, NULL, 'N'},
 		{"file", 1, NULL, 'f'},
 		{"format", 1, NULL, 'F'},
 		{"function", 1, NULL, 'P'},
@@ -148,7 +149,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:Op:P:RsS:t:T:U:vwWx1",
+	while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
 			cmdopts, NULL)) != -1)
 	{
 		switch (c)
@@ -196,6 +197,10 @@ main(int argc, char **argv)
 simple_string_list_append(&opts->schemaNames, optarg);
 break;
 
+			case 'N':			/* Do not dump data for this schema */
+simple_string_list_append(&opts->schemaExcludeNames, optarg);
+break;
+
 			case 'O':
 opts->noOwner = 1;
 break;

-- 
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] Is there a way around function search_path killing SQL function inlining?

2016-08-10 Thread Michael Banck
On Thu, Mar 10, 2016 at 11:48:41AM -0500, Tom Lane wrote:
> If you're worried about preserving relocatability of an extension
> containing such functions, the @extschema@ feature might help.

As I've been bitten by this problem recently, I thought I'd take a look
at editing the PostGIS extension SQL file to this end, but contrary to
the above, the @extschema@ feature only applies to non-relocatable
extensions, from src/backend/commands/extension.c:

  * If it's not relocatable, substitute the target schema name for
  * occurrences of @extschema@.
  *
  * For a relocatable extension, we needn't do this.  There cannot be
  * any need for @extschema@, else it wouldn't be relocatable.

I'm not sure that logic is sound - even if setting @extschema@ 
explicitly in the SQL functions bodies kills inlining (not sure about
that) or wouldn't help for other reasons, ISTM this should be 
reconsidered in the light of the use case with materialized views during
restore.


Best regards,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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

2016-05-14 Thread Michael Banck
On Fri, May 13, 2016 at 08:55:20PM -0400, David G. Johnston wrote:
> The opinion seems to be that major.0 is some kind of magic incantation in
> the broader world of users...

>From my reading of the thread, while certainly that is the general
definition of a .0, having infrequent .0 releases is not very practical
for PostgreSQL because the major versions are not that different from
each other and all are treated the same development-wise. So it would be
a huge drain on the project to discuss which major version should be a
.0 unless planning towards them steps up significantly.

So I think the (slight) consensus is more that all major versions are
mostly equal and hence only one version number is needed.


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

2016-05-14 Thread Michael Banck
On Fri, May 13, 2016 at 05:31:00PM -0400, David G. Johnston wrote:
> The underlying premise, for me, of choosing .4 or .5  was that presently we
> discontinue support after 5 years/releases.  A new .0 would come out just
> as we discontinue the previous .0

Well maybe the 5 year support cycle would be nice to encode, but how is
.0 different from .1 then?  You make sound like .0 would be similar to
Ubuntu's LTS which is not the case, unless you want to propose that only
.0 releases get 5 years and not the in-betweens? That'd be a shame.

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] Academic help for Postgres

2016-05-12 Thread Michael Banck
On Thu, May 12, 2016 at 08:57:34AM +0800, Craig Ringer wrote:
> On 11 May 2016 at 22:20, Bruce Momjian  wrote:
> > I am giving a keynote at an IEEE database conference in Helsinki next
> > week (http://icde2016.fi/).  (Yes, I am not attending PGCon Ottawa
> > because I accepted the Helsinki conference invitation before the PGCon
> > Ottawa date was changed from June to May).
> >
> > As part of the keynote, I would like to mention areas where academia can
> > help us.  The topics I can think of are:
> >
> > Any others?
> >
> 
> When publishing work, publish source code somewhere stable that won't just
> vanish. And build on the latest stable release, don't build your prototype
> on Pg 8.0. Don't just publish a tarball with no information about what
> revision it's based on, publish a git tree or a patch series.
> 
> While academic prototype source is rarely usable directly, it can serve a
> valuable role with helping to understand the changes that were made,
> reproducing results, exploring further related work, etc
> 
> Include your dummy data or data generators, setup scripts, etc.

That is all sound advise, but if they do all of the above, then they
should also make sure the source (or parts of it) is potentially usable
by the project, i.e. (joint?) PGDG copyright, if their academic
institution allows that.


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] So, can we stop supporting Windows native now?

2016-04-01 Thread Michael Banck
On Thu, Mar 31, 2016 at 09:04:35AM +0800, Craig Ringer wrote:
> If we eventually get a CMake build system conversion that'll mostly go
> away too.

Well, maybe the good message about this new development is that
autotools will start working much better on Windows and could be
eventually used for the Windows port as well, making a CMake conversion
moot.


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

2016-02-02 Thread Michael Banck
On Tue, Feb 02, 2016 at 08:28:54AM -0800, Joshua D. Drake wrote:
> On 02/02/2016 07:31 AM, curtis.r...@gmail.com wrote:
> >Its not available in rpm or premade binary forms in its current
> >instantiation, not a big deal for me, but it raises the barrier to
> >entry.
> >
> >If it was packaged into an RPM, what would be the process to get it
> >added to postgresql's yum repositories?
> 
> Assuming it is not placed into core, we would need to work with the
> deb and rpm projects. Which I am sure we could (and would) do.

We are looking into packaging pgaudit for Debian.

However, then another question comes up: Should the 2nd Quadrant or the
Crunchy Data codebase be added to the distribution? Who gets to decide?

Alternatively, both could be added, but that will likely confuse users.


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


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

2016-02-02 Thread Michael Banck
On Tue, Feb 02, 2016 at 07:24:23AM -0800, Joshua D. Drake wrote:
> PostgreSQL has auditing. It is available now, just not in core. Postgis
> isn't available in core either and it seems to do just fine.

I don't really buy that argument. For one, PostGIS has a pretty narrow
functional use-case (spatial), while auditing is a horizontal use-case
that could be required for any kind of database usage.

Second, PostGIS had 10+ (?) years to build a reputation so that people
say "if I have to choose between PostGIS and buying Oracle Spatial, of
course I choose PostGIS", the pgaudit extension does not have that.

Auditing is a pretty security/enterprisey-related thing that could do
with the "officially considered to of the PostgreSQL project standard
and ready for production" rubber-stamp that tends to go along with most
end-user/admin-oriented stuff shipped in the tarball.  I am aware that
2nd Quadrant, Crunchy Data and EnterpriseDB (different codebase via
PPAS) all support their auditing extensions commercially, so that there
is certainly some form of credibility, but still.

Now, whether or not the currently submitted approach actually meets the
above rubber-stamp requirements is a different story, but at least I
think it would be quite useful to ship auditing capabilites in the
distribution.


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] No Issue Tracker - Say it Ain't So!

2015-09-30 Thread Michael Banck
On Tue, Sep 29, 2015 at 12:08:56PM +1300, Gavin Flower wrote:
> Linux kernel project uses bugzilla (https://bugzilla.kernel.org)

AIUI this is not mandatory for kernel hackers, and more opt-in from a
some/many/a few(?) subsystem maintainers.  Some parts use it more, some
less or not at all.

> and so does LibreOffice (https://bugs.documentfoundation.org)

Thas is true, however. 

Same for freedesktop.org and the Gnome project, I believe.


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] Disabling trust/ident authentication configure option

2015-05-20 Thread Michael Banck
On Wed, May 20, 2015 at 07:03:26PM -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Michael Banck wrote:
> >> The other set of users I could think of are those who, for whatever
> >> reason, tend to always compile PostgreSQL from source for their
> >> company/organization.  Maybe they have internal rules that requires a
> >> custom installation prefix for all their servers or whatever. Due to
> >> procedural requirements, or just the unwillingness to carry deltas, they
> >> absolutely want to use the pristine tarballs as well but would be very
> >> happy to get rid of some of the authentication methods.
> 
> > Right.  That's the set of users that Josh B says is only comprised of
> > Volker (the OP).
> 
> That might be a bit harsh, but here's the thing: assuming you're willing
> to build from source, what is the reason for wanting $small_market_feature
> to be built into Postgres rather than being something you carry a patch
> for?  ISTM the core reason is that you're expecting the community to carry
> the load of testing and maintaining the feature.  And the fact of the
> matter is that we're not terribly good at testing non-mainstream build
> options.  (There is depressingly little variety in the configure options
> used in the buildfarm, for example.)  So I wouldn't be a bit surprised
> if something like this broke every time somebody touched the auth code,
> and we would not notice.  It would only be reliable if it were something
> the community tended to use regularly ... which gets us back to the point
> that what needs to happen first is a credible replacement for "trust"
> mode.

Fair enough.

> I think Andres' point about "trust" being an essential disaster recovery
> mode is something to consider, as well.  That puts pretty strict limits
> on what would be a credible replacement.

Then let's rename it from `trust' to `disaster'... ;)


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] Disabling trust/ident authentication configure option

2015-05-20 Thread Michael Banck
On Wed, May 20, 2015 at 02:10:30PM -0400, Tom Lane wrote:
> One reason why it would not be, if it's a build-time decision,
> is that it's quite unlikely that any popular packagers would build
> that way.  So this would only be applicable to custom-built binaries,
> which is a pretty small class of users to begin with.

There might be appliance vendors who ship PostgreSQL along with their
product.  Then, they decide they want to use the pristine tarballs for
reproducibility and accountability.  If done right, they could publish
their set of configure options and a build-id or whatever, and 3rd 
parties could verify the binaries they ship have not been tampered
with[1].  Granted, they could also just publish the patch for those 3rd
parties to apply as well, but that sounds slightly inelegant.

The other set of users I could think of are those who, for whatever
reason, tend to always compile PostgreSQL from source for their
company/organization.  Maybe they have internal rules that requires a
custom installation prefix for all their servers or whatever. Due to
procedural requirements, or just the unwillingness to carry deltas, they
absolutely want to use the pristine tarballs as well but would be very
happy to get rid of some of the authentication methods.

That said, I agree that both examples are rather contrived and this is 
more of an advocatus diaboli kind of reply.


Michael

[1] seems like PostgreSQL is in the set of packages which successfully 
build reproducibly according to the Debian reproducible builds effort, 
yay


-- 
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] controlling psql's use of the pager a bit more

2014-11-13 Thread Michael Banck
On Thu, Nov 13, 2014 at 05:14:47PM +0100, Andres Freund wrote:
> On 2014-11-13 11:09:06 -0500, Tom Lane wrote:
> > Andrew Dunstan  writes:
> > > I often get annoyed because psql is a bit too aggressive when it decides 
> > > whether to put output through the pager, and the only way to avoid this 
> > > is to turn the pager off (in which case your next query might dump many 
> > > thousands of lines to the screen). I'd like a way to be able to specify 
> > > a minumum number of lines of output before psql would invoke the pager, 
> > > rather than just always using the terminal window size.
> > 
> > Are you saying you'd want to set the threshold to *more* than the window
> > height?  Why?
> 
> Not sure what that'd be useful for.
 
If the output is of the same order of magnitude as the window height, it
is still easy to compare the new output with whatever was above it by
conventional terminal scrolling (pgup, screen scrolling, whatever, if
present).  If a comparison is desired in the first place, that is.

If the output goes to the pager, you're stuck with it.  Unless of course
if the pager could somehow include the previous output up to some
threshold (while still being positioned at the beginning of the new
output in order to retain current behaviour), so one could scroll up in
the pager window.  That would rock.


Michael

-- 
Michael Banck
Tel.: +49 (0) 2161 / 4643-171 

credativ GmbH, HRB Mönchengladbach 12080 
Hohenzollernstr. 133, 41061 Mönchengladbach 
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] TODO request: log_long_transaction

2014-11-07 Thread Michael Banck
Hi,

Am Montag, den 27.10.2014, 19:29 + schrieb Thom Brown:
> On 27 October 2014 19:21, Josh Berkus  wrote:
> > I just realized that there is one thing we can't log currently:
> > transactions which last more than #ms.  This is valuable diagnostic
> > information when looking for issues like causes of bloat and deadlocks.
> >
> > I'd like it to be on the TODO list because it seems like part of a good
> > GSOC project or first-time contribution.
> 
> 
> So effectively, log_min_duration_transaction?  Sounds useful.

Questions are:

1. Should this log when the duration is exceeded (like log_lock_waits),
or on commit? I guess the latter, cause log_lock_waits is kinda an
offshoot from the deadlock detector, and other things don't work in a
similar fashion and/or this might be quite tricky and a non-starter.

2. It would be quite nice to log long-running idle-in-transaction (i.e.
transactions which have been idle for a long time, not necessarily long
transactions which are idle every now and then), but see 1.

3. Should long transactions which are rolled back be logged as well?

4. We log the statement when exceeding log_min_duration_statement, but
for transactions, that does not make a lot of sense, or should the last
statement be logged?  I don't think that would be particularly useful.

So if you just want to log transactions which took longer than
log_min_duration_transaction on commit (but not rollback), that's rather
easy and I've attached a PoC patch against master for that. 

I took the logic from check_log_duration(), so it is pretty trivial.  In
general, one could argue that tcop/postgres.c might be the better place,
and check_log_duration() should be refactored to support both
log_min_duration_statement and log_min_duration_transaction, but (i) I
decided to include the xid in the log message to have at least some
information (even though that might duplicate information in
log_line_prefix) which I don't think is easily accesible from tcop and
(ii) when I hooked it into finish_xact_command(), it did not work well,
e.g. it logged on psql statements like \d.

Thoughts?


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 6f92bad..da08c46 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -794,6 +794,46 @@ TransactionStartedDuringRecovery(void)
 }
 
 /*
+ *	CheckCurrentTransactionDuration
+ *
+ * Returns true if the current transaction's duration is longer than
+ * log_min_duration_transaction.
+ */
+bool
+CheckCurrentTransactionDuration(char *msec_str)
+{
+	long		secs;
+	int		usecs;
+	int		msecs;
+	bool		exceeded;
+	
+	TimestampDifference(xactStartTimestamp, 
+		xactStopTimestamp, 
+		&secs, 
+		&usecs);
+	msecs = usecs / 1000;
+
+	/*
+	 * This odd-looking test for log_min_duration_transaction being exceeded
+	 * is designed to avoid integer overflow with very long durations:
+	 * don't compute secs * 1000 until we've verified it will fit in int.
+	 */
+	exceeded = (log_min_duration_transaction == 0 ||
+(log_min_duration_transaction > 0 &&
+(secs > log_min_duration_transaction / 1000 ||
+secs * 1000 + msecs >= log_min_duration_transaction)));
+	
+	if (exceeded)
+	{
+		snprintf(msec_str, 32, "%ld.%03d",
+			secs * 1000 + msecs, usecs % 1000);
+		return true;
+	}
+	return false;
+}
+
+
+/*
  *	CommandCounterIncrement
  */
 void
@@ -1007,6 +1047,7 @@ RecordTransactionCommit(void)
 	SharedInvalidationMessage *invalMessages = NULL;
 	bool		RelcacheInitFileInval = false;
 	bool		wrote_xlog;
+	char		msec_str[32];
 
 	/* Get data needed for commit record */
 	nrels = smgrGetPendingDeletes(true, &rels);
@@ -1235,6 +1276,12 @@ RecordTransactionCommit(void)
 		END_CRIT_SECTION();
 	}
 
+	/* Check whether to log the duration of the transaction */
+	if (CheckCurrentTransactionDuration(msec_str))
+		ereport(LOG,
+			(errmsg("transaction %u duration: %s ms", xid, msec_str),
+			 errhidestmt(true)));
+
 	/* Compute latestXid while we have the child XIDs handy */
 	latestXid = TransactionIdLatest(xid, nchildren, children);
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index aca4243..6e8cc43 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -451,6 +451,7 @@ int			log_min_error_statement = ERROR;
 int			log_min_messages = WARNING;
 int			client_min_messages = NOTICE;
 int			log_min_duration_statement = -1;
+int			log_min_duration_transaction = -1;
 int			log_temp_files = -1;
 int			trace_recovery_messages = 

Re: [HACKERS] Let's drop two obsolete features which are bear-traps for novices

2014-11-03 Thread Michael Banck
Am Sonntag, den 02.11.2014, 12:41 -0500 schrieb Tom Lane:
> BTW, after reflecting a bit more I'm less than convinced that this
> datatype is completely useless.  Even if you prefer to store currency
> values in numeric columns, casting to or from money provides a way to
> accept or emit values in whatever monetary format the LC_MONETARY locale
> setting specifies.  That seems like a useful feature, and it's one you
> could not easily duplicate using to_char/to_number (not to mention that
> those functions aren't without major shortcomings of their own).

As an additional datapoint, Vitesse Data changed the DB schema from
NUMERIC to MONEY for their TPCH benchmark for performance reasons: "The
modification to data types is easy to understand -- money and double
types are faster than Numeric (and no one on this planet has a bank
account that overflows the money type, not any time soon)."[1] And
"Replaced NUMERIC fields representing currency with MONEY"[2].

Not sure whether they modified/optimized PostgreSQL with respect to the
MONEY data type and/or how much performance that gained, so CCing CK Tan
as well.


Michael

[1] 
http://vitesse-timing-on.blogspot.de/2014/10/running-tpch-on-postgresql-part-1.html
[2] http://vitessedata.com/benchmark/

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer



-- 
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] CREATE INDEX CONCURRENTLY?

2014-10-31 Thread Michael Banck
Am Freitag, den 31.10.2014, 14:43 + schrieb Greg Stark:
> On Fri, Oct 31, 2014 at 2:28 PM, Mark Woodward
>  wrote:
> > I have not kept up with PostgreSQL changes and have just been using it. A
> > co-worker recently told me that you need to word "CONCURRENTLY" in "CREATE
> > INDEX" to avoid table locking. I called BS on this because to my knowledge
> > PostgreSQL does not lock tables. I referenced this page in the
> > documentation:
> 
> You can read from tables while a normal index build is in progress but
> you can't insert, update, or delete from them. CREATE INDEX
> CONCURRENTLY allows you to insert, update, and delete data while the
> index build is running at the expense of having the index build take
> longer.

I believe there is one caveat: If there is an idle-in-transaction
backend from before the start of CREATE INDEX CONCURRENTLY, it can hold
up the index creation indefinitely as long as it doesn't commit.

src/backend/access/heap/README.HOT mentions this WRT CIC: "Then we wait
until every transaction that could have a snapshot older than the second
reference snapshot is finished.  This ensures that nobody is alive any
longer who could need to see any tuples that might be missing from the
index, as well as ensuring that no one can see any inconsistent rows in
a broken HOT chain (the first condition is stronger than the second)."

I have seen CIC stall at clients when there were (seemlingy) unrelated
idle-in-transactions open (their locks even touching only other
schemas). I believe it depends on the specific locks that the other
backend acquired, but at least with a DECLARE CURSOR I can trivially
reproduce it:

first session:

postgres=# CREATE SCHEMA foo1;
CREATE SCHEMA
postgres=# CREATE TABLE foo1.foo1 (id int);
CREATE TABLE
postgres=# CREATE SCHEMA foo2;
CREATE SCHEMA
postgres=# CREATE TABLE foo2.foo2 (id int);
CREATE TABLE

second session:

postgres=# BEGIN; DECLARE c1 CURSOR FOR SELECT * FROM foo1.foo1;
BEGIN
DECLARE CURSOR

first session:

postgres=# CREATE INDEX CONCURRENTLY ixfoo2 ON foo2.foo2(id);
(hangs)

I wonder whether that is pilot error (fair enough), or whether something
could be done about this?


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer



-- 
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] Log notice that checkpoint is to be written on shutdown

2014-10-16 Thread Michael Banck
Hi,

Am Donnerstag, den 02.10.2014, 15:21 +0200 schrieb Michael Banck:
> we have seen repeatedly that users can be confused about why PostgreSQL
> is not shutting down even though they requested it.  Usually, this is
> because `log_checkpoints' is not enabled and the final checkpoint is
> being written, delaying shutdown. As no message besides "shutting down"
> is written to the server log in this case, we even had users believing
> the server was hanging and pondering killing it manually.

There were some comments that this might not actually be the case and/or
that the postmaster was simply waiting for clients to disconnect due to
smart shutdown being invoked.

About the former, it might be possible to hook into the checkpoint code
and try to estimate how much I/O is to be written, and decide on some
threshold above which a warning is issued, but this looks rather fragile
so I won't try to tackle it now.

About the latter, this is a valid concern, and I decided to add a
WARNING in this case (if normal clients are connected), thus moving into
the "additional logging on shutdown" territory.

The other point was changing the default shutdown method and/or the
default for the log_checkpoints parameter.  The former has not been
discussed much and the latter was contentious - I won't touch those
either.  And even if the defaults are changed, old installations might
still carry the old defaults or DBAs might change them for whatever
reasons, so ISTM additional logging is rather orthogonal to that.

Finally, I am not convinced changing the pg_ctl logging is worthwhile.

I have updated the initial patch with the following: (i) the message got
changed to mention that this is a shutdown checkpoint, (ii) the end of
the shutdown checkpoint is logged as well and (iii) if normal clients
are connected during smart shutdown, a warning is logged.

I'll attach it to the next commitfest and see whether anybody likes it.


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5a4dbb9..e6f03fe 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8085,10 +8085,14 @@ CreateCheckPoint(int flags)
 
 	/*
 	 * If enabled, log checkpoint start.  We postpone this until now so as not
-	 * to log anything if we decided to skip the checkpoint.
+	 * to log anything if we decided to skip the checkpoint.  If we are during
+	 * shutdown and checkpoints are not being logged, add a log message that a 
+	 * checkpoint is to be written and shutdown is potentially delayed.
 	 */
 	if (log_checkpoints)
 		LogCheckpointStart(flags, false);
+	else if (shutdown)
+		ereport(LOG, (errmsg("waiting for shutdown checkpoint ...")));
 
 	TRACE_POSTGRESQL_CHECKPOINT_START(flags);
 
@@ -8311,6 +8315,12 @@ CreateCheckPoint(int flags)
 	/* Real work is done, but log and update stats before releasing lock. */
 	LogCheckpointEnd(false);
 
+	/* On shutdown, log a note about the completed shutdown checkpoint even
+	 * if log_checkpoints is off. 
+	 */
+	if (!log_checkpoints && shutdown)
+		ereport(LOG, (errmsg("shutdown checkpoint completed")));
+
 	TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
 	 NBuffers,
 	 CheckpointStats.ckpt_segs_added,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ce920ab..99c8911 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2405,6 +2405,11 @@ pmdie(SIGNAL_ARGS)
 if (WalWriterPID != 0)
 	signal_child(WalWriterPID, SIGTERM);
 
+/* check whether normal children are connected and log a warning if so */
+if (CountChildren(BACKEND_TYPE_NORMAL) != 0)
+	ereport(WARNING,
+	(errmsg("waiting for clients to disconnect ... ")));
+
 /*
  * If we're in recovery, we can't kill the startup process
  * right away, because at present doing so does not release

-- 
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] Log notice that checkpoint is to be written on shutdown

2014-10-08 Thread Michael Banck
Hi,

Am Samstag, den 04.10.2014, 15:05 -0500 schrieb Jim Nasby:
> On 10/4/14, 1:21 PM, Jeff Janes wrote:
> > On Thu, Oct 2, 2014 at 6:21 AM, Michael Banck wrote:
> > we have seen repeatedly that users can be confused about why PostgreSQL
> > is not shutting down even though they requested it. Usually, this is
> > because `log_checkpoints' is not enabled and the final checkpoint is
> > being written, delaying shutdown. As no message besides "shutting down"
> > is written to the server log in this case, we even had users believing
> > the server was hanging and pondering killing it manually.
> >
> >
>> Wouldn't a better place to write this message be the terminal from 
>> which "pg_ctl stop" was invoked, rather than the server log file?

Looking at it from a DBA perspective, this would indeed be better, yes.

However, I see a few issues with that:

1. If you are using an init script (or another wrapper around pg_ctl),
you don't get any of its output it seems.

2. Having taken a quick look at pg_ctl, it seems to just kill the
postmaster on shutdown and wait for its PID file to disappear.  I don't
see how it should figure out that PostgreSQL is waiting for a checkpoint
to be finished?

> Or do both. I suspect elog( INFO, ... ) might do that.

That would imply that pg_ctl receives and writes out log messages
directed at clients, which I don't think is true?  Even if it was,
client_min_messages does not include an INFO level, and LOG is not being
logged to clients by default. So the first common level above the
default of both client_min_messages and log_min_messages would be
WARNING, which seems excessive to me.

As I said, I only took a quick look at pg_ctl though, so I might well be
missing something.


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer



-- 
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] Log notice that checkpoint is to be written on shutdown

2014-10-04 Thread Michael Banck
Am Freitag, den 03.10.2014, 12:07 -0300 schrieb Alvaro Herrera:
> Michael Banck wrote:
> > diff --git a/src/backend/access/transam/xlog.c 
> > b/src/backend/access/transam/xlog.c
> > index 5a4dbb9..f2716ae 100644
> > --- a/src/backend/access/transam/xlog.c
> > +++ b/src/backend/access/transam/xlog.c
> > @@ -8085,10 +8085,14 @@ CreateCheckPoint(int flags)
> >  
> > /*
> >  * If enabled, log checkpoint start.  We postpone this until now so as 
> > not
> > -* to log anything if we decided to skip the checkpoint.
> > +* to log anything if we decided to skip the checkpoint.  If we are 
> > during
> > +* shutdown and checkpoints are not being logged, add a log message 
> > that a 
> > +* checkpoint is to be written and shutdown is potentially delayed.
> >  */
> > if (log_checkpoints)
> > LogCheckpointStart(flags, false);
> > +   else if (shutdown)
> > +   ereport(LOG, (errmsg("waiting for checkpoint ...")));
> >  
> > TRACE_POSTGRESQL_CHECKPOINT_START(flags);
> 
> I think if we're going to emit log messages for shutdown checkpoints, we
> ought to use the same format we already have, i.e. instead of having the
> separate "waiting for checkpoint" message, just test "log_checkpoints ||
> shutdown", then LogCheckpointStart.  

I considered this at first, but the rationale is that if somebody sets
log_checkpoints = off, they probably don't want the actual checkpoint to
be logged, so just a note that a checkpoint is happening was better
(Christoph Berg pointed this out).  If there is consensus that forcing
this one-time checkpoint logging is fine, I can change the patch
accordingly.

> And for consistency also make sure
> that the checkpoint end log line is also reported on shutdown
> checkpoints regardless of log_checkpoints.

I did this in an earlier draft of the patch, but AIUI this either
requires some refactoring to not evaluate log_checkpoints inside
LogCheckpointEnd(), and/or to change the function signature of
LogCheckpointEnd() to include either the `flags' bitfield or the
`shutdown' bool in order to force logging of the finished checkpoint
even if log_checkpoints is set to `off'.


Cheers,

Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer



-- 
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] Log notice that checkpoint is to be written on shutdown

2014-10-02 Thread Michael Banck
Hi,

Am Donnerstag, den 02.10.2014, 08:17 -0700 schrieb David G Johnston:
> Michael Banck-2 wrote 
> > I've attached a trivial patch for this, should it be added to the next
> > commitfest?
> 
> Peeking at this provokes a couple of novice questions:
> 
> While apparently it is impossible to have a checkpoint to log at recovery
> end the decision to use the bitwise-OR instead of the local "shutdown" bool
> seemed odd at first...

Good point, using `shutdown' is probably better.  I guess that local
bool had escaped my notice when I first had a look at this a while ago.

> LogCheckpointStart creates the log entry unconditionally - leaving the
> caller responsible for evaluating log_checkpoints - while LogCheckpointEnd
> has the log_checkpoints condition built into it. 

I noticed this as well.  My guess would be that this is because
LogCheckpointEnd() also updates the BgWriterStats statistics, and should
do that every time, not just when the checkpoint is getting logged.
Whether or not log_checkpoint is set (and logging should happen) is
evaluated directly after that. 

Some refactoring of this might be useful (or there might be a very good
reason why this was done like this), but this seems outside the scope of
this patch.  AIUI, shutdown will not be further delayed after the
checkpoint is finished, so no further logging is required for the
purpose of this patch.

> The rationale makes perfect sense.  +1

Cool!

I have attached an updated patch using the `shutdown' bool.


Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5a4dbb9..f2716ae 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8085,10 +8085,14 @@ CreateCheckPoint(int flags)
 
 	/*
 	 * If enabled, log checkpoint start.  We postpone this until now so as not
-	 * to log anything if we decided to skip the checkpoint.
+	 * to log anything if we decided to skip the checkpoint.  If we are during
+	 * shutdown and checkpoints are not being logged, add a log message that a 
+	 * checkpoint is to be written and shutdown is potentially delayed.
 	 */
 	if (log_checkpoints)
 		LogCheckpointStart(flags, false);
+	else if (shutdown)
+		ereport(LOG, (errmsg("waiting for checkpoint ...")));
 
 	TRACE_POSTGRESQL_CHECKPOINT_START(flags);
 

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


[HACKERS] Log notice that checkpoint is to be written on shutdown

2014-10-02 Thread Michael Banck
Hi,

we have seen repeatedly that users can be confused about why PostgreSQL
is not shutting down even though they requested it.  Usually, this is
because `log_checkpoints' is not enabled and the final checkpoint is
being written, delaying shutdown. As no message besides "shutting down"
is written to the server log in this case, we even had users believing
the server was hanging and pondering killing it manually.

In order to alert those users that a checkpoint is being written, I
propose to add a log message "waiting for checkpoint ..." on shutdown,
even if log_checkpoints is disabled, as this particular checkpoint might
be important information.

I've attached a trivial patch for this, should it be added to the next
commitfest?


Cheers,

Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5a4dbb9..78483ca 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8085,10 +8085,14 @@ CreateCheckPoint(int flags)
 
 	/*
 	 * If enabled, log checkpoint start.  We postpone this until now so as not
-	 * to log anything if we decided to skip the checkpoint.
+	 * to log anything if we decided to skip the checkpoint.  If we are during
+	 * shutdown and checkpoints are not being logged, add a log message that a 
+	 * checkpoint is to be written and shutdown is potentially delayed.
 	 */
 	if (log_checkpoints)
 		LogCheckpointStart(flags, false);
+	else if (flags & CHECKPOINT_IS_SHUTDOWN)
+		ereport(LOG, (errmsg("waiting for checkpoint ...")));
 
 	TRACE_POSTGRESQL_CHECKPOINT_START(flags);
 

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


Re: [HACKERS] Proposal for updating src/timezone

2014-07-19 Thread Michael Banck
Hi,

On Sat, Jul 19, 2014 at 09:28:25AM -0400, John Cochran wrote:
> Agreed. Right now, I'm seeing about updating zic.c to match the IANA code
> combined with the modifications that postgres did to it. So far, it doesn't
> look like many functional changes have been done, but due to the use of
> pgindent, there's a LOT of cosmetic changes that add one heck of a lot of
> noise in determining the actual differences. 

Maybe if you pgindent the IANA code as well, you can more easily diff
the actual changes between the two, did you try that?


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] Is there a way to temporarily disable a index

2014-07-11 Thread Michael Banck
On Fri, Jul 11, 2014 at 11:07:21AM -0400, Tom Lane wrote:
> David G Johnston  writes:
> > Benedikt Grundmann wrote
> >> That is it possible to tell the planner that index is off limits
> >> i.e.
> >> don't ever generate a plan using it?
> 
> > Catalog hacking could work but not recommended (nor do I know the
> > proper
> > commands and limitations).  Do you need the database/table to accept
> > writes
> > during the testing period?
> 
> Hacking pg_index.indisvalid could work, given a reasonably recent PG.
> I would not try it in production until I'd tested it ;-)

I wonder whether this should be exposed at the SQL level?  Hacking
pg_index is left to superusers, but the creator of an index (or the
owner of the schema) might want to experiment with disabling indices
while debugging query plans as well.

Turns out this is already in the TODO, Steve Singer has requested this
(in particular, "ALTER TABLE ...  ENABLE|DISABLE INDEX ...") in
http://www.postgresql.org/message-id/87hbegz5ir@cbbrowne.afilias-int.info
(as linked to from the TODO wiki page), but the neighboring discussion
was mostly about FK constraints.

Thoughts?


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] docs: additional subsection for page-level locks in explicit-locking section

2014-07-02 Thread Michael Banck
Hi,

While reading through the Explicit Locking section of the manual today,
I felt the last paragraph of section 13.3.2. (Row-level Locks) might
merit its own subsection.  It talks about page-level locks as distinct
from table- and row-level locks.  Then again, it is just one paragraph,
so maybe this was deliberate and/or rejected before (though I couldn't
find prior discussion off-hand).  Proposed patch attached.


Cheers,

Michael

-- 
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax:  +49 (2161) 4643-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 12b7814..84501e0 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -1140,6 +1140,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
  will result in disk writes.
 
 
+   
+Page-level Locks
+  
 
  In addition to table and row locks, page-level share/exclusive locks are
  used to control read/write access to table pages in the shared buffer

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


Re: [HACKERS] Commitfest II CLosed

2013-10-23 Thread Michael Banck
On Mon, Oct 21, 2013 at 11:10:09AM -0400, Tom Lane wrote:
> Heikki Linnakangas  writes:
> > On 21.10.2013 16:15, Peter Eisentraut wrote:
> >> What is the alternative?
> 
> > If no-one really cares enough about a patch to review it, mark it as 
> > "rejected, because no-one but the patch author cares". Harsh, but that's 
> > effectively what pushing to the next commitfest means anyway.
> 
> Well, that could be the problem, but it's also possible that no one could
> get to it in the alloted CF timeframe.  Maybe the best-qualified reviewers
> were on vacation, or maybe there were just too many patches.  I could see
> bouncing a patch on this basis if it doesn't get touched for, say, two
> consecutive CFs.

Maybe it would help if patches which got punted from the last commitfest
without review were marked up in some way (red, bold) in the commitfest
app so reviewers are nudged to maybe consider picking them up first.


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] Bug tracker tool we need (was: Last gasp)

2012-04-16 Thread Michael Banck
On Mon, Apr 16, 2012 at 06:29:47PM +0200, Magnus Hagander wrote:
> FWIW, I think the closest thing we've found so far would be debbugs -
> which IIRC doesn't have any kind of reasonable database backend, which
> would be a strange choice for a project like ours :) And makes many
> things harder...

FWIW, Don Armstrong (the main debbugs hacker these days I believe)
recently posted a blog post about a more obscure feature of debuugs
(forcemerge), where he finishs with "so we can eventually keep a
postgresql database updated in addition to the flatfile database.":

http://www.donarmstrong.com/posts/debbugs_forcemerge/

I don't know whether this implies a full PostgreSQL backend or just a
helper DB for some part of the functionality, but it might be something
to keep watching.


Regards,

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] Debian readline/libedit breakage

2011-02-13 Thread Michael Banck
On Sun, Feb 13, 2011 at 12:56:03PM +0100, Dimitri Fontaine wrote:
> Magnus Hagander  writes:
> > Yes, since according to a comment somewhere the same issue will
> > bubble
> > into ubuntu soon. At this point, definitely 8.04 and 10.04, and
> > probably 10.10. If things can be easily automated, it would be great
> > if we could do *all* supported ubuntu, but doing LTS and the latest
> > one or two non-LTS releases.
> 
> Well, that needs I guess either several ubuntu VM or pbuilder support
> for ubuntu debootstraps, will check about that.

As pbuilder just runs debootstrap on --create and (Debian) debootstrap
supports the Ubuntu releases, this is not an issue.


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] Debian readline/libedit breakage

2011-02-11 Thread Michael Banck
On Thu, Feb 10, 2011 at 06:04:46PM -0500, Tom Lane wrote:
> Less narrow-minded interpretation of GPL requirements, perhaps.
> (And yes, we have real lawyers on staff considering these issues.)

Is their opinion public/can be made public?  This might possibly lead to
a re-evaluation of the situation by Debian.

> If Debian want to shoot themselves in the foot like that, we can't
> stop them

BTW, that change has been merged into Ubuntu and will be (as of now) in
the next Ubuntu release.


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] Spread checkpoint sync

2011-02-03 Thread Michael Banck
On Sat, Jan 15, 2011 at 05:47:24AM -0500, Greg Smith wrote:
> For example, the pre-release Squeeze numbers we're seeing are awful so
> far, but it's not really done yet either. 

Unfortunately, it does not look like Debian squeeze will change any more
(or has changed much since your post) at this point, except for maybe
further stable kernel updates.  

Which file system did you see those awful numbers on and could you maybe
go into some more detail?


Thanks,

Michael

-- 
 I did send an email to propose multithreading to
grub-devel on the first of april.
 Unfortunately everyone thought I was serious ;-)

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