Обсуждение: pg_receivexlog and replication slots

Is there a particular reason why pg_receivexlog only supports using
manually created slots but pg_recvlogical supports creating and
dropping them? Wouldn't it be good for consistency there?

I'm guessing it's related to not being exposed over the replication
protocol? We had a discussion earlier that I remember about being able
to use an "auto-drop" slot in pg_basebackup, but this would be
different - this would be about creating and dropping a regular
physical replication slot...

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

On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote:
> Is there a particular reason why pg_receivexlog only supports using
> manually created slots but pg_recvlogical supports creating and
> dropping them? Wouldn't it be good for consistency there?

I've added it to recvlogical because logical decoding isn't usable
without slots. I'm not sure what you'd want to create/drop a slot from
receivexlog for, but I'm not adverse to adding the capability.

> I'm guessing it's related to not being exposed over the replication
> protocol?

It's exposed:
create_replication_slot:        /* CREATE_REPLICATION_SLOT slot PHYSICAL */        K_CREATE_REPLICATION_SLOT IDENT
K_PHYSICAL

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote:
>> Is there a particular reason why pg_receivexlog only supports using
>> manually created slots but pg_recvlogical supports creating and
>> dropping them? Wouldn't it be good for consistency there?
>
> I've added it to recvlogical because logical decoding isn't usable
> without slots. I'm not sure what you'd want to create/drop a slot from
> receivexlog for, but I'm not adverse to adding the capability.

I was mostly thinking for consistency, really. Using slots with
pg_receivexlog makes quite a bit of sense, even though it's useful
without it. But having the ability to create and drop (with compatible
commandline arguments of course) them directly when you set it up
would certainly be more convenient.


>> I'm guessing it's related to not being exposed over the replication
>> protocol?
>
> It's exposed:
> create_replication_slot:
>                         /* CREATE_REPLICATION_SLOT slot PHYSICAL */
>                         K_CREATE_REPLICATION_SLOT IDENT K_PHYSICAL

Hmm. You know, it would help if I actually looked at a 9.4 version of
the file when I check for functions of this kind :) Thanks!

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

On 2014-07-11 11:18:58 +0200, Magnus Hagander wrote:
> On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote:
> >> Is there a particular reason why pg_receivexlog only supports using
> >> manually created slots but pg_recvlogical supports creating and
> >> dropping them? Wouldn't it be good for consistency there?
> >
> > I've added it to recvlogical because logical decoding isn't usable
> > without slots. I'm not sure what you'd want to create/drop a slot from
> > receivexlog for, but I'm not adverse to adding the capability.
> 
> I was mostly thinking for consistency, really. Using slots with
> pg_receivexlog makes quite a bit of sense, even though it's useful
> without it. But having the ability to create and drop (with compatible
> commandline arguments of course) them directly when you set it up
> would certainly be more convenient.

Ok. Do you plan to take care of it? If, I'd be fine with backpatching
it. I'm not likely to get to it right now :(

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Fri, Jul 11, 2014 at 6:23 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Ok. Do you plan to take care of it? If, I'd be fine with backpatching
> it. I'm not likely to get to it right now :(

Actually I came up with the same need as Magnus, but a bit later,
so... Attached is a patch to support physical slot creation and drop
in pg_receivexlog with the addition of new options --create and
--drop. It would be nice to have that in 9.4, but I will not the one
deciding that at the end :) Code has been refactored with what is
already available in pg_recvlogical for the slot creation and drop.

Regards,
--
Michael

> Actually I came up with the same need as Magnus, but a bit later, so...
> Attached is a patch to support physical slot creation and drop in
> pg_receivexlog with the addition of new options --create and --drop. It
> would be nice to have that in 9.4, but I will not the one deciding that
> at the end :) Code has been refactored with what is already available
> in pg_recvlogical for the slot creation and drop.

I think that create/drop options of a slot is unnecessary. 
It is not different from performing pg_create_physical_replication_slot() by psql. 

At consistency with pg_recvlogical, do you think about --start?

Initial review.

The patch was not able to be applied to the source file at this morning time. 

commit-id:5ff5bfb5f0d83a538766903275b230499fa9ebe1

[postgres postgresql-5ff5bfb]$ patch -p1 < 20140812_physical_slot_receivexlog.patch
patching file doc/src/sgml/ref/pg_receivexlog.sgml
patching file src/bin/pg_basebackup/pg_receivexlog.c
Hunk #2 FAILED at 80.
1 out of 7 hunks FAILED -- saving rejects to file src/bin/pg_basebackup/pg_receivexlog.c.rej
patching file src/bin/pg_basebackup/pg_recvlogical.c
patching file src/bin/pg_basebackup/streamutil.c
patching file src/bin/pg_basebackup/streamutil.h

The patch was applied to the source file before August 12. 
warning comes out then make.

commit-id:6aa61580e08d58909b2a8845a4087b7699335ee0

[postgres postgresql-6aa6158]$ make > /dev/null
streamutil.c: In function ‘CreateReplicationSlot’:
streamutil.c:244: warning: suggest parentheses around ‘&&’ within ‘||’

Regards,

--
Furuya Osamu

Thanks for your review.

On Fri, Aug 15, 2014 at 12:56 AM,  <furuyao@pm.nttdata.co.jp> wrote:
> At consistency with pg_recvlogical, do you think about --start?
I did not add that for the sake of backward-compatibility as in
pg_recvlogical an action is mandatory. It is not the case now of
pg_receivexlog.

> [postgres postgresql-6aa6158]$ make > /dev/null
> streamutil.c: In function 'CreateReplicationSlot':
> streamutil.c:244: warning: suggest parentheses around '&&' within '||'
I see. Here is a rebased patch.
Regards,
--
Michael

On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Thanks for your review.
>
> On Fri, Aug 15, 2014 at 12:56 AM,  <furuyao@pm.nttdata.co.jp> wrote:
>> At consistency with pg_recvlogical, do you think about --start?
> I did not add that for the sake of backward-compatibility as in
> pg_recvlogical an action is mandatory. It is not the case now of
> pg_receivexlog.
>
>> [postgres postgresql-6aa6158]$ make > /dev/null
>> streamutil.c: In function 'CreateReplicationSlot':
>> streamutil.c:244: warning: suggest parentheses around '&&' within '||'
> I see. Here is a rebased patch.

Looking more at the code, IDENTIFY_SYSTEM management can be unified
into a single function. So I have written a newer version of the patch
grouping IDENTIFY_SYSTEM call into a single function for both
utilities, fixing at the same time a couple of other issues:
- correct use of TimelineID in code
- IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
check was done but in 9.4 this command returns 4 fields:
(PQntuples(res) != 1 || PQnfields(res) < 3)
That's not directly related to this patch, but making some corrections
is not going to hurt..

