pg_receivexlog and replication slots

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

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

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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

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

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