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

2017-09-04 Thread Andres Freund
On 2017-09-05 11:36:47 +0900, Michael Paquier wrote:
> On Thu, Aug 31, 2017 at 9:19 PM, Magnus Hagander  wrote:
> > PFA an updated and rebased patch.
> >
> > Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
> > Forward only.
> >
> > I think that, in the end, covered all the comments?
> 
> +   if (backwards)
> +   ereport(WARNING,
> +   (errmsg("Not moving replication slot backwards!")));
> Shouldn't this be an ERROR, mentioning the current position of the slot?
> 
> +ereport(ERROR,
> +(errmsg("Only physical replication slots can be 
> advanced.")));
> ERRCODE_FEATURE_NOT_SUPPORTED, no?

Seither of these seem to follow the message guidelines.

Greetings,

Andres Freund


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


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

2017-09-04 Thread Michael Paquier
On Thu, Aug 31, 2017 at 9:19 PM, Magnus Hagander  wrote:
> PFA an updated and rebased patch.
>
> Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
> Forward only.
>
> I think that, in the end, covered all the comments?

+   if (backwards)
+   ereport(WARNING,
+   (errmsg("Not moving replication slot backwards!")));
Shouldn't this be an ERROR, mentioning the current position of the slot?

+ereport(ERROR,
+(errmsg("Only physical replication slots can be advanced.")));
ERRCODE_FEATURE_NOT_SUPPORTED, no?
-- 
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] Function to move the position of a replication slot

2017-09-03 Thread Magnus Hagander
On Sun, Sep 3, 2017 at 12:20 AM, Robert Haas  wrote:

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

It can also be combined with file-based restoring to keep a "blocker"
preventing removal before a segment has progressed through a workflow for
example.

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


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

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

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

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

- Andres


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


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

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

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

regards, tom lane


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


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

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

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

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

- Andres


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


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

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

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

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


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


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

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

One use-case possible, even if it is easy to counter it by dropping
and recreating a slot, is to give up with what has been retained and
allow another client to reuse the same slot for a new standby.
-- 
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] Function to move the position of a replication slot

2017-09-01 Thread Tom Lane
Peter Eisentraut  writes:
> On 8/31/17 08:19, Magnus Hagander wrote:
>> I think that, in the end, covered all the comments?

> I didn't see any explanation of what this would actually be useful for.
> I suppose you could skip over some changes you don't want replicated,
> but how do you find to what position to skip?

Um ... I can see how you might expect to skip some events in a logical
replication stream and have a chance of things not being utterly broken.
But how can that work for physical replication?  Missed updates are
normally spelled "unrecoverable data corruption" at that level.

regards, tom lane


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


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

2017-09-01 Thread Peter Eisentraut
On 8/31/17 08:19, Magnus Hagander wrote:
> Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
> Forward only. 
> 
> I think that, in the end, covered all the comments?

I didn't see any explanation of what this would actually be useful for.
I suppose you could skip over some changes you don't want replicated,
but how do you find to what position to skip?

Logical replication has a similar mechanism, using the function
pg_replication_origin_advance().  Any overlap there?  (Maybe the names
could be aligned.)
(https://www.postgresql.org/docs/devel/static/logical-replication-conflicts.html)

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


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


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

2017-08-31 Thread Magnus Hagander
On Thu, Aug 17, 2017 at 2:19 AM, Craig Ringer  wrote:

> On 17 August 2017 at 07:30, Michael Paquier 
> wrote:
>
>>
>> Definitely agreed on that. Any move function would need to check if
>> the WAL position given by caller is already newer than what's
>> available in the local pg_wal (minimum of all other slots), with a
>> shared lock that would need to be taken by xlog.c when recycling past
>> segments. A forward function works on a single entry, which should be
>> disabled at the moment of the update. It looks dangerous to me to do
>> such an operation if there is a consumer of a slot currently on it.
>>
>>
> pg_advance_replication_slot(...)
>
> ERROR's on logical slot, for now. Physical slots only.
>
> Forward-only.
>
> Future work to allow it to use the logical decoding infrastructure to
> fast-forward a slot by reading only catalog change information and
> invalidations, either via a dummy output plugin that filters out all xacts,
> or by lower level use of the decoding code.
>
> Reasonable?
>
>
PFA an updated and rebased patch.

Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
Forward only.

I think that, in the end, covered all the comments?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 641b3b8f4e..452559f260 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19030,6 +19030,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 be called when connected to the same database the slot was created on.

   
+  
+   
+
+ pg_advance_replication_slot
+
+pg_advance_replication_slot(slot_name name, position pg_lsn)
+   
+   
+bool
+   
+   
+Advances the current restart position of a physical
+replication slot named slot_name.
+The slot will not be moved backwards, and it will not be
+moved beyond the current insert location. Logical replication
+slots cannot be moved. Returns true if the position was changed.
+   
+  
 
   

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index d4cbd83bde..0e27e739bf 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -177,6 +177,73 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 }
 
 /*
+ * SQL function for moving the position in a replication slot.
+ */
+Datum
+pg_advance_replication_slot(PG_FUNCTION_ARGS)
+{
+	Name		slotname = PG_GETARG_NAME(0);
+	XLogRecPtr	moveto = PG_GETARG_LSN(1);
+	bool		changed = false;
+	bool 		backwards = false;
+
+	Assert(!MyReplicationSlot);
+
+	check_permissions();
+
+	if (XLogRecPtrIsInvalid(moveto))
+		ereport(ERROR,
+(errmsg("Invalid target xlog position ")));
+
+	/* Temporarily acquire the slot so we "own" it */
+	ReplicationSlotAcquire(NameStr(*slotname), true);
+
+	/* Only physical slots can be moved */
+	if (MyReplicationSlot->data.database != InvalidOid)
+	{
+		ReplicationSlotRelease();
+		ereport(ERROR,
+(errmsg("Only physical replication slots can be advanced.")));
+	}
+
+	if (moveto > GetXLogWriteRecPtr())
+		/* Can't move past current position, so truncate there */
+		moveto = GetXLogWriteRecPtr();
+
+	/* Now adjust it */
+	SpinLockAcquire(>mutex);
+	if (MyReplicationSlot->data.restart_lsn != moveto)
+	{
+		/* Never move backwards, because bad things can happen */
+		if (MyReplicationSlot->data.restart_lsn > moveto)
+			backwards = true;
+		else
+		{
+			MyReplicationSlot->data.restart_lsn = moveto;
+			changed = true;
+		}
+	}
+	SpinLockRelease(>mutex);
+
+	if (backwards)
+		ereport(WARNING,
+(errmsg("Not moving replication slot backwards!")));
+
+
+	if (changed)
+	{
+		ReplicationSlotMarkDirty();
+		ReplicationSlotsComputeRequiredLSN();
+		ReplicationSlotSave();
+	}
+
+	ReplicationSlotRelease();
+
+	PG_RETURN_BOOL(changed);
+}
+
+
+/*
  * pg_get_replication_slots - SQL SRF showing active replication slots.
  */
 Datum
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 8b33b4e0ea..3495447cf9 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5293,6 +5293,8 @@ DATA(insert OID = 3779 (  pg_create_physical_replication_slot PGNSP PGUID 12 1 0
 DESCR("create a physical replication slot");
 DATA(insert OID = 3780 (  pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 1 0 2278 "19" _null_ _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
 DESCR("drop a replication slot");
+DATA(insert OID = 3998 (  pg_advance_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 2 0 16 "19 3220" _null_ _null_ _null_ _null_ _null_ pg_advance_replication_slot _null_ _null_ _null_ ));
+DESCR("move a replication slot position");
 DATA(insert OID = 

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

2017-08-16 Thread Craig Ringer
On 17 August 2017 at 09:33, Andres Freund  wrote:

> On 2017-08-16 21:25:48 -0400, Robert Haas wrote:
> > On Wed, Aug 16, 2017 at 5:55 PM, Andres Freund 
> wrote:
> > > I think we should constrain the API to only allow later LSNs than
> > > currently in the slot, rather than arbitrary ones. That's why I was
> > > thinking of "forward".  I'm not convinced it's a good / safe idea to
> > > allow arbitrary values to be set.
> >
> > Maybe I shouldn't play the devil's advocate here, but isn't a feature
> > like this by definition only for people who Know What They Are Doing?
> > If so, why not let them back the slot up?  I'm sure that will work out
> > just fine.  They Know What They Are Doing.
>
> I have yet to hear a reason for allowing to move things backward etc. So
> I'm not sure what the benefit would be. But more importantly I'd like to
> make this available to non-superusers at some point, and there I think
> it's more important that they can't do bad things. The reason for
> allowing it for non-superusers is that I think it's quite a useful
> function to be used by an automated system. E.g. to ensure enough, but
> not too much, WAL is available for a tertiary standby both on the actual
> primary and a failover node.
>

I strongly agree.

If you really need to move a physical slot back (why?) you can do it with
an extension that uses the low level APIs. But I can't see why you would
want to.

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


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

2017-08-16 Thread Andres Freund
On 2017-08-16 21:25:48 -0400, Robert Haas wrote:
> On Wed, Aug 16, 2017 at 5:55 PM, Andres Freund  wrote:
> > I think we should constrain the API to only allow later LSNs than
> > currently in the slot, rather than arbitrary ones. That's why I was
> > thinking of "forward".  I'm not convinced it's a good / safe idea to
> > allow arbitrary values to be set.
> 
> Maybe I shouldn't play the devil's advocate here, but isn't a feature
> like this by definition only for people who Know What They Are Doing?
> If so, why not let them back the slot up?  I'm sure that will work out
> just fine.  They Know What They Are Doing.

I have yet to hear a reason for allowing to move things backward etc. So
I'm not sure what the benefit would be. But more importantly I'd like to
make this available to non-superusers at some point, and there I think
it's more important that they can't do bad things. The reason for
allowing it for non-superusers is that I think it's quite a useful
function to be used by an automated system. E.g. to ensure enough, but
not too much, WAL is available for a tertiary standby both on the actual
primary and a failover node.

Greetings,

Andres Freund


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


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

2017-08-16 Thread Robert Haas
On Wed, Aug 16, 2017 at 5:55 PM, Andres Freund  wrote:
> I think we should constrain the API to only allow later LSNs than
> currently in the slot, rather than arbitrary ones. That's why I was
> thinking of "forward".  I'm not convinced it's a good / safe idea to
> allow arbitrary values to be set.

Maybe I shouldn't play the devil's advocate here, but isn't a feature
like this by definition only for people who Know What They Are Doing?
If so, why not let them back the slot up?  I'm sure that will work out
just fine.  They Know What They Are Doing.

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


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


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

2017-08-16 Thread Michael Paquier
On Thu, Aug 17, 2017 at 9:19 AM, Craig Ringer  wrote:
> pg_advance_replication_slot(...)
>
> ERROR's on logical slot, for now. Physical slots only.
>
> Forward-only.
>
> Future work to allow it to use the logical decoding infrastructure to
> fast-forward a slot by reading only catalog change information and
> invalidations, either via a dummy output plugin that filters out all xacts,
> or by lower level use of the decoding code.
>
> Reasonable?

Yes. I did not imply logical slots in my previous message, sorry if my
words were incomplete.
-- 
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] Function to move the position of a replication slot

2017-08-16 Thread Craig Ringer
On 17 August 2017 at 07:30, Michael Paquier 
wrote:

>
> Definitely agreed on that. Any move function would need to check if
> the WAL position given by caller is already newer than what's
> available in the local pg_wal (minimum of all other slots), with a
> shared lock that would need to be taken by xlog.c when recycling past
> segments. A forward function works on a single entry, which should be
> disabled at the moment of the update. It looks dangerous to me to do
> such an operation if there is a consumer of a slot currently on it.
>
>
pg_advance_replication_slot(...)

ERROR's on logical slot, for now. Physical slots only.

Forward-only.

Future work to allow it to use the logical decoding infrastructure to
fast-forward a slot by reading only catalog change information and
invalidations, either via a dummy output plugin that filters out all xacts,
or by lower level use of the decoding code.

Reasonable?

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


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

2017-08-16 Thread Michael Paquier
On Thu, Aug 17, 2017 at 7:14 AM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2017-08-16 17:06:42 -0400, Tom Lane wrote:
>>> If I understand what this is meant to do, maybe better
>>> pg_move_replication_slot_lsn() or pg_change_replication_slot_lsn() ?
>>> The point being that you're adjusting the LSN pointer contained
>>> in the slot, which is distinct from the slot itself.
>
>> I think we should constrain the API to only allow later LSNs than
>> currently in the slot, rather than arbitrary ones. That's why I was
>> thinking of "forward".  I'm not convinced it's a good / safe idea to
>> allow arbitrary values to be set.
>
> +1 for constraining it like that, but I don't think that's an argument
> against using "move" or "change" as the verb.  I don't like "forward"
> because that's not the right word.  The only verb senses of "forward"
> in my Mac's dictionary are "send a message on to a further destination"
> and "help to advance or promote" (the latter usage is pretty obscure IMO).
> Neither one seems applicable here.

Definitely agreed on that. Any move function would need to check if
the WAL position given by caller is already newer than what's
available in the local pg_wal (minimum of all other slots), with a
shared lock that would need to be taken by xlog.c when recycling past
segments. A forward function works on a single entry, which should be
disabled at the moment of the update. It looks dangerous to me to do
such an operation if there is a consumer of a slot currently on it.
-- 
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] Function to move the position of a replication slot

2017-08-16 Thread Andres Freund
On 2017-08-16 18:14:45 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-08-16 17:06:42 -0400, Tom Lane wrote:
> >> If I understand what this is meant to do, maybe better
> >> pg_move_replication_slot_lsn() or pg_change_replication_slot_lsn() ?
> >> The point being that you're adjusting the LSN pointer contained
> >> in the slot, which is distinct from the slot itself.
> 
> > I think we should constrain the API to only allow later LSNs than
> > currently in the slot, rather than arbitrary ones. That's why I was
> > thinking of "forward".  I'm not convinced it's a good / safe idea to
> > allow arbitrary values to be set.
> 
> +1 for constraining it like that, but I don't think that's an argument
> against using "move" or "change" as the verb.  I don't like "forward"
> because that's not the right word.  The only verb senses of "forward"
> in my Mac's dictionary are "send a message on to a further destination"
> and "help to advance or promote" (the latter usage is pretty obscure IMO).
> Neither one seems applicable here.

I might have thinking too much with the German "version" of the word in
mind.  However looking it up now I do see "to advance or help onward;
promote" and "to advance or play a cassette, digital recording, slide
projector, etc., in the forward direction: " -
http://www.dictionary.com/browse/forward
which kind of seems to fit.  don't know how authoritative which
dictionary is considered to be...

- Andres


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


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

2017-08-16 Thread Tom Lane
Alvaro Herrera  writes:
> Andres Freund wrote:
>> I think we should constrain the API to only allow later LSNs than
>> currently in the slot, rather than arbitrary ones. That's why I was
>> thinking of "forward".  I'm not convinced it's a good / safe idea to
>> allow arbitrary values to be set.

> Hmm.  In terms of safety, it is safe to move the LSN backwards, as long
> as the oldest LSN across all slots is not changed -- in other words, the
> actual safe limit is the oldest of all slot LSNs, rather than the
> current position of the slot being manipulated (which is what you're
> saying).

True, but you'd need to take some kind of global lock to verify that,
whereas "move forward only" would only need to examine/change the one
slot.

regards, tom lane


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


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

2017-08-16 Thread Alvaro Herrera
Andres Freund wrote:
> On 2017-08-16 17:06:42 -0400, Tom Lane wrote:

> > If I understand what this is meant to do, maybe better
> > pg_move_replication_slot_lsn() or pg_change_replication_slot_lsn() ?
> > The point being that you're adjusting the LSN pointer contained
> > in the slot, which is distinct from the slot itself.
> 
> I think we should constrain the API to only allow later LSNs than
> currently in the slot, rather than arbitrary ones. That's why I was
> thinking of "forward".  I'm not convinced it's a good / safe idea to
> allow arbitrary values to be set.

Hmm.  In terms of safety, it is safe to move the LSN backwards, as long
as the oldest LSN across all slots is not changed -- in other words, the
actual safe limit is the oldest of all slot LSNs, rather than the
current position of the slot being manipulated (which is what you're
saying).

I don't know if this is useful for the use case Magnus described; TBH I
didn't understand that use case.

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


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


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

2017-08-16 Thread Tom Lane
Andres Freund  writes:
> On 2017-08-16 17:06:42 -0400, Tom Lane wrote:
>> If I understand what this is meant to do, maybe better
>> pg_move_replication_slot_lsn() or pg_change_replication_slot_lsn() ?
>> The point being that you're adjusting the LSN pointer contained
>> in the slot, which is distinct from the slot itself.

> I think we should constrain the API to only allow later LSNs than
> currently in the slot, rather than arbitrary ones. That's why I was
> thinking of "forward".  I'm not convinced it's a good / safe idea to
> allow arbitrary values to be set.

+1 for constraining it like that, but I don't think that's an argument
against using "move" or "change" as the verb.  I don't like "forward"
because that's not the right word.  The only verb senses of "forward"
in my Mac's dictionary are "send a message on to a further destination"
and "help to advance or promote" (the latter usage is pretty obscure IMO).
Neither one seems applicable here.

regards, tom lane


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


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

2017-08-16 Thread Andres Freund
On 2017-08-16 17:06:42 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-08-16 12:24:11 -0400, Peter Eisentraut wrote:
> >> On 5/4/17 08:05, Magnus Hagander wrote:
> >>> PFA a patch that adds a new function, pg_move_replication_slot, that
> >>> makes it possible to move the location of a replication slot without
> >>> actually consuming all the WAL on it.
> 
> >> The name keeps confusing me.  I understand "move" to be "rename" or
> >> possibly "move it elsewhere", but not "move around in".  I understand
> >> that there is an analogy with FETCH/MOVE in cursors, but it's still
> >> confusing.
> 
> > pg_forward_replication_slot()?
> 
> If I understand what this is meant to do, maybe better
> pg_move_replication_slot_lsn() or pg_change_replication_slot_lsn() ?
> The point being that you're adjusting the LSN pointer contained
> in the slot, which is distinct from the slot itself.

I think we should constrain the API to only allow later LSNs than
currently in the slot, rather than arbitrary ones. That's why I was
thinking of "forward".  I'm not convinced it's a good / safe idea to
allow arbitrary values to be set.

Greetings,

Andres Freund


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


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

2017-08-16 Thread Tom Lane
Andres Freund  writes:
> On 2017-08-16 12:24:11 -0400, Peter Eisentraut wrote:
>> On 5/4/17 08:05, Magnus Hagander wrote:
>>> PFA a patch that adds a new function, pg_move_replication_slot, that
>>> makes it possible to move the location of a replication slot without
>>> actually consuming all the WAL on it.

>> The name keeps confusing me.  I understand "move" to be "rename" or
>> possibly "move it elsewhere", but not "move around in".  I understand
>> that there is an analogy with FETCH/MOVE in cursors, but it's still
>> confusing.

> pg_forward_replication_slot()?

If I understand what this is meant to do, maybe better
pg_move_replication_slot_lsn() or pg_change_replication_slot_lsn() ?
The point being that you're adjusting the LSN pointer contained
in the slot, which is distinct from the slot itself.

regards, tom lane


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


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

2017-08-16 Thread Andres Freund
On 2017-08-16 12:24:11 -0400, Peter Eisentraut wrote:
> On 5/4/17 08:05, Magnus Hagander wrote:
> > PFA a patch that adds a new function, pg_move_replication_slot, that
> > makes it possible to move the location of a replication slot without
> > actually consuming all the WAL on it.
> 
> The name keeps confusing me.  I understand "move" to be "rename" or
> possibly "move it elsewhere", but not "move around in".  I understand
> that there is an analogy with FETCH/MOVE in cursors, but it's still
> confusing.

pg_forward_replication_slot()?

- Andres


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


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

2017-08-16 Thread Peter Eisentraut
On 5/4/17 08:05, Magnus Hagander wrote:
> PFA a patch that adds a new function, pg_move_replication_slot, that
> makes it possible to move the location of a replication slot without
> actually consuming all the WAL on it.

The name keeps confusing me.  I understand "move" to be "rename" or
possibly "move it elsewhere", but not "move around in".  I understand
that there is an analogy with FETCH/MOVE in cursors, but it's still
confusing.

I would also like to see some documentation for a use case of this.

Anyway, as discussed elsewhere in this thread, this patch needs several
changes.

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


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


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

2017-05-10 Thread Petr Jelinek
On 10/05/17 22:17, Dmitry Dolgov wrote:
>> On 4 May 2017 at 20:05, Magnus Hagander  > wrote:
>>
>> PFA a patch that adds a new function, pg_move_replication_slot, that
> makes it
>> possible to move the location of a replication slot without actually
>> consuming all the WAL on it.
>  
> Just a few questions just a few questions out of curiosity:
> 
> * does it make sense to create a few tests for this function in
>   `contrib/test_decoding` (as shown in attachment)?
> 

As Craig said this will not work correctly for logical slots so it
should throw error on those at the moment (at least until we write
something that works, but that's far more complex than this patch).

-- 
  Petr Jelinek  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


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

2017-05-10 Thread Dmitry Dolgov
> On 4 May 2017 at 20:05, Magnus Hagander  wrote:
>
> PFA a patch that adds a new function, pg_move_replication_slot, that
makes it
> possible to move the location of a replication slot without actually
> consuming all the WAL on it.

Just a few questions just a few questions out of curiosity:

* does it make sense to create a few tests for this function in
  `contrib/test_decoding` (as shown in attachment)?

* what should happen if the second argument is `NULL`? There is a
verification
  `XLogRecPtrIsInvalid(moveto)`, but it's possible to pass `NULL`, and looks
  like it leads to result different from boolean:

```
SELECT pg_move_replication_slot('regression_slot_5', NULL);
 pg_move_replication_slot
--

(1 row)
```
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index 9f5f8a9..7c69110 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -101,3 +101,39 @@ SELECT pg_drop_replication_slot('regression_slot1');
 ERROR:  replication slot "regression_slot1" does not exist
 SELECT pg_drop_replication_slot('regression_slot2');
 ERROR:  replication slot "regression_slot2" does not exist
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_3', 'test_decoding');
+ ?column? 
+--
+ init
+(1 row)
+
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_4', 'test_decoding', true);
+ ?column? 
+--
+ init
+(1 row)
+
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_5', 'test_decoding');
+ ?column? 
+--
+ init
+(1 row)
+
+SELECT pg_move_replication_slot('regression_slot_3', '0/aabbccdd');
+ pg_move_replication_slot 
+--
+ t
+(1 row)
+
+SELECT pg_move_replication_slot('regression_slot_4', '0/aabbccdd');
+ pg_move_replication_slot 
+--
+ t
+(1 row)
+
+SELECT pg_move_replication_slot('regression_slot_5', NULL);
+ pg_move_replication_slot 
+--
+ 
+(1 row)
+
diff --git a/contrib/test_decoding/sql/slot.sql b/contrib/test_decoding/sql/slot.sql
index fa9561f..207f845 100644
--- a/contrib/test_decoding/sql/slot.sql
+++ b/contrib/test_decoding/sql/slot.sql
@@ -53,3 +53,12 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot1', 'test_
 -- both should error as they should be dropped on error
 SELECT pg_drop_replication_slot('regression_slot1');
 SELECT pg_drop_replication_slot('regression_slot2');
+
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_3', 'test_decoding');
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_4', 'test_decoding', true);
+
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_5', 'test_decoding');
+
+SELECT pg_move_replication_slot('regression_slot_3', '0/aabbccdd');
+SELECT pg_move_replication_slot('regression_slot_4', '0/aabbccdd');
+SELECT pg_move_replication_slot('regression_slot_5', NULL);
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6298657..d05974e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18986,6 +18986,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 be called when connected to the same database the slot was created on.

   
+  
+   
+
+ pg_move_replication_slot
+
+pg_move_replication_slot(slot_name name, position pg_lsn)
+   
+   
+bool
+   
+   
+Moves the current restart position of a physical or logical
+replication slot named slot_name.
+The slot will not be moved backwards, and it will not be
+moved beyond the current insert location. Returns true if
+the position was changed.
+   
+  
 
   

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 6ee1e68..a9faa10 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -176,6 +176,66 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 }
 
 /*
+ * SQL function for moving the position in a replication slot.
+ */
+Datum
+pg_move_replication_slot(PG_FUNCTION_ARGS)
+{
+	Name		slotname = PG_GETARG_NAME(0);
+	XLogRecPtr	moveto = PG_GETARG_LSN(1);
+	char	   *slotnamestr;
+	bool		changed = false;
+	bool 		backwards = false;
+
+	Assert(!MyReplicationSlot);
+
+	check_permissions();
+
+	if (XLogRecPtrIsInvalid(moveto))
+		ereport(ERROR,
+(errmsg("Invalid target xlog position ")));
+
+	/* Temporarily acquire the slot so we "own" it */
+	ReplicationSlotAcquire(NameStr(*slotname));
+
+	if (moveto > GetXLogWriteRecPtr())
+		/* Can't move past current position, so truncate there */
+		moveto = GetXLogWriteRecPtr();
+
+	/* Now adjust it */
+	SpinLockAcquire(>mutex);
+	if (MyReplicationSlot->data.restart_lsn != moveto)
+	{
+		/* Never move backwards, because bad things can happen */
+		if 

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

2017-05-04 Thread Craig Ringer
On 4 May 2017 at 20:45, Magnus Hagander  wrote:
> On Thu, May 4, 2017 at 2:42 PM, Craig Ringer  wrote:
>>
>> On 4 May 2017 at 20:05, Magnus Hagander  wrote:
>> > PFA a patch that adds a new function, pg_move_replication_slot, that
>> > makes
>> > it possible to move the location of a replication slot without actually
>> > consuming all the WAL on it.
>>
>> > This can be useful for example to keep replication slots in sync between
>> > different servers in a replication cluster.
>>
>> It needs a test to ensure it only operates on physical slots. It
>> should ERROR on a logical slot, since it has no way of correctly
>> advancing the catalog_xmin or finding a reasonable restart_lsn  for
>> logical decoding.
>
>
> I guess that makes sense, yeah. I didn't try it with that :)

Barring that and matching docs changes, looks fine to me at first glance.

Not sure you need to acquire the spinlock on the slot, since you
acquired the slot for your backend. It won't hurt, but I don't think
it's necessary.

I'm not convinced you need a WARNING for moving the slot backwards. If
one is emitted, it should be a proper ereport with current position
and requested position in errdetail. I'm not sure it's a useful
message though.

-- 
 Craig Ringer   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


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

2017-05-04 Thread Magnus Hagander
On Thu, May 4, 2017 at 2:42 PM, Craig Ringer  wrote:

> On 4 May 2017 at 20:05, Magnus Hagander  wrote:
> > PFA a patch that adds a new function, pg_move_replication_slot, that
> makes
> > it possible to move the location of a replication slot without actually
> > consuming all the WAL on it.
>
> > This can be useful for example to keep replication slots in sync between
> > different servers in a replication cluster.
>
> It needs a test to ensure it only operates on physical slots. It
> should ERROR on a logical slot, since it has no way of correctly
> advancing the catalog_xmin or finding a reasonable restart_lsn  for
> logical decoding.
>

I guess that makes sense, yeah. I didn't try it with that :)



> I'm still fine with the name, since I plan to add that capability in
> pg11 by running through logical decoding and ReorderBufferSkip()ing
> each xact until we reach the target lsn.
>

Cool.

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


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

2017-05-04 Thread Craig Ringer
On 4 May 2017 at 20:05, Magnus Hagander  wrote:
> PFA a patch that adds a new function, pg_move_replication_slot, that makes
> it possible to move the location of a replication slot without actually
> consuming all the WAL on it.

> This can be useful for example to keep replication slots in sync between
> different servers in a replication cluster.

It needs a test to ensure it only operates on physical slots. It
should ERROR on a logical slot, since it has no way of correctly
advancing the catalog_xmin or finding a reasonable restart_lsn  for
logical decoding.

I'm still fine with the name, since I plan to add that capability in
pg11 by running through logical decoding and ReorderBufferSkip()ing
each xact until we reach the target lsn.

-- 
 Craig Ringer   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


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

2017-05-04 Thread Magnus Hagander
PFA a patch that adds a new function, pg_move_replication_slot, that makes
it possible to move the location of a replication slot without actually
consuming all the WAL on it.

This can be useful for example to keep replication slots in sync between
different servers in a replication cluster.

(Obviously this is intended for 11, as we're well into the freeze for 10.
Just to be clear. so I'll go add itto the summer commitfest)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f06d0a9..6b1ff0a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18968,6 +18968,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 be called when connected to the same database the slot was created on.

   
+  
+   
+
+ pg_move_replication_slot
+
+pg_move_replication_slot(slot_name name, position pg_lsn)
+   
+   
+bool
+   
+   
+Moves the current restart position of a physical or logical
+replication slot named slot_name.
+The slot will not be moved backwards, and it will not be
+moved beyond the current insert location. Returns true if
+the position was changed.
+   
+  
 
   

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 6ee1e68..a9faa10 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -176,6 +176,66 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 }
 
 /*
+ * SQL function for moving the position in a replication slot.
+ */
+Datum
+pg_move_replication_slot(PG_FUNCTION_ARGS)
+{
+	Name		slotname = PG_GETARG_NAME(0);
+	XLogRecPtr	moveto = PG_GETARG_LSN(1);
+	char	   *slotnamestr;
+	bool		changed = false;
+	bool 		backwards = false;
+
+	Assert(!MyReplicationSlot);
+
+	check_permissions();
+
+	if (XLogRecPtrIsInvalid(moveto))
+		ereport(ERROR,
+(errmsg("Invalid target xlog position ")));
+
+	/* Temporarily acquire the slot so we "own" it */
+	ReplicationSlotAcquire(NameStr(*slotname));
+
+	if (moveto > GetXLogWriteRecPtr())
+		/* Can't move past current position, so truncate there */
+		moveto = GetXLogWriteRecPtr();
+
+	/* Now adjust it */
+	SpinLockAcquire(>mutex);
+	if (MyReplicationSlot->data.restart_lsn != moveto)
+	{
+		/* Never move backwards, because bad things can happen */
+		if (MyReplicationSlot->data.restart_lsn > moveto)
+			backwards = true;
+		else
+		{
+			MyReplicationSlot->data.restart_lsn = moveto;
+			changed = true;
+		}
+	}
+	SpinLockRelease(>mutex);
+
+	if (backwards)
+		ereport(WARNING,
+(errmsg("Not moving replication slot backwards!")));
+
+
+	if (changed)
+	{
+		ReplicationSlotMarkDirty();
+		ReplicationSlotsComputeRequiredLSN();
+		ReplicationSlotSave();
+	}
+
+	ReplicationSlotRelease();
+
+	PG_RETURN_BOOL(changed);
+}
+
+
+/*
  * pg_get_replication_slots - SQL SRF showing active replication slots.
  */
 Datum
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 82562ad..04e97ff 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5289,6 +5289,8 @@ DATA(insert OID = 3779 (  pg_create_physical_replication_slot PGNSP PGUID 12 1 0
 DESCR("create a physical replication slot");
 DATA(insert OID = 3780 (  pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 1 0 2278 "19" _null_ _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
 DESCR("drop a replication slot");
+DATA(insert OID = 3998 (  pg_move_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 2 0 16 "19 3220" _null_ _null_ _null_ _null_ _null_ pg_move_replication_slot _null_ _null_ _null_ ));
+DESCR("move a replication slot position");
 DATA(insert OID = 3781 (  pg_get_replication_slots	PGNSP PGUID 12 1 10 0 0 f f f f f t s s 0 0 2249 "" "{19,19,25,26,16,16,23,28,28,3220,3220}" "{o,o,o,o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn}" _null_ _null_ pg_get_replication_slots _null_ _null_ _null_ ));
 DESCR("information about replication slots currently in use");
 DATA(insert OID = 3786 (  pg_create_logical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 3 0 2249 "19 19 16" "{19,19,16,25,3220}" "{i,i,i,o,o}" "{slot_name,plugin,temporary,slot_name,wal_position}" _null_ _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));

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