Regards,
--
Michael

On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Thanks for your review.
>>
>> On Fri, Aug 15, 2014 at 12:56 AM,  <furuyao@pm.nttdata.co.jp> wrote:
>>> At consistency with pg_recvlogical, do you think about --start?
>> I did not add that for the sake of backward-compatibility as in
>> pg_recvlogical an action is mandatory. It is not the case now of
>> pg_receivexlog.
>>
>>> [postgres postgresql-6aa6158]$ make > /dev/null
>>> streamutil.c: In function 'CreateReplicationSlot':
>>> streamutil.c:244: warning: suggest parentheses around '&&' within '||'
>> I see. Here is a rebased patch.
>
> Looking more at the code, IDENTIFY_SYSTEM management can be unified
> into a single function. So I have written a newer version of the patch
> grouping IDENTIFY_SYSTEM call into a single function for both
> utilities, fixing at the same time a couple of other issues:
> - correct use of TimelineID in code
> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
> check was done but in 9.4 this command returns 4 fields:
> (PQntuples(res) != 1 || PQnfields(res) < 3)
> That's not directly related to this patch, but making some corrections
> is not going to hurt..

Good catch! I found that libpqwalreceiver.c, etc have the same problem.
It's better to fix this separately. Patch attached.

Regards,

--
Fujii Masao

On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> Thanks for your review.
>>>
>>> On Fri, Aug 15, 2014 at 12:56 AM,  <furuyao@pm.nttdata.co.jp> wrote:
>>>> At consistency with pg_recvlogical, do you think about --start?
>>> I did not add that for the sake of backward-compatibility as in
>>> pg_recvlogical an action is mandatory. It is not the case now of
>>> pg_receivexlog.
>>>
>>>> [postgres postgresql-6aa6158]$ make > /dev/null
>>>> streamutil.c: In function 'CreateReplicationSlot':
>>>> streamutil.c:244: warning: suggest parentheses around '&&' within '||'
>>> I see. Here is a rebased patch.
>>
>> Looking more at the code, IDENTIFY_SYSTEM management can be unified
>> into a single function. So I have written a newer version of the patch
>> grouping IDENTIFY_SYSTEM call into a single function for both
>> utilities, fixing at the same time a couple of other issues:
>> - correct use of TimelineID in code
>> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
>> check was done but in 9.4 this command returns 4 fields:
>> (PQntuples(res) != 1 || PQnfields(res) < 3)
>> That's not directly related to this patch, but making some corrections
>> is not going to hurt..
>
> Good catch! I found that libpqwalreceiver.c, etc have the same problem.
> It's better to fix this separately. Patch attached.

BTW, the name of the forth column in IDENTIFY_SYSETM is "dbname". Maybe I don't
like this name because we usually use "datname" as the name of column which
identify the database name. For example, pg_database, pg_stat_activity, etc use
"datname".

Regards,

-- 
Fujii Masao

On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
>> check was done but in 9.4 this command returns 4 fields:
>> (PQntuples(res) != 1 || PQnfields(res) < 3)
>> That's not directly related to this patch, but making some corrections
>> is not going to hurt..
>
> Good catch! I found that libpqwalreceiver.c, etc have the same problem.
> It's better to fix this separately. Patch attached.
Patch looks good to me. Once you push that I'll rebase the stuff on
this thread once again, that's going to conflict for sure. And now
looking at your patch there is additional refactoring possible with
IDENTIFY_SYSTEM and pg_basebackup as well...
Regards,
-- 
Michael

On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
> And now looking at your patch there is additional refactoring possible
> with IDENTIFY_SYSTEM and pg_basebackup as well...
And attached is a rebased patch doing so.
Regards,
--
Michael

On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
>>> check was done but in 9.4 this command returns 4 fields:
>>> (PQntuples(res) != 1 || PQnfields(res) < 3)
>>> That's not directly related to this patch, but making some corrections
>>> is not going to hurt..
>>
>> Good catch! I found that libpqwalreceiver.c, etc have the same problem.
>> It's better to fix this separately. Patch attached.
> Patch looks good to me.

Okay, applied!

> Once you push that I'll rebase the stuff on
> this thread once again, that's going to conflict for sure. And now
> looking at your patch there is additional refactoring possible with
> IDENTIFY_SYSTEM and pg_basebackup as well...

Yep, that's possible. But since the patch needs to be back-patch to 9.4,
I didn't do the refactoring.

Regards,

-- 
Fujii Masao

On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
> check was done but in 9.4 this command returns 4 fields:
> (PQntuples(res) != 1 || PQnfields(res) < 3)

Which is correct. We don't want to error out in the case where 3 columns
are returned because that'd unnecessarily break compatibility with <
9.4. Previously that check was != 3...

This isn't a bug.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
>> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
>> check was done but in 9.4 this command returns 4 fields:
>> (PQntuples(res) != 1 || PQnfields(res) < 3)
>
> Which is correct. We don't want to error out in the case where 3 columns
> are returned because that'd unnecessarily break compatibility with <
> 9.4. Previously that check was != 3...
>
> This isn't a bug.

Okay, I understood why you didn't update those codes.

Since we don't allow replication between different major versions,
it's better to apply this change at least into libpqwalreceiver.c. Thought?

Regards,

-- 
Fujii Masao

On 2014-08-19 18:02:32 +0900, Fujii Masao wrote:
> On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
> >> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
> >> check was done but in 9.4 this command returns 4 fields:
> >> (PQntuples(res) != 1 || PQnfields(res) < 3)
> >
> > Which is correct. We don't want to error out in the case where 3 columns
> > are returned because that'd unnecessarily break compatibility with <
> > 9.4. Previously that check was != 3...
> >
> > This isn't a bug.
> 
> Okay, I understood why you didn't update those codes.
> 
> Since we don't allow replication between different major versions,
> it's better to apply this change at least into libpqwalreceiver.c. Thought?

We'd discussed that we'd rather keep it consistent. It also results in a
more explanatory error message lateron.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, Aug 19, 2014 at 6:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-08-19 18:02:32 +0900, Fujii Masao wrote:
>> On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
>> >> - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
>> >> check was done but in 9.4 this command returns 4 fields:
>> >> (PQntuples(res) != 1 || PQnfields(res) < 3)
>> >
>> > Which is correct. We don't want to error out in the case where 3 columns
>> > are returned because that'd unnecessarily break compatibility with <
>> > 9.4. Previously that check was != 3...
>> >
>> > This isn't a bug.
>>
>> Okay, I understood why you didn't update those codes.
>>
>> Since we don't allow replication between different major versions,
>> it's better to apply this change at least into libpqwalreceiver.c. Thought?
>
> We'd discussed that we'd rather keep it consistent. It also results in a
> more explanatory error message lateron.

