Re: Set new system identifier using pg_resetxlog

Hello,

attached is a simple patch which makes it possible to change the system
identifier of the cluster in pg_control. This is useful for
individualization of the instance that is started on top of data
directory produced by pg_basebackup - something that's helpful for
logical replication setup where you need to easily identify each node
(it's used by Bidirectional Replication for example).

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

On Fri, Jun 13, 2014 at 8:31 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> attached is a simple patch which makes it possible to change the system
> identifier of the cluster in pg_control. This is useful for
> individualization of the instance that is started on top of data directory
> produced by pg_basebackup - something that's helpful for logical replication
> setup where you need to easily identify each node (it's used by
> Bidirectional Replication for example).

I can clearly understand the utility of being able to reset the system
ID to a new, randomly-generated system ID - but giving the user the
ability to set a particular value of their own choosing seems like a
pretty sharp tool. What is the use case for that?

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

On 17/06/14 16:18, Robert Haas wrote:
> On Fri, Jun 13, 2014 at 8:31 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> attached is a simple patch which makes it possible to change the system
>> identifier of the cluster in pg_control. This is useful for
>> individualization of the instance that is started on top of data directory
>> produced by pg_basebackup - something that's helpful for logical replication
>> setup where you need to easily identify each node (it's used by
>> Bidirectional Replication for example).
>
> I can clearly understand the utility of being able to reset the system
> ID to a new, randomly-generated system ID - but giving the user the
> ability to set a particular value of their own choosing seems like a
> pretty sharp tool. What is the use case for that?
>

Let's say you want to initialize new logical replication node via
pg_basebackup and you want your replication slots to be easily
identifiable so you use your local system id as part of the slot name.

In that case you need to know the future system id of the node because
you need to register the slot before consistent point to which you
replay via streaming replication (and you can't replay anymore once you
changed the system id). Which means you need to generate your system id
in advance and be able to change it in pg_control later.

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

On Tue, Jun 17, 2014 at 10:33 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> On 17/06/14 16:18, Robert Haas wrote:
>> On Fri, Jun 13, 2014 at 8:31 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com>
>> wrote:
>>> attached is a simple patch which makes it possible to change the system
>>> identifier of the cluster in pg_control. This is useful for
>>> individualization of the instance that is started on top of data
>>> directory
>>> produced by pg_basebackup - something that's helpful for logical
>>> replication
>>> setup where you need to easily identify each node (it's used by
>>> Bidirectional Replication for example).
>>
>>
>> I can clearly understand the utility of being able to reset the system
>> ID to a new, randomly-generated system ID - but giving the user the
>> ability to set a particular value of their own choosing seems like a
>> pretty sharp tool. What is the use case for that?
>
> Let's say you want to initialize new logical replication node via
> pg_basebackup and you want your replication slots to be easily identifiable
> so you use your local system id as part of the slot name.
>
> In that case you need to know the future system id of the node because you
> need to register the slot before consistent point to which you replay via
> streaming replication (and you can't replay anymore once you changed the
> system id). Which means you need to generate your system id in advance and
> be able to change it in pg_control later.

Hmm. I guess that makes sense.

But it seems to me that we might need to have a process discussion
here, because, while I'm all in favor of incremental feature proposals
that build towards a larger goal, it currently appears that the larger
goal toward which you are building is not something that's been
publicly discussed and debated on this list. And I really think we
need to have that conversation. Obviously, individual patches will
still need to be debated, but I feel like 2ndQuadrant is trying to
construct a castle without showing the community the floor plan. I
believe that there is relatively broad agreement that we would all
like a castle, but different people may have legitimately different
ideas about how it should be constructed. If the work arrives as a
series of disconnected pieces (user-specified system ID, event
triggers for CREATE, etc.), then everyone outside of 2ndQuadrant has
to take it on faith that those pieces are going to eventually fit
together in a way that we'll all be happy with. In some cases, that's
fine, because the feature is useful on its own merits whether it ends
up being part of the castle or not.

But in other cases, like this one, if the premise that the slot name
should match the system identifier isn't something the community wants
to accept, then taking a patch that lets people do that is probably a
bad idea, because at least one person will use it to set the system
identifier of a system to a value that enables physical replication to
take place when that is actually totally unsafe, and we don't want to
enable that for no reason. Maybe the slot name should match the
replication identifier rather than the standby system ID, for example.
There are conflicting proposals for how replication identifiers should
work, but one of those proposals limits it to 16 bits. If we're going
to have multiple identifiers floating around anyway, I'd rather have a
slot called 7 than one called 6024402925054484590. On the other hand,
maybe there's going to be a new proposal to use the database system
identifier as a replication identifier, which might be a fine idea and
which would demolish that argument.

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

On 2014-06-17 12:07:04 -0400, Robert Haas wrote:
> On Tue, Jun 17, 2014 at 10:33 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> > On 17/06/14 16:18, Robert Haas wrote:
> >> On Fri, Jun 13, 2014 at 8:31 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com>
> >> wrote:
> >>> attached is a simple patch which makes it possible to change the system
> >>> identifier of the cluster in pg_control. This is useful for
> >>> individualization of the instance that is started on top of data
> >>> directory
> >>> produced by pg_basebackup - something that's helpful for logical
> >>> replication
> >>> setup where you need to easily identify each node (it's used by
> >>> Bidirectional Replication for example).
> >>
> >>
> >> I can clearly understand the utility of being able to reset the system
> >> ID to a new, randomly-generated system ID - but giving the user the
> >> ability to set a particular value of their own choosing seems like a
> >> pretty sharp tool. What is the use case for that?

I've previously hacked this up adhoc during data recovery when I needed
to make another cluster similar enough that I could replay WAL.

Another usecase is to mark a database as independent from its
origin. Imagine a database that gets sharded across several
servers. It's not uncommon to do that by initially basebackup'ing the
database to several nodes and then use them separately from
thereon. It's quite useful to actually mark them as being
distinct. Especially as several of them right now would end up with the
same timeline id...

> But it seems to me that we might need to have a process discussion
> here, because, while I'm all in favor of incremental feature proposals
> that build towards a larger goal, it currently appears that the larger
> goal toward which you are building is not something that's been
> publicly discussed and debated on this list. And I really think we
> need to have that conversation. Obviously, individual patches will
> still need to be debated, but I feel like 2ndQuadrant is trying to
> construct a castle without showing the community the floor plan. I
> believe that there is relatively broad agreement that we would all
> like a castle, but different people may have legitimately different
> ideas about how it should be constructed. If the work arrives as a
> series of disconnected pieces (user-specified system ID, event
> triggers for CREATE, etc.), then everyone outside of 2ndQuadrant has
> to take it on faith that those pieces are going to eventually fit
> together in a way that we'll all be happy with. In some cases, that's
> fine, because the feature is useful on its own merits whether it ends
> up being part of the castle or not.
>

Uh. Right now this patch has been written because it's needed for a out
of core replication solution. That's what BDR is at this point. The
patch is unobtrusive, has other usecases than just our internal one and
doesn't make pg_resetxlog even more dangerous than it already is. I
don't see much problem with considering it on it's own cost/benefit?

So this seems to be a concern that's relatively independent of this
patch. Am I seing that right?

I think one very important point here is that BDR is *not* the proposed
in core solution. I think a reasonable community perspective - besides
also being useful on it's own - is to view it as a *prototype* for a in
core solution. And e.g. logical decoding would have looked much worse -
and likely not have been integrated - without externally already being
used for BDR.

I'm not sure how we can ease or even resolve your conerns when talking
about pretty independent and general pieces of functionality like the
DDL even trigger stuff. We needed to actually *write* those to see how
BDR will look like. And the communities feedback heavily influenced how
BDR looks like by accepting some pieces, demanding others, and outright
rejecting the remainder.

I think there's some pieces that need to consider them on their own
merit. Logical decoding is useful on it's own. The ability for out of
core systems to do DDL replication is another piece (that you referred
to above).
I think the likelihood of success if we were to try to design a in-core
system from ground up first and then follow through prety exactly along
those lines is minimal.

So, what I think we can do is to continue trying to build independent,
generally useful bits. Which imo all the stuff that's been integrated
is. Then, somewhat soon I think, we'll have to come up with a proposal
how the parts that are *not* necessarily useful outside of in-core
logical rep. might look like. Which will likely trigger some long long
discussions that turn that design around a couple of times. Which is
fine. I *don't* think that's going to be a trimmed down version of
todays BDR.

> But in other cases, like this one, if the premise that the slot name
> should match the system identifier isn't something the community wants
> to accept, then taking a patch that lets people do that is probably a
> bad idea, because at least one person will use it to set the system
> identifier of a system to a value that enables physical replication to
> take place when that is actually totally unsafe, and we don't want to
> enable that for no reason.

It also allows many other dangerous things. Many of which are much more
dangerous than changing the system identifier. Resetting an independent
cluster is also not very likely to work - the LSNs would still not
match. But it wouldn't corrupt the copy of the database that's been
changed...

> Maybe the slot name should match the
> replication identifier rather than the standby system ID, for example.
> There are conflicting proposals for how replication identifiers should
> work, but one of those proposals limits it to 16 bits.

I actually don't think any of the discussions I was involved in had the
externally visible version of replication identifiers limited to 16bits?
If you are referring to my patch, 16bits was just the width of the
*internal* name that should basically never be looked at. User visible
replication identifiers are always identified by an arbitrary string -
whose format is determined by the user of the replication identifier
facility. *BDR* currently stores the system identifer, the database id
and a name in there - but that's nothing core needs to concern itself
with.

Greetings,

Andres Freund

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

On Tue, Jun 17, 2014 at 12:50 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> >> I can clearly understand the utility of being able to reset the system
>> >> ID to a new, randomly-generated system ID - but giving the user the
>> >> ability to set a particular value of their own choosing seems like a
>> >> pretty sharp tool. What is the use case for that?
>
> I've previously hacked this up adhoc during data recovery when I needed
> to make another cluster similar enough that I could replay WAL.
>
> Another usecase is to mark a database as independent from its
> origin. Imagine a database that gets sharded across several
> servers. It's not uncommon to do that by initially basebackup'ing the
> database to several nodes and then use them separately from
> thereon. It's quite useful to actually mark them as being
> distinct. Especially as several of them right now would end up with the
> same timeline id...

Sure, but that only requires being able to reset the ID randomly, not
to a particular value.

>> But it seems to me that we might need to have a process discussion
>> here, because, while I'm all in favor of incremental feature proposals
>> that build towards a larger goal, it currently appears that the larger
>> goal toward which you are building is not something that's been
>> publicly discussed and debated on this list. And I really think we
>> need to have that conversation. Obviously, individual patches will
>> still need to be debated, but I feel like 2ndQuadrant is trying to
>> construct a castle without showing the community the floor plan. I
>> believe that there is relatively broad agreement that we would all
>> like a castle, but different people may have legitimately different
>> ideas about how it should be constructed. If the work arrives as a
>> series of disconnected pieces (user-specified system ID, event
>> triggers for CREATE, etc.), then everyone outside of 2ndQuadrant has
>> to take it on faith that those pieces are going to eventually fit
>> together in a way that we'll all be happy with. In some cases, that's
>> fine, because the feature is useful on its own merits whether it ends
>> up being part of the castle or not.
>>
>
> Uh. Right now this patch has been written because it's needed for a out
> of core replication solution. That's what BDR is at this point. The
> patch is unobtrusive, has other usecases than just our internal one and
> doesn't make pg_resetxlog even more dangerous than it already is. I
> don't see much problem with considering it on it's own cost/benefit?

Well, I think it *does* make pg_resetxlog more dangerous; see previous
discussion of pg_computemaxlsn.

> So this seems to be a concern that's relatively independent of this
> patch. Am I seing that right?

Partially; not completely.

> I actually don't think any of the discussions I was involved in had the
> externally visible version of replication identifiers limited to 16bits?
> If you are referring to my patch, 16bits was just the width of the
> *internal* name that should basically never be looked at. User visible
> replication identifiers are always identified by an arbitrary string -
> whose format is determined by the user of the replication identifier
> facility. *BDR* currently stores the system identifer, the database id
> and a name in there - but that's nothing core needs to concern itself
> with.

I don't think you're going to be able to avoid users needing to know
about those IDs. The configuration table is going to have to be the
same on all nodes, and how are you going to get that set up without
those IDs being user-visible?

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

On 2014-06-18 12:36:13 -0400, Robert Haas wrote:
> > I actually don't think any of the discussions I was involved in had the
> > externally visible version of replication identifiers limited to 16bits?
> > If you are referring to my patch, 16bits was just the width of the
> > *internal* name that should basically never be looked at. User visible
> > replication identifiers are always identified by an arbitrary string -
> > whose format is determined by the user of the replication identifier
> > facility. *BDR* currently stores the system identifer, the database id
> > and a name in there - but that's nothing core needs to concern itself
> > with.
>
> I don't think you're going to be able to avoid users needing to know
> about those IDs. The configuration table is going to have to be the
> same on all nodes, and how are you going to get that set up without
> those IDs being user-visible?

Why? Users and other systems only ever see the external ID. Everything
leaving the system is converted to the external form. The short id
basically is only used in shared memory and in wal records. For both
using longer strings would be problematic.

In the patch I have the user can actually see them as they're stored in
pg_replication_identifier, but there should never be a need for that.

Greetings,

Andres Freund

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

On 2014-06-18 12:36:13 -0400, Robert Haas wrote:
> On Tue, Jun 17, 2014 at 12:50 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> >> I can clearly understand the utility of being able to reset the system
> >> >> ID to a new, randomly-generated system ID - but giving the user the
> >> >> ability to set a particular value of their own choosing seems like a
> >> >> pretty sharp tool. What is the use case for that?
> >
> > I've previously hacked this up adhoc during data recovery when I needed
> > to make another cluster similar enough that I could replay WAL.
> >
> > Another usecase is to mark a database as independent from its
> > origin. Imagine a database that gets sharded across several
> > servers. It's not uncommon to do that by initially basebackup'ing the
> > database to several nodes and then use them separately from
> > thereon. It's quite useful to actually mark them as being
> > distinct. Especially as several of them right now would end up with the
> > same timeline id...
>
> Sure, but that only requires being able to reset the ID randomly, not
> to a particular value.

I can definitely see a point in a version of the option that generates
the id randomly. But the use case one up actually does require setting
it to a s specific value...

> > Uh. Right now this patch has been written because it's needed for a out
> > of core replication solution. That's what BDR is at this point. The
> > patch is unobtrusive, has other usecases than just our internal one and
> > doesn't make pg_resetxlog even more dangerous than it already is. I
> > don't see much problem with considering it on it's own cost/benefit?
>
> Well, I think it *does* make pg_resetxlog more dangerous; see previous
> discussion of pg_computemaxlsn.

Wasn't the thing around pg_computemaxlsn that there's actually no case
where it could be used safely? And that experienced people suggested to
use it an unsafe fashion?
I don't see how the proposed ability makes it more dangerous. It
*already* has the ability to reset the timelineid. That's the case where
users are much more likely to think about resetting it because that's
much more plausible than taking a unrelated cluster and resetting its
sysid, timeline and LSN.

Greetings,

Andres Freund

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

On 2014-06-18 18:54:05 +0200, Andres Freund wrote:
> On 2014-06-18 12:36:13 -0400, Robert Haas wrote:
> > Sure, but that only requires being able to reset the ID randomly, not
> > to a particular value.
>
> I can definitely see a point in a version of the option that generates
> the id randomly.

That's actually included in the patch btw (thanks Abhijit)...

Greetings,

Andres Freund

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

On Wed, Jun 18, 2014 at 12:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Well, I think it *does* make pg_resetxlog more dangerous; see previous
>> discussion of pg_computemaxlsn.
>
> Wasn't the thing around pg_computemaxlsn that there's actually no case
> where it could be used safely? And that experienced people suggested to
> use it an unsafe fashion?

There is a use case - to determine whether WAL has been replayed "from
the future" relative to the WAL stream and control file you have on
disk. But the rest is true enough.

> I don't see how the proposed ability makes it more dangerous. It
> *already* has the ability to reset the timelineid. That's the case where
> users are much more likely to think about resetting it because that's
> much more plausible than taking a unrelated cluster and resetting its
> sysid, timeline and LSN.

All right, well, I've said my piece. I don't have anything to add to
that that isn't sheer repetition. My vote is to hold off on this
until we've talked about replication identifiers and other related
topics in more depth. But if that position doesn't garner majority
support ... so be it!

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

On 18/06/14 19:26, Robert Haas wrote:
> On Wed, Jun 18, 2014 at 12:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I don't see how the proposed ability makes it more dangerous. It
>> *already* has the ability to reset the timelineid. That's the case where
>> users are much more likely to think about resetting it because that's
>> much more plausible than taking a unrelated cluster and resetting its
>> sysid, timeline and LSN.
>
> All right, well, I've said my piece. I don't have anything to add to
> that that isn't sheer repetition. My vote is to hold off on this
> until we've talked about replication identifiers and other related
> topics in more depth. But if that position doesn't garner majority
> support ... so be it!
>

I am not sure I get what does this have to do with replication
identifiers. The patch has several use-cases, one of them has to do that
you can know the future system id before you set it, which is useful for
automating some things...

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

On 2014-06-18 13:26:37 -0400, Robert Haas wrote:
> My vote is to hold off on this until we've talked about replication
> identifiers and other related topics in more depth.

I really don't understand this. We're *NOT* proposing this patch as an
underhanded way of preempting the discussion of whether/how replication
identifiers are going to be used. We're proposing it because we
currently have a need for the facility and this will reduce the number
of patches we have to keep around after 9.5. And more importantly
because there's several other use cases than our internal one for it.

To settle one more point: I am not planning to propose BDR's generation
of replication identifier names for integration. It works well enough
for BDR but I think we can come up with something better for core. If I
had my current knowledge two years back I'd not have chosen the current
scheme.

Greetings,

Andres Freund

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

On 06/13/2014 05:31 PM, Petr Jelinek wrote:
> Hello,
>
> attached is a simple patch which makes it possible to change the system
> identifier of the cluster in pg_control. This is useful for
> individualization of the instance that is started on top of data
> directory produced by pg_basebackup - something that's helpful for
> logical replication setup where you need to easily identify each node
> (it's used by Bidirectional Replication for example).

I'm unclear on why we would overload pg_resetxlog for this. Wouldn't it
be better design to have an independant function,
"pg_set_system_identifier"?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus wrote:
> On 06/13/2014 05:31 PM, Petr Jelinek wrote:
> > Hello,
> >
> > attached is a simple patch which makes it possible to change the system
> > identifier of the cluster in pg_control. This is useful for
> > individualization of the instance that is started on top of data
> > directory produced by pg_basebackup - something that's helpful for
> > logical replication setup where you need to easily identify each node
> > (it's used by Bidirectional Replication for example).
>
> I'm unclear on why we would overload pg_resetxlog for this. Wouldn't it
> be better design to have an independant function,
> "pg_set_system_identifier"?

We have overloaded pg_resetxlog for all pg_control-tweaking needs. I
don't see any reason to do differently here.

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

On 2014-06-18 10:44:56 -0700, Josh Berkus wrote:
> On 06/13/2014 05:31 PM, Petr Jelinek wrote:
> > Hello,
> >
> > attached is a simple patch which makes it possible to change the system
> > identifier of the cluster in pg_control. This is useful for
> > individualization of the instance that is started on top of data
> > directory produced by pg_basebackup - something that's helpful for
> > logical replication setup where you need to easily identify each node
> > (it's used by Bidirectional Replication for example).
>
> I'm unclear on why we would overload pg_resetxlog for this. Wouldn't it
> be better design to have an independant function,
> "pg_set_system_identifier"?

You mean an independent binary? Because it's not possible to change this
at runtime.

If so, it's because pg_resetxlog already has the option to change many
related things (e.g. the timeline id). And it'd require another copy of
several hundred lines of code. It's all stored in the control file/checkpoints.

Greetings,

Andres Freund

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

At 2014-06-18 10:44:56 -0700, josh(at)agliodbs(dot)com wrote:
>
> I'm unclear on why we would overload pg_resetxlog for this.

Because pg_resetxlog already does something very similar, so the patch
is small. If it were independent, it would have to copy quite some code
from pg_resetxlog.

> Wouldn't it be better design to have an independant function,
> "pg_set_system_identifier"?

A *function*? Why?

-- Abhijit

On 06/18/2014 10:48 AM, Abhijit Menon-Sen wrote:
> At 2014-06-18 10:44:56 -0700, josh(at)agliodbs(dot)com wrote:
>>
>> I'm unclear on why we would overload pg_resetxlog for this.
>
> Because pg_resetxlog already does something very similar, so the patch
> is small. If it were independent, it would have to copy quite some code
> from pg_resetxlog.

Aha. In that case, it seems like it's time to rename pg_resetxlog, if
it does a bunch of things that aren't resetting the xlog.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2014-06-18 10:59:59 -0700, Josh Berkus wrote:
> On 06/18/2014 10:48 AM, Abhijit Menon-Sen wrote:
> > At 2014-06-18 10:44:56 -0700, josh(at)agliodbs(dot)com wrote:
> >>
> >> I'm unclear on why we would overload pg_resetxlog for this.
> >
> > Because pg_resetxlog already does something very similar, so the patch
> > is small. If it were independent, it would have to copy quite some code
> > from pg_resetxlog.
>
> Aha. In that case, it seems like it's time to rename pg_resetxlog, if
> it does a bunch of things that aren't resetting the xlog.

Well, all those actually do write to the xlog (to write a new
checkpoint, containing the updated control file). Since pg_resetxlog has
done all this pretty much since forever renaming it now seems to be a
big hassle for users for pretty much no benefit? This isn't a tool the
average user should ever touch.

Greetings,

Andres Freund

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

On 06/18/2014 11:03 AM, Andres Freund wrote:
> Well, all those actually do write to the xlog (to write a new
> checkpoint, containing the updated control file). Since pg_resetxlog has
> done all this pretty much since forever renaming it now seems to be a
> big hassle for users for pretty much no benefit? This isn't a tool the
> average user should ever touch.

If we're using it to create a unique system ID which can be used to
orchestrate replication and clustering systems, a lot more people are
going to be touching it than ever did before -- and not just for BDR.

Or are you saying that we have to destroy the data by resetting the xlog
before we can change the system identifier? If so, this feature is less
than completely useful ...

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Jun 18, 2014 at 12:46 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-06-18 12:36:13 -0400, Robert Haas wrote:
>> > I actually don't think any of the discussions I was involved in had the
>> > externally visible version of replication identifiers limited to 16bits?
>> > If you are referring to my patch, 16bits was just the width of the
>> > *internal* name that should basically never be looked at. User visible
>> > replication identifiers are always identified by an arbitrary string -
>> > whose format is determined by the user of the replication identifier
>> > facility. *BDR* currently stores the system identifer, the database id
>> > and a name in there - but that's nothing core needs to concern itself
>> > with.
>>
>> I don't think you're going to be able to avoid users needing to know
>> about those IDs. The configuration table is going to have to be the
>> same on all nodes, and how are you going to get that set up without
>> those IDs being user-visible?
>
> Why? Users and other systems only ever see the external ID. Everything
> leaving the system is converted to the external form. The short id
> basically is only used in shared memory and in wal records. For both
> using longer strings would be problematic.
>
> In the patch I have the user can actually see them as they're stored in
> pg_replication_identifier, but there should never be a need for that.

Hmm, so there's no requirement that the short IDs are consistent
across different clusters that are replication to each other? If
that's the case, that might address my concern, but I'd probably want
to go back through the latest patch and think about it a bit more.

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

On 2014-06-23 10:09:49 -0400, Robert Haas wrote:
> On Wed, Jun 18, 2014 at 12:46 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-06-18 12:36:13 -0400, Robert Haas wrote:
> >> > I actually don't think any of the discussions I was involved in had the
> >> > externally visible version of replication identifiers limited to 16bits?
> >> > If you are referring to my patch, 16bits was just the width of the
> >> > *internal* name that should basically never be looked at. User visible
> >> > replication identifiers are always identified by an arbitrary string -
> >> > whose format is determined by the user of the replication identifier
> >> > facility. *BDR* currently stores the system identifer, the database id
> >> > and a name in there - but that's nothing core needs to concern itself
> >> > with.
> >>
> >> I don't think you're going to be able to avoid users needing to know
> >> about those IDs. The configuration table is going to have to be the
> >> same on all nodes, and how are you going to get that set up without
> >> those IDs being user-visible?
> >
> > Why? Users and other systems only ever see the external ID. Everything
> > leaving the system is converted to the external form. The short id
> > basically is only used in shared memory and in wal records. For both
> > using longer strings would be problematic.
> >
> > In the patch I have the user can actually see them as they're stored in
> > pg_replication_identifier, but there should never be a need for that.
>
> Hmm, so there's no requirement that the short IDs are consistent
> across different clusters that are replication to each other?

Nope. That seemed to be a hard requirement in the earlier discussions we
had (~2 years ago).

> If
> that's the case, that might address my concern, but I'd probably want
> to go back through the latest patch and think about it a bit more.

I'll send out a new version after I'm finished with the newest atomic
ops patch.

Greetings,

Andres Freund

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

On Mon, Jun 23, 2014 at 10:11 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > Why? Users and other systems only ever see the external ID. Everything
>> > leaving the system is converted to the external form. The short id
>> > basically is only used in shared memory and in wal records. For both
>> > using longer strings would be problematic.
>> >
>> > In the patch I have the user can actually see them as they're stored in
>> > pg_replication_identifier, but there should never be a need for that.
>>
>> Hmm, so there's no requirement that the short IDs are consistent
>> across different clusters that are replication to each other?
>
> Nope. That seemed to be a hard requirement in the earlier discussions we
> had (~2 years ago).

Oh, great. Somehow I missed the fact that that had been addressed. I
had assumed that we still needed global identifiers in which case I
think they'd need to be 64+ bits (preferably more like 128). If they
only need to be locally significant that makes things much better.

Is there any real reason to add a pg_replication_identifier table, or
should we just let individual replication solutions manage the
identifiers within their own configuration tables? I guess one
question is: What happens if there are multiple replication solutions
in use on a single server? How do they coordinate?

>> If
>> that's the case, that might address my concern, but I'd probably want
>> to go back through the latest patch and think about it a bit more.
>
> I'll send out a new version after I'm finished with the newest atomic
> ops patch.

Sweet. I'm a little backed up right now, but will look at it when able.

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

On 2014-06-23 10:45:51 -0400, Robert Haas wrote:
> On Mon, Jun 23, 2014 at 10:11 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> > Why? Users and other systems only ever see the external ID. Everything
> >> > leaving the system is converted to the external form. The short id
> >> > basically is only used in shared memory and in wal records. For both
> >> > using longer strings would be problematic.
> >> >
> >> > In the patch I have the user can actually see them as they're stored in
> >> > pg_replication_identifier, but there should never be a need for that.
> >>
> >> Hmm, so there's no requirement that the short IDs are consistent
> >> across different clusters that are replication to each other?
> >
> > Nope. That seemed to be a hard requirement in the earlier discussions we
> > had (~2 years ago).
>
> Oh, great. Somehow I missed the fact that that had been addressed. I
> had assumed that we still needed global identifiers in which case I
> think they'd need to be 64+ bits (preferably more like 128). If they
> only need to be locally significant that makes things much better.

Well, I was just talking about the 'short ids' here and how they are
used in crash recovery/shmem et al. Those indeed don't need to be
coordinated.
If you ever use logical decoding on a system that receives changes from
other systems (cascading replication, multimaster) you'll likely want to
add the *long* form of that identifier to the output in the output
plugin so the downstream nodes can identify the source. How one
specific replication solution deals with coordinating this between
systems is essentially that suite's problem.

The external identifier currently is a 'text' column, so essentially
unlimited. (Well, I just noticed that the table currently doesn't have a
toast table assigned, so it's only a couple kb right now, but ...)

> Is there any real reason to add a pg_replication_identifier table, or
> should we just let individual replication solutions manage the
> identifiers within their own configuration tables?

I don't think that'd work. During crash recovery the short/internal IDs
are read from WAL records and need to be unique across *all*
databases. Since there's no way for different replication solutions or
even the same to coordinate this across databases (as there's no way to
add shared relations) it has to be builtin.

It's also useful so we can have stuff like the
'pg_replication_identifier_progress' view which tells you internal_id,
external_id, remote_lsn, local_lsn. Just showing the internal ID would
imo be bad.

> I guess one
> question is: What happens if there are multiple replication solutions
> in use on a single server? How do they coordinate?

What's your concern here? You're wondering how they can make sure the
identifiers they create are non-overlapping?

Greetings,

Andres Freund

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

On Mon, Jun 23, 2014 at 11:28 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Oh, great. Somehow I missed the fact that that had been addressed. I
>> had assumed that we still needed global identifiers in which case I
>> think they'd need to be 64+ bits (preferably more like 128). If they
>> only need to be locally significant that makes things much better.
>
> Well, I was just talking about the 'short ids' here and how they are
> used in crash recovery/shmem et al. Those indeed don't need to be
> coordinated.
> If you ever use logical decoding on a system that receives changes from
> other systems (cascading replication, multimaster) you'll likely want to
> add the *long* form of that identifier to the output in the output
> plugin so the downstream nodes can identify the source. How one
> specific replication solution deals with coordinating this between
> systems is essentially that suite's problem.

OK.

> The external identifier currently is a 'text' column, so essentially
> unlimited. (Well, I just noticed that the table currently doesn't have a
> toast table assigned, so it's only a couple kb right now, but ...)

OK. I have no clear reason to dislike that.

>> Is there any real reason to add a pg_replication_identifier table, or
>> should we just let individual replication solutions manage the
>> identifiers within their own configuration tables?
>
> I don't think that'd work. During crash recovery the short/internal IDs
> are read from WAL records and need to be unique across *all*
> databases. Since there's no way for different replication solutions or
> even the same to coordinate this across databases (as there's no way to
> add shared relations) it has to be builtin.

That makes sense.

> It's also useful so we can have stuff like the
> 'pg_replication_identifier_progress' view which tells you internal_id,
> external_id, remote_lsn, local_lsn. Just showing the internal ID would
> imo be bad.

OK.

>> I guess one
>> question is: What happens if there are multiple replication solutions
>> in use on a single server? How do they coordinate?
>
> What's your concern here? You're wondering how they can make sure the
> identifiers they create are non-overlapping?

Yeah, I was just thinking that might be why you installed a catalog
table for this, but now I see that there are several other reasons
also.

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

Hi,

I send you review comment about thie patch.

I found no error/warning with compling and installation.
I have executed pg_resetxlog with some input pattern.

$ initdb -D data -E UTF8 --no-locale
$ pg_controldata data | grep "Database system identifier"
Database system identifier: 6028907917695471865

--
$ pg_resetxlog -s -n data | grep "Database system identifier"
Database system identifier: 6028907917695471865

The -s option does not works fine with -n option.

--
$ pg_resetxlog
-s6028907917695471865111111111111111111111111111111111111111111111111111111
data
Transaction log reset
$ pg_controldata data | grep "Database system identifier"
Database system identifier: 18446744073709551615

pg_resetxlog is finished successfully, but system identifier was not
changed.
Also I think that checking data about number of digits is needed.

regards
--
Sawada Masahiko

On Thursday, June 19, 2014, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> On 18/06/14 19:26, Robert Haas wrote:
>
>> On Wed, Jun 18, 2014 at 12:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
>> wrote:
>>
>>> I don't see how the proposed ability makes it more dangerous. It
>>> *already* has the ability to reset the timelineid. That's the case where
>>> users are much more likely to think about resetting it because that's
>>> much more plausible than taking a unrelated cluster and resetting its
>>> sysid, timeline and LSN.
>>>
>>
>> All right, well, I've said my piece. I don't have anything to add to
>> that that isn't sheer repetition. My vote is to hold off on this
>> until we've talked about replication identifiers and other related
>> topics in more depth. But if that position doesn't garner majority
>> support ... so be it!
>>
>>
> I am not sure I get what does this have to do with replication
> identifiers. The patch has several use-cases, one of them has to do that
> you can know the future system id before you set it, which is useful for
> automating some things...
>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
Regards,

-------
Sawada Masahiko

On Thu, Jun 26, 2014 at 2:43 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> Hi,
>
> I send you review comment about thie patch.
>
> I found no error/warning with compling and installation.
> I have executed pg_resetxlog with some input pattern.
>
> $ initdb -D data -E UTF8 --no-locale
> $ pg_controldata data | grep "Database system identifier"
> Database system identifier: 6028907917695471865
>
> --
> $ pg_resetxlog -s -n data | grep "Database system identifier"
> Database system identifier: 6028907917695471865
>
> The -s option does not works fine with -n option.
>
> --
> $ pg_resetxlog
> -s6028907917695471865111111111111111111111111111111111111111111111111111111
> data
> Transaction log reset
> $ pg_controldata data | grep "Database system identifier"
> Database system identifier: 18446744073709551615
>
> pg_resetxlog is finished successfully, but system identifier was not
> changed.
> Also I think that checking data about number of digits is needed.
>

Yep, system_identifier is a uint64, and the input you are giving here is
incompatible with that.
--
Michael

On 25/06/14 19:43, Sawada Masahiko wrote:
> Hi,
>
> I send you review comment about thie patch.

Hello, thanks.

> --
> $ pg_resetxlog -s -n data | grep "Database system identifier"
> Database system identifier: 6028907917695471865
>
> The -s option does not worksfine with -n option.

Fixed.

> --
> $ pg_resetxlog
> -s6028907917695471865111111111111111111111111111111111111111111111111111111
> data
> Transaction log reset
> $ pg_controldata data | grep "Database system identifier"
> Database system identifier: 18446744073709551615
>
> pg_resetxlog is finished successfully, but system identifier was not
> changed.
> Also I think that checking data about number of digits is needed.
>

It actually did change the identifier, just to ULONG_MAX, since that's
the maximum value that can be set (scanf does that conversion), I added
check that limits the maximum value of system identifier input to
ULONG_MAX-1 and reports error if it's bigger. I also added some
additional input validation.

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

Thank you for updating the patch.
I think that the following behaviour of pg_resetxlog is bug.

$ pg_controldata data | grep "Database system identifier"
Database system identifier: 6029284919152642525

--
$ pg_resetxlog -s0 -n data
Current pg_control values:

pg_control version number: 942
Catalog version number: 201406181
Database system identifier: 6029284919152642525
Latest checkpoint's TimeLineID: 1
Latest checkpoint's full_page_writes: on
Latest checkpoint's NextXID: 0/1810
Latest checkpoint's NextOID: 13004
Latest checkpoint's NextMultiXactId: 1
Latest checkpoint's NextMultiOffset: 0
Latest checkpoint's oldestXID: 1800
Latest checkpoint's oldestXID's DB: 1
Latest checkpoint's oldestActiveXID: 0
Latest checkpoint's oldestMultiXid: 1
Latest checkpoint's oldestMulti's DB: 1
Maximum data alignment: 8
Database block size: 8192
Blocks per segment of large relation: 131072
WAL block size: 8192
Bytes per WAL segment: 16777216
Maximum length of identifiers: 64
Maximum columns in an index: 32
Maximum size of a TOAST chunk: 1996
Size of a large-object chunk: 2048
Date/time type storage: 64-bit integers
Float4 argument passing: by value
Float8 argument passing: by value
Data page checksum version: 0

Values to be changed:

First log segment after reset: 000000010000000000000002

--
$ pg_resetxlog -s0 data
Transaction log reset
$ pg_controldata data | grep "Database system identifier"
Database system identifier: 6029284919152642525

this patch dose not works fine with -s0.

Regards,
--
Sawada Masahiko

On Thursday, June 26, 2014, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> On 25/06/14 19:43, Sawada Masahiko wrote:
>
>> Hi,
>>
>> I send you review comment about thie patch.
>>
>
> Hello, thanks.
>
> --
>> $ pg_resetxlog -s -n data | grep "Database system identifier"
>> Database system identifier: 6028907917695471865
>>
>> The -s option does not worksfine with -n option.
>>
>
> Fixed.
>
> --
>> $ pg_resetxlog
>> -s60289079176954718651111111111111111111111111111111111111111
>> 11111111111111
>> data
>> Transaction log reset
>> $ pg_controldata data | grep "Database system identifier"
>> Database system identifier: 18446744073709551615
>>
>> pg_resetxlog is finished successfully, but system identifier was not
>> changed.
>> Also I think that checking data about number of digits is needed.
>>
>>
> It actually did change the identifier, just to ULONG_MAX, since that's the
> maximum value that can be set (scanf does that conversion), I added check
> that limits the maximum value of system identifier input to ULONG_MAX-1 and
> reports error if it's bigger. I also added some additional input validation.
>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

--
Regards,

-------
Sawada Masahiko

On 26/06/14 19:57, Sawada Masahiko wrote:
> $ pg_resetxlog -s0 data
> Transaction log reset
> $ pg_controldata data | grep "Database system identifier"
> Database system identifier: 6029284919152642525
>
> this patch dose not works fine with -s0.
>

Yes, this is a bug, 0 input should throw error, which it does now.

Also based on Alvaro's comment, I replaced the scanf parsing code with
strtoul(l) function.

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

At 2014-06-27 00:51:02 +0200, petr(at)2ndquadrant(dot)com wrote:
>
> Also based on Alvaro's comment, I replaced the scanf parsing code with
> strtoul(l) function.

As far as I can tell (from the thread and a quick look at the patch),
your latest version of the patch addresses all the review comments.

Should I mark this ready for committer now?

-- Abhijit

On 2014-06-29 19:44:21 +0530, Abhijit Menon-Sen wrote:
> At 2014-06-27 00:51:02 +0200, petr(at)2ndquadrant(dot)com wrote:
> >
> > Also based on Alvaro's comment, I replaced the scanf parsing code with
> > strtoul(l) function.
>
> As far as I can tell (from the thread and a quick look at the patch),
> your latest version of the patch addresses all the review comments.
>
> Should I mark this ready for committer now?

Well, we haven't really agreed on a way forward yet. Robert disagrees
that the feature is independently useful and thinks it might be
dangerous for some users. I don't agree with either of those points, but
that doesn't absolve the patch from the objection. I think tentatively
have been more people in favor, but it's not clear cut imo.

Greetings,

Andres Freund

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

On Sun, Jun 29, 2014 at 11:18 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-06-29 19:44:21 +0530, Abhijit Menon-Sen wrote:
>> At 2014-06-27 00:51:02 +0200, petr(at)2ndquadrant(dot)com wrote:
>> >
>> > Also based on Alvaro's comment, I replaced the scanf parsing code with
>> > strtoul(l) function.
>>
>> As far as I can tell (from the thread and a quick look at the patch),
>> your latest version of the patch addresses all the review comments.
>>
>> Should I mark this ready for committer now?
>
> Well, we haven't really agreed on a way forward yet. Robert disagrees
> that the feature is independently useful and thinks it might be
> dangerous for some users. I don't agree with either of those points, but
> that doesn't absolve the patch from the objection. I think tentatively
> have been more people in favor, but it's not clear cut imo.

So what's the usecase of this feature? If it's for "normal operation",
using pg_resetxlog for that is a bit dangerous because it can corrupt
the database easily.

Regards,

--
Fujii Masao

On 2014-06-30 19:57:58 +0900, Fujii Masao wrote:
> On Sun, Jun 29, 2014 at 11:18 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-06-29 19:44:21 +0530, Abhijit Menon-Sen wrote:
> >> At 2014-06-27 00:51:02 +0200, petr(at)2ndquadrant(dot)com wrote:
> >> >
> >> > Also based on Alvaro's comment, I replaced the scanf parsing code with
> >> > strtoul(l) function.
> >>
> >> As far as I can tell (from the thread and a quick look at the patch),
> >> your latest version of the patch addresses all the review comments.
> >>
> >> Should I mark this ready for committer now?
> >
> > Well, we haven't really agreed on a way forward yet. Robert disagrees
> > that the feature is independently useful and thinks it might be
> > dangerous for some users. I don't agree with either of those points, but
> > that doesn't absolve the patch from the objection. I think tentatively
> > have been more people in favor, but it's not clear cut imo.
>
> So what's the usecase of this feature? If it's for "normal operation",
> using pg_resetxlog for that is a bit dangerous because it can corrupt
> the database easily.

a) Mark a database as not being the same. Currently if you promote two
databases, e.g. to shard out, they'll continue to have same system
identifier. Which really sucks, especially because timeline ids will
often increase synchronously.

b) For data recovery it's sometimes useful to create a new database
(with the same catalog state) and replay all WAL. For that you need to
reset the system identifier. I've done so hacking up resetxlog
before.

c) We already allow to set pretty much all aspects of the control file
via resetxlog - there seems little point of not having the ability to
change the system identifier.

d) In a logical replication scenario one way to identify individual
nodes is via the system identifier. If you want to convert a
basebackup into logical standby one sensible way to do so is to
create a logical replication slots *before* promoting a physical
backup to guarantee that slot is able to stream out all changes. If
the slot names contain the consumer's system identifier you need to
know the new system identifier beforehand.

Greetings,

Andres Freund

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

Andres Freund wrote:

> c) We already allow to set pretty much all aspects of the control file
> via resetxlog - there seems little point of not having the ability to
> change the system identifier.

I think it's pretty much a given that pg_resetxlog is a tool that can
have disastrous effects if used lightly. If people changes their sysid
wrongly, they're not any worse than if they change their multixact
counters and start getting failures because the old values stored in
data cannot be resolved anymore ("it's already been wrapped around").
Or if they remove all the XLOG they have since the latest crash. From
that POV, I don't think the objection that "but this can be used to
corrupt data!" has any value.

Also on the other hand pg_resetxlog is already a tool for PG hackers to
fool around and test things. In a way, being able to change values in
pg_control is useful to many of us; this is only an extension of that.

If we only had bricks and mortar, I think we would have a tool to
display and tweak pg_control separately from emptying pg_xlog, rather
than this odd separation between pg_controldata and pg_resetxlog, each
of which do a mixture of those things. But we have a wall two thirds
done already, so it seems to make more sense to me to continue forward
rather than tear it down and start afresh. This patch is a natural
extension of what we already have.

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

On Mon, Jun 30, 2014 at 12:46 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> I think it's pretty much a given that pg_resetxlog is a tool that can
> have disastrous effects if used lightly. If people changes their sysid
> wrongly, they're not any worse than if they change their multixact
> counters and start getting failures because the old values stored in
> data cannot be resolved anymore ("it's already been wrapped around").
> Or if they remove all the XLOG they have since the latest crash. From
> that POV, I don't think the objection that "but this can be used to
> corrupt data!" has any value.

After thinking about this a little more, I guess I don't really think
it's a bit problem either - so consider my objection withdrawn.

I am, however, kind of frustrated, still, that the pg_computemaxlsn
patch, which I thought was rather a good idea, was scuttled by the
essentially that same objection: let's not extend pg_resetxlog &
friends because people might use the new functionality to do bad
things and then blame us.

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

Robert Haas wrote:
> On Mon, Jun 30, 2014 at 12:46 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > I think it's pretty much a given that pg_resetxlog is a tool that can
> > have disastrous effects if used lightly. If people changes their sysid
> > wrongly, they're not any worse than if they change their multixact
> > counters and start getting failures because the old values stored in
> > data cannot be resolved anymore ("it's already been wrapped around").
> > Or if they remove all the XLOG they have since the latest crash. From
> > that POV, I don't think the objection that "but this can be used to
> > corrupt data!" has any value.
>
> After thinking about this a little more, I guess I don't really think
> it's a bit problem either - so consider my objection withdrawn.

Great.

> I am, however, kind of frustrated, still, that the pg_computemaxlsn
> patch, which I thought was rather a good idea, was scuttled by the
> essentially that same objection: let's not extend pg_resetxlog &
> friends because people might use the new functionality to do bad
> things and then blame us.

Uh, I thought you killed that patch yourself:
http://www.postgresql.org/message-id/CA+TgmoZqbYHWYbi18FUk-+dGHig=icV+pj-NNav3miy4dajgrQ@mail.gmail.com

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

On 2014-07-01 11:11:12 -0400, Robert Haas wrote:
> On Mon, Jun 30, 2014 at 12:46 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > I think it's pretty much a given that pg_resetxlog is a tool that can
> > have disastrous effects if used lightly. If people changes their sysid
> > wrongly, they're not any worse than if they change their multixact
> > counters and start getting failures because the old values stored in
> > data cannot be resolved anymore ("it's already been wrapped around").
> > Or if they remove all the XLOG they have since the latest crash. From
> > that POV, I don't think the objection that "but this can be used to
> > corrupt data!" has any value.
>
> After thinking about this a little more, I guess I don't really think
> it's a bit problem either - so consider my objection withdrawn.

Thanks!

> I am, however, kind of frustrated, still, that the pg_computemaxlsn
> patch, which I thought was rather a good idea, was scuttled by the
> essentially that same objection: let's not extend pg_resetxlog &
> friends because people might use the new functionality to do bad
> things and then blame us.

Well, the reasons were a bit different. Senior community members
repeatedly suggested that it'd be usable for faillback - and it's not a
unreasonable to think it is. Even though it'd fail subtly because of
hint bit and related reasons.
In contrast you have to be pretty desperate to think that you could make
two clusters replicate from each other by just fudging pg_control long
enough, even if the clusters aren't actually related.

Greetings,

Andres Freund

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

On Tue, Jul 1, 2014 at 11:19 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I am, however, kind of frustrated, still, that the pg_computemaxlsn
>> patch, which I thought was rather a good idea, was scuttled by the
>> essentially that same objection: let's not extend pg_resetxlog &
>> friends because people might use the new functionality to do bad
>> things and then blame us.
>
> Well, the reasons were a bit different. Senior community members
> repeatedly suggested that it'd be usable for faillback - and it's not a
> unreasonable to think it is. Even though it'd fail subtly because of
> hint bit and related reasons.
> In contrast you have to be pretty desperate to think that you could make
> two clusters replicate from each other by just fudging pg_control long
> enough, even if the clusters aren't actually related.

Well, I still think it's pretty likely someone could make that mistake
here, too. Maybe not a senior community member, but somebody.
However, I think the right answer in that case and this one is to tell
the person who has made the mistake "you screwed up" and move on.

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

On 2014-06-27 00:51:02 +0200, Petr Jelinek wrote:
> - while ((c = getopt(argc, argv, "fl:m:no:O:x:e:")) != -1)
> + while ((c = getopt(argc, argv, "fl:m:no:O:x:e:s::")) != -1)

Why two :?

> {
> switch (c)
> {
> @@ -227,6 +229,33 @@ main(int argc, char *argv[])
> XLogFromFileName(optarg, &minXlogTli, &minXlogSegNo);
> break;
>
> + case 's':
> + if (optarg)
> + {
> +#ifdef HAVE_STRTOULL
> + set_sysid = strtoull(optarg, &endptr, 10);
> +#else
> + set_sysid = strtoul(optarg, &endptr, 10);
> +#endif

Wouldn't it be better to use sscanf()? That should work with large
inputs across platforms.

> + /*
> + * Validate input, we use strspn because strtoul(l) accepts
> + * negative numbers and silently converts them to unsigned
> + */
> + if (set_sysid == 0 || errno != 0 ||
> + endptr == optarg || *endptr != '\0' ||
> + strspn(optarg, "0123456789") != strlen(optarg))
> + {
> + fprintf(stderr, _("%s: invalid argument for option -s\n"), progname);
> + fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
> + exit(1);
> + }

Maybe: 'invalid argument \"%s\" ...'?

> @@ -1087,6 +1147,7 @@ usage(void)
> printf(_(" -o OID set next OID\n"));
> printf(_(" -O OFFSET set next multitransaction offset\n"));
> printf(_(" -V, --version output version information, then exit\n"));
> + printf(_(" -s [SYSID] set system identifier (or generate one)\n"));
> printf(_(" -x XID set next transaction ID\n"));
> printf(_(" -?, --help show this help, then exit\n"));
> printf(_("\nReport bugs to <pgsql-bugs(at)postgresql(dot)org>.\n"));

I think we usually try to keep the options roughly alphabetically
ordered. Same in the getopt() above.

Greetings,

Andres Freund

On 2014-07-18 00:41:05 +0200, Andres Freund wrote:
> On 2014-06-27 00:51:02 +0200, Petr Jelinek wrote:
> > - while ((c = getopt(argc, argv, "fl:m:no:O:x:e:")) != -1)
> > + while ((c = getopt(argc, argv, "fl:m:no:O:x:e:s::")) != -1)
>
> Why two :?

Obviously strike that, wanted to delete the paragraph, but forgot about
it. I hadn't remembered the autogeneration of sysids at the beginning of
the patch.

Greetings,

Andres Freund

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

On 06/30/2014 09:46 AM, Alvaro Herrera wrote:
> If we only had bricks and mortar, I think we would have a tool to
> display and tweak pg_control separately from emptying pg_xlog, rather
> than this odd separation between pg_controldata and pg_resetxlog, each
> of which do a mixture of those things. But we have a wall two thirds
> done already, so it seems to make more sense to me to continue forward
> rather than tear it down and start afresh. This patch is a natural
> extension of what we already have.

As I said previously, I disagree. Being able to set a system identifier
at runtime is useful for a variety of scenarios which do not involve
database surgery or the risk of wiping out your data; in fact, the only
bad thing you can do by changing the system id is break the replication
connection.

As such, tying a change in system id to pg_resetxlog is kind of like
having a combination dental floss dispenser and bazooka. "No, not
*that* trigger!"

It pretty much guarantees that the ability to change the systemid, which
could be a generally useful feature with all kinds of application for
HA, clusters and sharding, would remain an internal feature of BDR only,
because everyone else will be afraid to touch it.

If this means that we need to finally create pg_editcontroldata, then so
be it.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 18/07/14 00:41, Andres Freund wrote:
> On 2014-06-27 00:51:02 +0200, Petr Jelinek wrote:
>> {
>> switch (c)
>> {
>> @@ -227,6 +229,33 @@ main(int argc, char *argv[])
>> XLogFromFileName(optarg, &minXlogTli, &minXlogSegNo);
>> break;
>>
>> + case 's':
>> + if (optarg)
>> + {
>> +#ifdef HAVE_STRTOULL
>> + set_sysid = strtoull(optarg, &endptr, 10);
>> +#else
>> + set_sysid = strtoul(optarg, &endptr, 10);
>> +#endif
>
> Wouldn't it be better to use sscanf()? That should work with large
> inputs across platforms.
>

Well, sscanf does not do much validation and silently truncates too big
input (which is why I replaced it with strtoull, original patch did have
sscanf), also I think the portability of sscanf might not be as good,
see 9d7ded0f4277f5c0063eca8e871a34e2355a8371 commit.

>> + /*
>> + * Validate input, we use strspn because strtoul(l) accepts
>> + * negative numbers and silently converts them to unsigned
>> + */
>> + if (set_sysid == 0 || errno != 0 ||
>> + endptr == optarg || *endptr != '\0' ||
>> + strspn(optarg, "0123456789") != strlen(optarg))
>> + {
>> + fprintf(stderr, _("%s: invalid argument for option -s\n"), progname);
>> + fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
>> + exit(1);
>> + }
>
> Maybe: 'invalid argument \"%s\" ...'?
>

Ok

>> @@ -1087,6 +1147,7 @@ usage(void)
>> printf(_(" -o OID set next OID\n"));
>> printf(_(" -O OFFSET set next multitransaction offset\n"));
>> printf(_(" -V, --version output version information, then exit\n"));
>> + printf(_(" -s [SYSID] set system identifier (or generate one)\n"));
>> printf(_(" -x XID set next transaction ID\n"));
>> printf(_(" -?, --help show this help, then exit\n"));
>> printf(_("\nReport bugs to <pgsql-bugs(at)postgresql(dot)org>.\n"));
>
> I think we usually try to keep the options roughly alphabetically
> ordered. Same in the getopt() above.
>

Ok, attached v4 with those changes.

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

On 2014-07-18 13:08:24 +0200, Petr Jelinek wrote:
> On 18/07/14 00:41, Andres Freund wrote:
> >On 2014-06-27 00:51:02 +0200, Petr Jelinek wrote:
> >> {
> >> switch (c)
> >> {
> >>@@ -227,6 +229,33 @@ main(int argc, char *argv[])
> >> XLogFromFileName(optarg, &minXlogTli, &minXlogSegNo);
> >> break;
> >>
> >>+ case 's':
> >>+ if (optarg)
> >>+ {
> >>+#ifdef HAVE_STRTOULL
> >>+ set_sysid = strtoull(optarg, &endptr, 10);
> >>+#else
> >>+ set_sysid = strtoul(optarg, &endptr, 10);
> >>+#endif
> >
> >Wouldn't it be better to use sscanf()? That should work with large
> >inputs across platforms.
> >
>
> Well, sscanf does not do much validation and silently truncates too big
> input (which is why I replaced it with strtoull, original patch did have
> sscanf)

Well, the checks on length you've added should catch that when adapted
properly.

>, also I think the portability of sscanf might not be as good, see
> 9d7ded0f4277f5c0063eca8e871a34e2355a8371 commit.

Fair point. But I don't think the arguments why using strtoul instead of
strtoull is safe hold much water here. In the pg_stat_statement case the
worst that could happen is that the rowcount isn't accurate. Here it's
setting a wrong system identifier. Not really the same.

Maybe it's time to pursue
http://archives.postgresql.org/message-id/20140603144654.GM24145%40awork2.anarazel.de
further :(

Greetings,

Andres Freund

On 18/07/14 13:18, Andres Freund wrote:
> On 2014-07-18 13:08:24 +0200, Petr Jelinek wrote:
>> On 18/07/14 00:41, Andres Freund wrote:
>>> Wouldn't it be better to use sscanf()? That should work with large
>>> inputs across platforms.
>>>
>>
>> Well, sscanf does not do much validation and silently truncates too big
>> input (which is why I replaced it with strtoull, original patch did have
>> sscanf)
>
> Well, the checks on length you've added should catch that when adapted
> properly.

I don't see a portable way how to validate too big input values when
using sscanf.

>
> Maybe it's time to pursue
> http://archives.postgresql.org/message-id/20140603144654.GM24145%40awork2.anarazel.de
> further :(
>

Yes, I've seen that patch and was quite sad that it didn't make it in core.

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

On 06/18/2014 09:17 PM, Josh Berkus wrote:
> On 06/18/2014 11:03 AM, Andres Freund wrote:
>> Well, all those actually do write to the xlog (to write a new
>> checkpoint, containing the updated control file). Since pg_resetxlog has
>> done all this pretty much since forever renaming it now seems to be a
>> big hassle for users for pretty much no benefit? This isn't a tool the
>> average user should ever touch.
>
> If we're using it to create a unique system ID which can be used to
> orchestrate replication and clustering systems, a lot more people are
> going to be touching it than ever did before -- and not just for BDR.

I think pg_resetxlog is still appropriate: changing the system ID will
reset the WAL. In particular, any archived WAL will become useless.

But yeah, this does change the target audience of pg_resetxlog
significantly. Now that we'll have admins running pg_resetxlog as part
of normal operations, we have to document it carefully. We also have to
design the user interface carefully, to make it more clear that while
resetting the system ID won't eat your data, some of the other settings
will.

The proposed "pg_resetxlog --help" output looks like this:

> pg_resetxlog resets the PostgreSQL transaction log.
>
> Usage:
> pg_resetxlog [OPTION]... DATADIR
>
> Options:
> -e XIDEPOCH set next transaction ID epoch
> -f force update to be done
> -l XLOGFILE force minimum WAL starting location for new transaction log
> -m MXID,MXID set next and oldest multitransaction ID
> -n no update, just show what would be done (for testing)
> -o OID set next OID
> -O OFFSET set next multitransaction offset
> -s [SYSID] set system identifier (or generate one)
> -V, --version output version information, then exit
> -x XID set next transaction ID
> -?, --help show this help, then exit
>
> Report bugs to <pgsql-bugs(at)postgresql(dot)org>.

I don't think that's good enough. The -s option should be displayed more
prominently, and the dangerous options like -l and -x should be more
clearly labeled as such.

I would de-emphasize setting the system ID to a particular value. It
might be useful for disaster recovery, like -x, but in general you
should always reset it to a new unique value. If you make it too easy to
set it to a particular value, someone will try initialize a streaming
standby server using initdb+pg_dump, and changing the system ID to match
the master's.

The user manual for pg_resetxlog says:

> pg_resetxlog clears the write-ahead log (WAL) and optionally resets
> some other control information stored in the pg_control file. This
> function is sometimes needed if these files have become corrupted. It
> should be used only as a last resort, when the server will not start
> due to such corruption.

That's clearly not true for the -s option.

In summary, I think we want this feature in some form, but we'll somehow
need to be make the distinction to the dangerous pg_resetxlog usage. It
might be best, after all, to make this a separate utility,
pg_resetsystemid. It would not need to have the capability to set the
system ID to a particular value, only a randomly assigned one (setting
it to a particular value could be added to pg_resetxlog, where other
dangerous options are).

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> In summary, I think we want this feature in some form, but we'll somehow
> need to be make the distinction to the dangerous pg_resetxlog usage. It
> might be best, after all, to make this a separate utility,
> pg_resetsystemid.

That sounds fairly reasonable given your point about not wanting people to
confuse this with the can-eat-your-data aspects of pg_resetxlog. (OTOH,
won't this result in a lot of code duplication? We'd still need to erase
and refill the WAL area.)

> It would not need to have the capability to set the
> system ID to a particular value, only a randomly assigned one (setting
> it to a particular value could be added to pg_resetxlog, where other
> dangerous options are).

I'm less convinced about that. While you can shoot yourself in the foot
by assigning the same system ID to two installations that share WAL
archive or something like that, this feels a bit different than the ways
you can shoot yourself in the foot with pg_resetxlog. If we do what you
say here then I think we'll be right back to the discussion of how to
separate the assign-a-sysID option from pg_resetxlog's other, more
dangerous options.

regards, tom lane

On August 25, 2014 9:45:50 PM CEST, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> In summary, I think we want this feature in some form, but we'll
>somehow
>> need to be make the distinction to the dangerous pg_resetxlog usage.
>It
>> might be best, after all, to make this a separate utility,
>> pg_resetsystemid.
>
>That sounds fairly reasonable given your point about not wanting people
>to
>confuse this with the can-eat-your-data aspects of pg_resetxlog.
>(OTOH,
>won't this result in a lot of code duplication? We'd still need to
>erase
>and refill the WAL area.)

Let's move pg-controldata, resetxlog, resetsysid into a common src/bin directory. Then we can unify the relevant code between all three.

--
Please excuse brevity and formatting - I am writing this on my mobile phone.

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

On 08/25/2014 10:45 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> It would not need to have the capability to set the
>> system ID to a particular value, only a randomly assigned one (setting
>> it to a particular value could be added to pg_resetxlog, where other
>> dangerous options are).
>
> I'm less convinced about that. While you can shoot yourself in the foot
> by assigning the same system ID to two installations that share WAL
> archive or something like that, this feels a bit different than the ways
> you can shoot yourself in the foot with pg_resetxlog. If we do what you
> say here then I think we'll be right back to the discussion of how to
> separate the assign-a-sysID option from pg_resetxlog's other, more
> dangerous options.

I don't see the use case for setting system id to a particular value.
Andres listed four use cases upthread:

> a) Mark a database as not being the same. Currently if you promote two
> databases, e.g. to shard out, they'll continue to have same system
> identifier. Which really sucks, especially because timeline ids will
> often increase synchronously.

Yes, this is the legitimate use case a DBA would use this feature for.
Resetting the system ID to a random value suffices.

> b) For data recovery it's sometimes useful to create a new database
> (with the same catalog state) and replay all WAL. For that you need to
> reset the system identifier. I've done so hacking up resetxlog
> before.

This falls squarely in the "dangerous" category, and you'll have to
reset other things than the system ID to make it work. Having the option
in pg_resetxlog is fine for this.

> c) We already allow to set pretty much all aspects of the control file
> via resetxlog - there seems little point of not having the ability to
> change the system identifier.

Ok, but it's not something a regular admin would ever use.

> d) In a logical replication scenario one way to identify individual
> nodes is via the system identifier. If you want to convert a
> basebackup into logical standby one sensible way to do so is to
> create a logical replication slots *before* promoting a physical
> backup to guarantee that slot is able to stream out all changes. If
> the slot names contain the consumer's system identifier you need to
> know the new system identifier beforehand.

I didn't understand this one. But it seems like the obvious solution is
to not use the consumer's system identifier as the slot name. Or rename
it afterwards.

- Heikki

On Mon, Aug 25, 2014 at 4:06 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> I didn't understand this one. But it seems like the obvious solution is to
> not use the consumer's system identifier as the slot name. Or rename it
> afterwards.

You can't use the consumer's system identifier as the slot name,
because you have to create the slot before you create the consumer.
But you could rename it afterwards, or just use some other naming
convention entirely, which is why I'm -0.25 on this whole proposal.
What the 2ndQuadrant folks are proposing is not unreasonable (which is
why I'm only -0.25) but it opens an (admittedly small) can of worms
that I see no real need to open.

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