Hmm... okay, will revert the commit.

Regards,

-- 
Fujii Masao

On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>>> <michael.paquier@gmail.com> wrote:
>> And now looking at your patch there is additional refactoring possible
>> with IDENTIFY_SYSTEM and pg_basebackup as well...
> And attached is a rebased patch doing so.
I reworked this patch a bit to take into account the fact that the
number of columns to check after running IDENTIFY_SYSTEM is not always
4 as pointed out by Andres:
- pg_basebackup => 3
- pg_receivexlog => 3
- pg_recvlogical => 4

Regards,
--
Michael

On Tue, Aug 26, 2014 at 9:43 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>>>> <michael.paquier@gmail.com> wrote:
>>> And now looking at your patch there is additional refactoring possible
>>> with IDENTIFY_SYSTEM and pg_basebackup as well...
>> And attached is a rebased patch doing so.
> I reworked this patch a bit to take into account the fact that the
> number of columns to check after running IDENTIFY_SYSTEM is not always
> 4 as pointed out by Andres:
> - pg_basebackup => 3
> - pg_receivexlog => 3
> - pg_recvlogical => 4

As this is a number of patches rolled into one - do you happen to keep
them separate in your local repo? If so can you send them as separate
ones (refactor identify_system should be quite unrelated to supporting
replication slots, right?), for easier review? (if not, I'll just
split them apart mentally, but it's easier to review separately)

On the identify_system part - my understanding of the code is that
what you pass in as num_cols is the number of columns required for it
to work, right? We probably need to adjust the error message as well
in that case, because it's no longer what's "expected", it's what's
"required"? And we might want to include a hint about the reason
(wrong version)?

There's also a note "get LSN start position if necessary", but it
tries to do it unconditionally. What is the "if necessary" supposed to
refer to?

Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
as it never actually looks at the 4th column anyway? If we do
specifically want it to fail in the case of pg_recvlogical, we really
need to think up a better error message for it, and perhaps a
different way of specifying it?

Do we really want those Asserts? There is not a single Assert in
bin/pg_basebackup today - as is the case for most things in bin/. We
typically use regular if statements for things that "can happen", and
just ignore the others I think - since the callers are fairly simple
to trace.


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

On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander <magnus@hagander.net> wrote:
> As this is a number of patches rolled into one - do you happen to keep
> them separate in your local repo? If so can you send them as separate
> ones (refactor identify_system should be quite unrelated to supporting
> replication slots, right?), for easier review? (if not, I'll just
> split them apart mentally, but it's easier to review separately)
Thanks for your review!

OK, here are 2 patches, the 2nd needing the 1st one:
1) Refactor IDENTIFY_SYSTEM and replslot create/drop APIs
2) Support for --create and --drop in pg_receivexlog

> On the identify_system part - my understanding of the code is that
> what you pass in as num_cols is the number of columns required for it
> to work, right?
The argument is I would say cross-version compatibility and
consistency with the existing 9.4 code, but... (see below for the rest
of the story).

> We probably need to adjust the error message as well
> in that case, because it's no longer what's "expected", it's what's
> "required"?
OK, changed this way.

> And we might want to include a hint about the reason (wrong version)?
I am not sure about that, a simple error message looks fine IMO, and
there is no notion of error hinting in the other client utilities as
well.

> There's also a note "get LSN start position if necessary", but it
> tries to do it unconditionally. What is the "if necessary" supposed to
> refer to?
That's remnant of some old code, so I removed it. Thanks for spotting that.

> Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
> as it never actually looks at the 4th column anyway? If we do
> specifically want it to fail in the case of pg_recvlogical, we really
> need to think up a better error message for it, and perhaps a
> different way of specifying it?
Hm. I'd vote to simplify the code a bit based on the argument that the
current API only looks at the 3 first columns and does not care about
the 4th which is the plugin name.

> Do we really want those Asserts? There is not a single Assert in
> bin/pg_basebackup today - as is the case for most things in bin/. We
> typically use regular if statements for things that "can happen", and
> just ignore the others I think - since the callers are fairly simple
> to trace.
OK, removed.

Regards,
--
Michael

On 2014-09-01 20:58:29 +0900, Michael Paquier wrote:
> On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander <magnus@hagander.net> wrote:
> > As this is a number of patches rolled into one - do you happen to keep
> > them separate in your local repo? If so can you send them as separate
> > ones (refactor identify_system should be quite unrelated to supporting
> > replication slots, right?), for easier review? (if not, I'll just
> > split them apart mentally, but it's easier to review separately)
> Thanks for your review!
> 
> OK, here are 2 patches, the 2nd needing the 1st one:
> 1) Refactor IDENTIFY_SYSTEM and replslot create/drop APIs
> 2) Support for --create and --drop in pg_receivexlog
> 
> > On the identify_system part - my understanding of the code is that
> > what you pass in as num_cols is the number of columns required for it
> > to work, right?
> The argument is I would say cross-version compatibility and
> consistency with the existing 9.4 code, but... (see below for the rest
> of the story).

I don't really see a need to that for slot specific code. The locations
where we more lenient are ones where < 9.4 is ok, or a better message is
following shortly afterwards.

> > We probably need to adjust the error message as well
> > in that case, because it's no longer what's "expected", it's what's
> > "required"?
> OK, changed this way.

The reason for the formulation of the current error message is that it's
the same across all callsites emitting it to make it easier for
translators. It used to be more specific at some point and then was
changed. Since these aren't expected to be hit much I don't really see a
need to be very detailed.

> > And we might want to include a hint about the reason (wrong version)?
> I am not sure about that, a simple error message looks fine IMO, and
> there is no notion of error hinting in the other client utilities as
> well.

Agreed.

> > Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
> > as it never actually looks at the 4th column anyway? If we do
> > specifically want it to fail in the case of pg_recvlogical, we really
> > need to think up a better error message for it, and perhaps a
> > different way of specifying it?

> Hm. I'd vote to simplify the code a bit based on the argument that the
> current API only looks at the 3 first columns and does not care about
> the 4th which is the plugin name.

Why not return all four columns from RunIdentifySystem(), setting plugin
to NULL if not available. Then the caller can error out.

Greetings,

Andres Freund

On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Do we really want those Asserts? There is not a single Assert in
> bin/pg_basebackup today - as is the case for most things in bin/. We
> typically use regular if statements for things that "can happen", and
> just ignore the others I think - since the callers are fairly simple
> to trace.

I have no opinion on whether we want these particular Assert() calls,
but I note that using Assert() in front-end code only became possible
in February of 2013, as a result of commit
e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9.  So the lack of assertions
there may not be so much because people thought it was a bad idea as
that it didn't use to work.  Generally, I favor the use of Assert() in
front-end code in the same scenarios in which we use it in back-end
code: for checks that shouldn't burden production builds but are
useful during development.

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

On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> Do we really want those Asserts? There is not a single Assert in
>> bin/pg_basebackup today - as is the case for most things in bin/. We
>> typically use regular if statements for things that "can happen", and
>> just ignore the others I think - since the callers are fairly simple
>> to trace.
>
> I have no opinion on whether we want these particular Assert() calls,
> but I note that using Assert() in front-end code only became possible
> in February of 2013, as a result of commit
> e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9.  So the lack of assertions
> there may not be so much because people thought it was a bad idea as
> that it didn't use to work.  Generally, I favor the use of Assert() in
> front-end code in the same scenarios in which we use it in back-end
> code: for checks that shouldn't burden production builds but are
> useful during development.

Well that was exactly why they have been added first. The assertions
have been placed in some functions to check for incompatible
combinations of argument values when those functions are called. I
don't mind re-adding them if people agree that they make sense. IMO
they do and they help as well for the development of future utilities
using replication protocol in src/bin/pg_basebackup as much as the
refactoring work done on this thread.
Regards,
-- 
Michael

On Mon, Sep 8, 2014 at 8:50 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander <magnus@hagander.net> wrote:
>>> Do we really want those Asserts? There is not a single Assert in
>>> bin/pg_basebackup today - as is the case for most things in bin/. We
>>> typically use regular if statements for things that "can happen", and
>>> just ignore the others I think - since the callers are fairly simple
>>> to trace.
>>
>> I have no opinion on whether we want these particular Assert() calls,
>> but I note that using Assert() in front-end code only became possible
>> in February of 2013, as a result of commit
>> e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9.  So the lack of assertions
>> there may not be so much because people thought it was a bad idea as
>> that it didn't use to work.  Generally, I favor the use of Assert() in
>> front-end code in the same scenarios in which we use it in back-end
>> code: for checks that shouldn't burden production builds but are
>> useful during development.
>
> Well that was exactly why they have been added first. The assertions
> have been placed in some functions to check for incompatible
> combinations of argument values when those functions are called. I
> don't mind re-adding them if people agree that they make sense. IMO
> they do and they help as well for the development of future utilities
> using replication protocol in src/bin/pg_basebackup as much as the
> refactoring work done on this thread.
Let's keep them then.

>>> We probably need to adjust the error message as well
>>> in that case, because it's no longer what's "expected", it's what's
>>> "required"?
>> OK, changed this way.
> The reason for the formulation of the current error message is that it's
> the same across all callsites emitting it to make it easier for
> translators. It used to be more specific at some point and then was
> changed. Since these aren't expected to be hit much I don't really see a
> need to be very detailed.
Re-changed back to that.

>> Hm. I'd vote to simplify the code a bit based on the argument that the
>> current API only looks at the 3 first columns and does not care about
>> the 4th which is the plugin name.
> Why not return all four columns from RunIdentifySystem(), setting plugin
> to NULL if not available. Then the caller can error out.
This seems the cleaner approach, do did so.

New patches taking into account all those comments are attached.
Thanks for the feedback.
Regards,
--
Michael

On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I have looked into refactoring related patch and would like
> to share my observations with you:
Thanks! Useful input is always welcome.

> 1.
> + * Run IDENTIFY_SYSTEM through a given connection and give back to caller
> This API also gets plugin if requested, so I think it is better
> to mention the same in function header as well.
True.

> 2.
> + /* Get LSN start position if necessary */
> Isn't it better if we try to get the LSN position only incase
> startpos!=NULL (as BaseBackup don't need this)
OK. Let's do that for consistency with the other fields.

> 3.
> I find existing comments okay, is there a need to change
> in this case?  Part of the new comment mentions
> "obtaining start LSN position", actually the start position is
> identified as part of next function call FindStreamingStart(),
> only incase it return InvalidXLogRecPtr, the value returned
> by IDENTIFY_SYSTEM will be used.
Hopefully I am following you correctly here: comment has been updated
to mention that the start LSN and TLI fetched from IDENTIFY_SYSTEM are
always fetched but used only if no valid position is found in output
folder of pg_receivexlog.

> 4.
> + /* Obtain a connection before doing anything */
> + conn = GetConnection();
> + if (!conn)
> + /* Error message already written in GetConnection() */
> Is there a reason for moving this function out of StreamLog(),
> there is no harm in moving it, but there doesn't seem to be any
> problem even if it is called inside StreamLog().
For pg_recvlogical, this move is done to reduce code redundancy, and
actually it makes sense to just grab one connection in the main() code
path before performing any replication commands. For pg_receivexlog,
the move is done because it makes the code more consistent with
pg_recvlogical, and also it is a preparatory work for the second patch
that introduces the create/drop slot. Even if 2nd patch is not
accepted I figured out that it would not hurt to simply grab one
connection in the main code path before doing any action.

> 5.
> Shouldn't there be Assert instead of if (conn == NULL), as
> RunIdentifySystem() is not expected to be called without connection.
Fine for me. That's indeed more consistent with the rest this way.

> 6.
> + /* Identify system, obtaining start LSN position at the same time */
> + if (!RunIdentifySystem(conn,
> NULL, NULL, &startpos, &plugin_name))
> + disconnect_and_exit(1);
> a.
> Generally IdentifySystem is called as first API, but I think you
> have changed its location after CreateReplicationSlot as you want
> to double check the value of plugin, not sure if that is necessary or
> important enough that we start calling it later.
Funny part here is that even the current code on master and
REL9_4_STABLE of pg_recvlogical.c is fetching a start position when
creating a replication slot that is not used as two different actions
cannot be used at the same time. That's a matter of removing this line
in code though:
startpos = ((uint64) hi) << 32 | lo;
As that's only cosmetic for 9.4, and this patch makes things more
correct I guess that's fine to do nothing and just try to get this
patch in.

> b.
> Above call is trying to get startpos, won't it overwrite the value
> passed by user (case 'I':) for startpos?
Yep, that's a bug. The value that user could give would have been
overwritten all the time. Current code does not even care about
checking startpos in IDENTIFY_SYSTEM so we're fine by just removing
this part.

Updated patches are attached.
--
Michael

On Mon, Sep 22, 2014 at 2:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Sep 20, 2014 at 10:06 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> > 3.
>> > I find existing comments okay, is there a need to change
>> > in this case?  Part of the new comment mentions
>> > "obtaining start LSN position", actually the start position is
>> > identified as part of next function call FindStreamingStart(),
>> > only incase it return InvalidXLogRecPtr, the value returned
>> > by IDENTIFY_SYSTEM will be used.
>> Hopefully I am following you correctly here: comment has been updated
>> to mention that the start LSN and TLI fetched from IDENTIFY_SYSTEM are
>> always fetched but used only if no valid position is found in output
>> folder of pg_receivexlog.
>
> Updated comment is consistent with code, however my main point
> was that in this case, I don't see much need to change existing
> comment.  I think this point is more related to each individual's
> perspective, so if you think there is a need to update the existing
> comment, then retain as it is in your patch and let Committer
> take a call about it.
Well, let's use your suggestion then and switch back to the former comment then.

>> > 4.
>> > + /* Obtain a connection before doing anything */
>> > + conn = GetConnection();
>> > + if (!conn)
>> > + /* Error message already written in GetConnection() */
>> > Is there a reason for moving this function out of StreamLog(),
>> > there is no harm in moving it, but there doesn't seem to be any
>> > problem even if it is called inside StreamLog().
>> For pg_recvlogical, this move is done to reduce code redundancy, and
>> actually it makes sense to just grab one connection in the main() code
>> path before performing any replication commands. For pg_receivexlog,
>> the move is done because it makes the code more consistent with
>> pg_recvlogical, and also it is a preparatory work for the second patch
>> that introduces the create/drop slot. Even if 2nd patch is not
>> accepted I figured out that it would not hurt to simply grab one
>> connection in the main code path before doing any action.
>
> In pg_receivexlog, StreamLog() calls PQfinish() to close a connection
> to the backend and StreamLog() is getting called in while(true) loop,
> so if you just grab the connection once in main loop, the current
> logic won't work.  For same reasons, the current coding related to
> GetConnection() in pg_receivelogical seems to be right, so it is better
> not to change that as well.  For the second patch (that introduces the
> create/drop slot),  I think it is better to do in a way similar to what
> current pg_receivelogical does.
OK let's do so then. I changed back the GetConnection stuff back to
what is done on master. In the second patch, I added an extra call to
GetConnection before the create/drop commands.

>> > 6.
>> > + /* Identify system, obtaining start LSN position at the same time */
>> > + if (!RunIdentifySystem(conn,
>> > NULL, NULL, &startpos, &plugin_name))
>> > + disconnect_and_exit(1);
>> > a.
>> > Generally IdentifySystem is called as first API, but I think you
>> > have changed its location after CreateReplicationSlot as you want
>> > to double check the value of plugin, not sure if that is necessary or
>> > important enough that we start calling it later.
>> Funny part here is that even the current code on master and
>> REL9_4_STABLE of pg_recvlogical.c is fetching a start position when
>> creating a replication slot that is not used as two different actions
>> cannot be used at the same time. That's a matter of removing this line
>> in code though:
>> startpos = ((uint64) hi) << 32 | lo;
>
> User is not allowed to give startpos with drop or create of replication
> slot.  It is prevented by check:
> if (startpos != InvalidXLogRecPtr && (do_create_slot || do_drop_slot))
> So it seems you cannot remove the startpos assignment in code.
Ah yes true, it is actually possible to start the replication process
after creating the slot, so better not to remove it... I have updated
CreateReplicationSlot to add startpos in the fields that can be
requested from results.

>> As that's only cosmetic for 9.4, and this patch makes things more
>> correct I guess that's fine to do nothing and just try to get this
>> patch in.
>
> In what sense do you think that this part of patch is better than
> current code?
I was trying to make the code a maximum consistent between the two
utilities, but your approach makes more sense.

> I think calling Identify_System as a first command makes sense
> (as it is in current code) because if that fails we should not
> proceed with execution of other command's.
Yes looking at that again you are right.

> Another point about refactoring patch is that fourth column in
> Identify_System is dbname and in patch, you are referring it as
> plugin which seems to be inconsistent.
Oops. Thanks for checking, I changed the check to have something for
the database name.

At the same time, I noticed an unnecessary limitation in the second
patch: we should be able to create a slot and stream from it directly.
Last version exited immediately after the creation, error or not.

Updated patches attached.
Regards,
--
Michael

On 2014-09-22 15:40:41 +0900, Michael Paquier wrote:
> Updated patches attached.

These are patches about pg_dump. I don't think these were the ones you
wanted to attach... :)

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Hi,

I've split and edited patch 0001:
0001) Old WIP patch for pg_recvlogical tests. Useful for easier development.
0002) Copy editing that should be in 9.4
0003) Rebased patches of yours
0004) My changes to 0003 besides the rebase. This'll be squashed, but I
      thought it might be interesting for you.

I haven't tested my edits besides running 0001...

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2014-10-01 21:54:56 +0900, Michael Paquier wrote:
> Thanks!
> 
> On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> 
> > 0001) Old WIP patch for pg_recvlogical tests. Useful for easier
> > development.
> >
> From where is this patch? Is it some rest from the time when pg_recvlogical
> was developed?
> 
> 
> > 0002) Copy editing that should be in 9.4
> >
> Definitely makes sense for a backpatch.
> 
> 
> > 0003) Rebased patches of yours
> >
> 0004) My changes to 0003 besides the rebase. This'll be squashed, but I
> >       thought it might be interesting for you.
> >
> (thanks for caring)
> -    /* Drop a replication slot */
> +    /* Drop a replication slot. */
> I don't recall that comments on a single line have a dot even if this
> single line is a complete sentence.

Then it shouldn't start with a uppercase - which you changed...

> -static void StreamLog();
> +static void StreamLogicalLog();
> Not related at all to those patches, but for correctness it is better to
> add a declaration "(void)".

Agreed.

> Except those small things the changes look good to me.

Cool. Will push and then, sometime this week, review the next patch.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund wrote:

> From d667f7a63cd62733d88ec5b7228dfd5d7136b9d7 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@otacoo.com>
> Date: Mon, 1 Sep 2014 20:48:43 +0900
> Subject: [PATCH 3/4] Refactoring of pg_basebackup utilities
> 
> Code duplication is reduced with the introduction of new APIs for each
> individual replication command:
> - IDENTIFY_SYSTEM
> - CREATE_REPLICATION_SLOT
> - DROP_REPLICATION_SLOT
> A couple of variables used to identify a timeline ID are changed as well
> to be more consistent with core code.
> ---
>  src/bin/pg_basebackup/pg_basebackup.c  |  21 +----
>  src/bin/pg_basebackup/pg_receivexlog.c |  38 ++------
>  src/bin/pg_basebackup/pg_recvlogical.c | 116 ++++++------------------
>  src/bin/pg_basebackup/streamutil.c     | 159 +++++++++++++++++++++++++++++++++
>  src/bin/pg_basebackup/streamutil.h     |  10 +++
>  5 files changed, 207 insertions(+), 137 deletions(-)

Not objecting to any of this, but isn't it a bit funny that a patch that
aims to reduce duplication ends up with more lines than there were
originally?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-10-01 11:21:12 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > From d667f7a63cd62733d88ec5b7228dfd5d7136b9d7 Mon Sep 17 00:00:00 2001
> > From: Michael Paquier <michael@otacoo.com>
> > Date: Mon, 1 Sep 2014 20:48:43 +0900
> > Subject: [PATCH 3/4] Refactoring of pg_basebackup utilities
> > 
> > Code duplication is reduced with the introduction of new APIs for each
> > individual replication command:
> > - IDENTIFY_SYSTEM
> > - CREATE_REPLICATION_SLOT
> > - DROP_REPLICATION_SLOT
> > A couple of variables used to identify a timeline ID are changed as well
> > to be more consistent with core code.
> > ---
> >  src/bin/pg_basebackup/pg_basebackup.c  |  21 +----
> >  src/bin/pg_basebackup/pg_receivexlog.c |  38 ++------
> >  src/bin/pg_basebackup/pg_recvlogical.c | 116 ++++++------------------
> >  src/bin/pg_basebackup/streamutil.c     | 159 +++++++++++++++++++++++++++++++++
> >  src/bin/pg_basebackup/streamutil.h     |  10 +++
> >  5 files changed, 207 insertions(+), 137 deletions(-)
> 
> Not objecting to any of this, but isn't it a bit funny that a patch that
> aims to reduce duplication ends up with more lines than there were
> originally?

The reduction is there in combination with a later patch - which needs
most of the code moved to streamutil.c.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Hi,

I pushed the first part.

On 2014-10-01 21:54:56 +0900, Michael Paquier wrote:
> On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> > 0001) Old WIP patch for pg_recvlogical tests. Useful for easier
> > development.
> From where is this patch? Is it some rest from the time when pg_recvlogical
> was developed?

IIRC I wrote it when looking at Peter's prove patches. There's a bit of
a problem with it because it requires test_decoding, a contrib module,
to be installed. That's easily solvable for 'make check', but less
clearly so for 'make installcheck'.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 10/01/2014 08:59 AM, Andres Freund wrote:
> On 2014-10-01 21:54:56 +0900, Michael Paquier wrote:
>> Thanks!
>>
>> On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund <andres@2ndquadrant.com>
>> wrote:
>>
>> > 0001) Old WIP patch for pg_recvlogical tests. Useful for easier
>> > development.
>> >
>> From where is this patch? Is it some rest from the time when pg_recvlogical
>> was developed?
>>
>>
>> > 0002) Copy editing that should be in 9.4
>> >
>> Definitely makes sense for a backpatch.
>>
>>
>> > 0003) Rebased patches of yours
>> >
>> 0004) My changes to 0003 besides the rebase. This'll be squashed, but I
>> >       thought it might be interesting for you.
>> >
>> (thanks for caring)
>> -    /* Drop a replication slot */
>> +    /* Drop a replication slot. */
>> I don't recall that comments on a single line have a dot even if this
>> single line is a complete sentence.
>
> Then it shouldn't start with a uppercase - which you changed...
>
>> -static void StreamLog();
>> +static void StreamLogicalLog();
>> Not related at all to those patches, but for correctness it is better to
>> add a declaration "(void)".
>
> Agreed.
>
>> Except those small things the changes look good to me.
>
> Cool. Will push and then, sometime this week, review the next patch.
>
> Greetings,
>
> Andres Freund
>


You might want to do that function renaming in pg_receivexlog.c as well.


Jan

-- 
Jan Wieck
Senior Software Engineer
http://slony.info

On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:
> >>-static void StreamLog();
> >>+static void StreamLogicalLog();
> >>Not related at all to those patches, but for correctness it is better to
> >>add a declaration "(void)".
> >
> >Agreed.
> >
> >>Except those small things the changes look good to me.
> >
> >Cool. Will push and then, sometime this week, review the next patch.

> You might want to do that function renaming in pg_receivexlog.c as well.

Why? You mean to StreamPhysicalLog()?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 10/01/2014 01:19 PM, Andres Freund wrote:
> On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:
>> >>-static void StreamLog();
>> >>+static void StreamLogicalLog();
>> >>Not related at all to those patches, but for correctness it is better to
>> >>add a declaration "(void)".
>> >
>> >Agreed.
>> >
>> >>Except those small things the changes look good to me.
>> >
>> >Cool. Will push and then, sometime this week, review the next patch.
>
>> You might want to do that function renaming in pg_receivexlog.c as well.
>
> Why? You mean to StreamPhysicalLog()?
>
> Greetings,
>
> Andres Freund
>

Like that.

Plus, there is one more occurrence of StreamLog() in pg_recvlogical.c.


Jan

-- 
Jan Wieck
Senior Software Engineer
http://slony.info

On 2014-10-01 13:22:53 -0400, Jan Wieck wrote:
> On 10/01/2014 01:19 PM, Andres Freund wrote:
> >On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:
> >>>>-static void StreamLog();
> >>>>+static void StreamLogicalLog();
> >>>>Not related at all to those patches, but for correctness it is better to
> >>>>add a declaration "(void)".
> >>>
> >>>Agreed.
> >>>
> >>>>Except those small things the changes look good to me.
> >>>
> >>>Cool. Will push and then, sometime this week, review the next patch.
> >
> >>You might want to do that function renaming in pg_receivexlog.c as well.
> >
> >Why? You mean to StreamPhysicalLog()?
> 
> Like that.

Don't see that much point - it just seemed wrong to have StreamLog do
two somewhat different things. And StreamLog() in receivexlog is much older...

> Plus, there is one more occurrence of StreamLog() in pg_recvlogical.c.

Gah. Wrongly split commit :(. Fixed in 9.4, it's already fixed by the
subsequent commit in master.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 10/01/2014 01:32 PM, Andres Freund wrote:
> On 2014-10-01 13:22:53 -0400, Jan Wieck wrote:
>> On 10/01/2014 01:19 PM, Andres Freund wrote:
>> >On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:
>> >>>>-static void StreamLog();
>> >>>>+static void StreamLogicalLog();
>> >>>>Not related at all to those patches, but for correctness it is better to
>> >>>>add a declaration "(void)".
>> >>>
>> >>>Agreed.
>> >>>
>> >>>>Except those small things the changes look good to me.
>> >>>
>> >>>Cool. Will push and then, sometime this week, review the next patch.
>> >
>> >>You might want to do that function renaming in pg_receivexlog.c as well.
>> >
>> >Why? You mean to StreamPhysicalLog()?
>>
>> Like that.
>
> Don't see that much point - it just seemed wrong to have StreamLog do
> two somewhat different things. And StreamLog() in receivexlog is much older...

The point is that StreamLog is semantically a superset of StreamWhateverLog.


Jan


>
>> Plus, there is one more occurrence of StreamLog() in pg_recvlogical.c.
>
> Gah. Wrongly split commit :(. Fixed in 9.4, it's already fixed by the
> subsequent commit in master.
>
> Greetings,
>
> Andres Freund
>


-- 
Jan Wieck
Senior Software Engineer
http://slony.info

On 2014-10-03 10:30:19 +0900, Michael Paquier wrote:
> On Thu, Oct 2, 2014 at 12:44 AM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> 
> > I pushed the first part.
> >
> Thanks. Attached is a rebased version of patch 2, implementing the actual
> feature. One thing I noticed with more testing is that if --create is used
> and that the destination folder does not exist, pg_receivexlog was creating
> the slot, and left with an error. This does not look user-friendly so I
> changed the logic a bit to check for the destination folder before creating
> any slot. This results in a bit of refactoring, but this way behavior is
> more intuitive.

Ok.

>     <para>
> +    <application>pg_receivexlog</application> can run in one of two following
> +    modes, which control physical replication slot:

I don't think that's good enough. There's also the important mode where
it's not doing --create/--drop at all.


> +    /*
> +     * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
> +     * position.
> +     */
> +    if (!RunIdentifySystem(conn, NULL, NULL, NULL, &db_name))
> +        disconnect_and_exit(1);
> +
> +    /*
> +     * Check that there is a database associated with connection, none
> +     * should be defined in this context.
> +     */
> +    if (db_name)
> +    {
> +        fprintf(stderr,
> +                _("%s: database defined for replication connection \"%s\"\n"),
> +                progname, replication_slot);
> +        disconnect_and_exit(1);
> +    }

I don't like 'defined' here. 'replication connection unexpectedly is
database specific' or something would be better.

I do wonder whether --create/--drop aren't somewhat wierd for
pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
could rename them --create-slot/drop-slot.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> I do wonder whether --create/--drop aren't somewhat wierd for
> pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
> could rename them --create-slot/drop-slot.

+1 on doing it, -1 on it being ugly.

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

On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
> On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > I do wonder whether --create/--drop aren't somewhat wierd for
> > pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
> > could rename them --create-slot/drop-slot.
> 
> +1 on doing it, -1 on it being ugly.

The reason I'm calling it uglyu is that it's different from
pg_recvlogical. We could change it there, too? A bit late, but probably
better than having a discrepancy forever?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Sat, Oct 4, 2014 at 6:48 AM, Andres
Freund<span dir="ltr"><<a href="mailto:andres@2ndquadrant.com" target="_blank">andres@2ndquadrant.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span
class="">On2014-10-03 14:02:08 -0400, Robert Haas wrote:<br /> > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund
<<ahref="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>> wrote:<br /> > > I do wonder whether
--create/--droparen't somewhat wierd for<br /> > > pg_receivexlog. It's not that clear what it means. It'd be
ugly,but we<br /> > > could rename them --create-slot/drop-slot.<br /> ><br /> > +1 on doing it, -1 on it
beingugly.<br /><br /></span>The reason I'm calling it uglyu is that it's different from<br /> pg_recvlogical. We could
changeit there, too? A bit late, but probably<br /> better than having a discrepancy forever<br
/></blockquote></div>I'mon board to make things as consistent as possible between both utilities, the only reason why
--create/--dropare used in my patch is for the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
importantfrom the user prospective if options are a maximum consistent between pg_receivexlog and pg_recvlogical. That
wouldbe even better if change is done before 9.4beta3 shows up, and I doubt that there are many users using the
--create/--dropoptions already.<br />-- <br />Michael<br /></div></div> 

On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >     <para>
> > +    <application>pg_receivexlog</application> can run in one of two following
> > +    modes, which control physical replication slot:
>
> I don't think that's good enough. There's also the important mode where
> it's not doing --create/--drop at all.
Well, yes, however the third mode is not explicitly present, and I
don't see much point in adding a --start mode thinking
backward-compatibility. Now, I refactored a bit the documentation to
mention that pg_receivexlog can perform additional actions to control
replication slots. I added as well in the portion of option --slot how
it interacts with --create-slot and --drop-slot.

> > +     if (db_name)
> > +     {
> > +             fprintf(stderr,
> > +                             _("%s: database defined for replication connection \"%s\"\n"),
> > +                             progname, replication_slot);
> > +             disconnect_and_exit(1);
> > +     }
>
> I don't like 'defined' here. 'replication connection unexpectedly is
> database specific' or something would be better.

Sure, IMO the error message should as well mention the replication
slot being used, so I reformulated as such:
"replication connection using slot foo is unexpectedly database specific"

>
> I do wonder whether --create/--drop aren't somewhat weird for
> pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
> could rename them --create-slot/drop-slot.

In line with the other patch sent earlier, options are renamed to
--create-slot and --drop-slot.
Regards,
--
Michael

On 2014-10-04 09:24:07 +0900, Michael Paquier wrote:
> On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> 
> > On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
> > > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund <andres@2ndquadrant.com>
> > wrote:
> > > > I do wonder whether --create/--drop aren't somewhat wierd for
> > > > pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
> > > > could rename them --create-slot/drop-slot.
> > >
> > > +1 on doing it, -1 on it being ugly.
> >
> > The reason I'm calling it uglyu is that it's different from
> > pg_recvlogical. We could change it there, too? A bit late, but probably
> > better than having a discrepancy forever
> >
> I'm on board to make things as consistent as possible between both
> utilities, the only reason why --create/--drop are used in my patch is for
> the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
> important from the user prospective if options are a maximum consistent
> between pg_receivexlog and pg_recvlogical. That would be even better if
> change is done before 9.4beta3 shows up, and I doubt that there are many
> users using the --create/--drop options already.

Any opinion on whether whe should accept both --create and --create-slot
or only the latter? Accepting both would get rid of problems due to
potential usages of the old syntax - and it's easier to type...

I don't really care.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 2014-10-04 14:25:27 +0900, Michael Paquier wrote:
> On Sat, Oct 4, 2014 at 9:24 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
> 
> >
> >
> > On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund <andres@2ndquadrant.com>
> > wrote:
> >
> >> On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
> >> > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund <andres@2ndquadrant.com>
> >> wrote:
> >> > > I do wonder whether --create/--drop aren't somewhat wierd for
> >> > > pg_receivexlog. It's not that clear what it means. It'd be ugly, but
> >> we
> >> > > could rename them --create-slot/drop-slot.
> >> >
> >> > +1 on doing it, -1 on it being ugly.
> >>
> >> The reason I'm calling it uglyu is that it's different from
> >> pg_recvlogical. We could change it there, too? A bit late, but probably
> >> better than having a discrepancy forever
> >>
> > I'm on board to make things as consistent as possible between both
> > utilities, the only reason why --create/--drop are used in my patch is for
> > the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
> > important from the user prospective if options are a maximum consistent
> > between pg_receivexlog and pg_recvlogical. That would be even better if
> > change is done before 9.4beta3 shows up, and I doubt that there are many
> > users using the --create/--drop options already.
> >
> And as I am on it, attached is a patch that can be applied to master and
> REL9_4_STABLE to rename the --create and --drop to --create-slot and
> --drop-slot.

Misses logicaldecoding.sgml... I'll fix that up, once there's agreement
whether we should continue accept the old form. No need to send a new
version.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Sun, Oct 5, 2014 at 12:09 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> Any opinion on whether we should accept both --create and --create-slot
> or only the latter? Accepting both would get rid of problems due to
> potential usages of the old syntax - and it's easier to type...
My vote goes for only --create-slot and --drop-slot.
-- 
Michael

On Sat, Oct 4, 2014 at 7:03 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sun, Oct 5, 2014 at 12:09 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> Any opinion on whether we should accept both --create and --create-slot
>> or only the latter? Accepting both would get rid of problems due to
>> potential usages of the old syntax - and it's easier to type...
> My vote goes for only --create-slot and --drop-slot.

Ditto.

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

On 2014-10-04 14:25:27 +0900, Michael Paquier wrote:
> And as I am on it, attached is a patch that can be applied to master and
> REL9_4_STABLE to rename the --create and --drop to --create-slot and
> --drop-slot.

Thanks.

> diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
> index c48cecc..585d7b0 100644
> --- a/src/bin/pg_basebackup/pg_recvlogical.c
> +++ b/src/bin/pg_basebackup/pg_recvlogical.c
> @@ -91,8 +91,8 @@ usage(void)
>               "                         time between status packets sent to server (default: %d)\n"),
(standby_message_timeout/ 1000));
 
>      printf(_("  -S, --slot=SLOT        name of the logical replication slot\n"));
>      printf(_("\nAction to be performed:\n"));
> -    printf(_("      --create           create a new replication slot (for the slot's name see --slot)\n"));
> -    printf(_("      --start            start streaming in a replication slot (for the slot's name see --slot)\n"));
> +    printf(_("      --create-slot      create a new replication slot (for the slot's name see --slot)\n"));
> +    printf(_("      --start-slot       start streaming in a replication slot (for the slot's name see --slot)\n"));
>      printf(_("      --drop             drop the replication slot (for the slot's name see --slot)\n"));
>      printf(_("\nReport bugs to <pgsql-bugs@postgresql.org>.\n"));
>  }
> @@ -618,9 +618,9 @@ main(int argc, char **argv)
>          {"status-interval", required_argument, NULL, 's'},
>          {"slot", required_argument, NULL, 'S'},
>  /* action */
> -        {"create", no_argument, NULL, 1},
> +        {"create-slot", no_argument, NULL, 1},
>          {"start", no_argument, NULL, 2},
> -        {"drop", no_argument, NULL, 3},
> +        {"drop-slot", no_argument, NULL, 3},
>          {NULL, 0, NULL, 0}

Uh? You're documenting --start-slot here and a couple other places, but
you implement --drop-slot. And --start-slot doesn't seem to make much
sense to me.

I've pushed a polished up version. Fixing the above and
logicaldecoding.sgml

Note that at least gnu's getopt_long() accepts --create for
--create-slot...

Thanks,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Hi,

On 2014-10-04 15:01:03 +0900, Michael Paquier wrote:
> On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > >     <para>
> > > +    <application>pg_receivexlog</application> can run in one of two following
> > > +    modes, which control physical replication slot:
> >
> > I don't think that's good enough. There's also the important mode where
> > it's not doing --create/--drop at all.
> Well, yes, however the third mode is not explicitly present, and I
> don't see much point in adding a --start mode thinking
> backward-compatibility. Now, I refactored a bit the documentation to
> mention that pg_receivexlog can perform additional actions to control
> replication slots. I added as well in the portion of option --slot how
> it interacts with --create-slot and --drop-slot.

That's better.

> > > +     if (db_name)
> > > +     {
> > > +             fprintf(stderr,
> > > +                             _("%s: database defined for replication connection \"%s\"\n"),
> > > +                             progname, replication_slot);
> > > +             disconnect_and_exit(1);
> > > +     }
> >
> > I don't like 'defined' here. 'replication connection unexpectedly is
> > database specific' or something would be better.
> 
> Sure, IMO the error message should as well mention the replication
> slot being used, so I reformulated as such:
> "replication connection using slot foo is unexpectedly database specific"

I don't see why the slot should be there. If this has gone wrong it's
not related to the slot.

> +    <application>pg_receivexlog</application> can perform one of the two
> +    following actions in order to control physical replication slots:
> +
> +    <variablelist>
> +     <varlistentry>
> +      <term><option>--create-slot</option></term>
> +      <listitem>
> +       <para>
> +        Create a new physical replication slot with the name specified in
> +        <option>--slot</option>, then start stream.
> +       </para>
> +      </listitem>
> +     </varlistentry>

*, then start to stream WAL.

> +    if (replication_slot == NULL && (do_drop_slot || do_create_slot))
> +    {
> +        fprintf(stderr, _("%s: replication slot needed with action --create-slot or --drop-slot\n"), progname);
> +        fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
> +                progname);
> +        exit(1);
> +    }

I changed this to refer to --slot.

> +    /*
> +     * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
> +     * position.
> +     */
> +    if (!RunIdentifySystem(conn, NULL, NULL, NULL, &db_name))
> +        disconnect_and_exit(1);

Comment is wrongly copied. Obviously we're neither looking at the
timeline nor the WAL position.

Pushed with these adjustments.

Amit, Fujii: Sorry for not mentioning you as reviewers of the
patchset. I hadn't followed the earlier development and thus somehow
missed that you'd both done a couple rounds of reivew.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services