Обсуждение: Re: [COMMITTERS] pgsql: Send new protocol keepalive messages to standby servers.

On Sat, Dec 31, 2011 at 10:34 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Send new protocol keepalive messages to standby servers.
> Allows streaming replication users to calculate transfer latency
> and apply delay via internal functions. No external functions yet.

Is there plan to implement such external functions before 9.2 release?
If not, keepalive protocol seems to be almost useless because there is
no use of it for a user and the increase in the number of packets might
increase the replication performance overhead slightly. No?

Regards,

-- 
Fujii Masao

On Wed, May 23, 2012 at 2:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Sat, Dec 31, 2011 at 10:34 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Send new protocol keepalive messages to standby servers.
>> Allows streaming replication users to calculate transfer latency
>> and apply delay via internal functions. No external functions yet.
>
> Is there plan to implement such external functions before 9.2 release?
> If not, keepalive protocol seems to be almost useless because there is
> no use of it for a user and the increase in the number of packets might
> increase the replication performance overhead slightly. No?

Good point.  IMHO, this shouldn't really have been committed like
this, but since it was, we had better fix it, either by reverting the
change or forcing an initdb to expose the functionality.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 23, 2012 at 2:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Is there plan to implement such external functions before 9.2 release?
>> If not, keepalive protocol seems to be almost useless because there is
>> no use of it for a user and the increase in the number of packets might
>> increase the replication performance overhead slightly. No?

> Good point.  IMHO, this shouldn't really have been committed like
> this, but since it was, we had better fix it, either by reverting the
> change or forcing an initdb to expose the functionality.

I see no reason to rip the code out if we have plans to make use of it
in the near future.  I am also not for going back into development mode
on 9.2, which is what adding new functions now would amount to.  What's
wrong with leaving well enough alone?  It's not like there is no
unfinished work anywhere else in Postgres ...
        regards, tom lane

On Thu, May 24, 2012 at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, May 23, 2012 at 2:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> Is there plan to implement such external functions before 9.2 release?
>>> If not, keepalive protocol seems to be almost useless because there is
>>> no use of it for a user and the increase in the number of packets might
>>> increase the replication performance overhead slightly. No?
>
>> Good point.  IMHO, this shouldn't really have been committed like
>> this, but since it was, we had better fix it, either by reverting the
>> change or forcing an initdb to expose the functionality.
>
> I see no reason to rip the code out if we have plans to make use of it
> in the near future.  I am also not for going back into development mode
> on 9.2, which is what adding new functions now would amount to.  What's
> wrong with leaving well enough alone?  It's not like there is no
> unfinished work anywhere else in Postgres ...

So, extra TCP overhead for no user-visible benefit doesn't bother you?

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

On 24 May 2012 21:11, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, May 24, 2012 at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Wed, May 23, 2012 at 2:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> Is there plan to implement such external functions before 9.2 release?
>>>> If not, keepalive protocol seems to be almost useless because there is
>>>> no use of it for a user and the increase in the number of packets might
>>>> increase the replication performance overhead slightly. No?
>>
>>> Good point.  IMHO, this shouldn't really have been committed like
>>> this, but since it was, we had better fix it, either by reverting the
>>> change or forcing an initdb to expose the functionality.
>>
>> I see no reason to rip the code out if we have plans to make use of it
>> in the near future.  I am also not for going back into development mode
>> on 9.2, which is what adding new functions now would amount to.  What's
>> wrong with leaving well enough alone?  It's not like there is no
>> unfinished work anywhere else in Postgres ...
>
> So, extra TCP overhead for no user-visible benefit doesn't bother you?

Other changes occurred such that WAL messages don't get sent at all in
many cases on an idle server. The keep alive replaces that, so is of
value in itself.

The new functions would have made most sense if file based keepalives
had been approved. But that didn't make it in and hence incomplete.

Adding functions is the work of a few hours, but not worth starting
that if you intend to block it.

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

On Wed, May 30, 2012 at 4:34 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 24 May 2012 21:11, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, May 24, 2012 at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> On Wed, May 23, 2012 at 2:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>> Is there plan to implement such external functions before 9.2 release?
>>>>> If not, keepalive protocol seems to be almost useless because there is
>>>>> no use of it for a user and the increase in the number of packets might
>>>>> increase the replication performance overhead slightly. No?
>>>
>>>> Good point.  IMHO, this shouldn't really have been committed like
>>>> this, but since it was, we had better fix it, either by reverting the
>>>> change or forcing an initdb to expose the functionality.
>>>
>>> I see no reason to rip the code out if we have plans to make use of it
>>> in the near future.  I am also not for going back into development mode
>>> on 9.2, which is what adding new functions now would amount to.  What's
>>> wrong with leaving well enough alone?  It's not like there is no
>>> unfinished work anywhere else in Postgres ...
>>
>> So, extra TCP overhead for no user-visible benefit doesn't bother you?
>
> Other changes occurred such that WAL messages don't get sent at all in
> many cases on an idle server. The keep alive replaces that, so is of
> value in itself.
>
> The new functions would have made most sense if file based keepalives
> had been approved. But that didn't make it in and hence incomplete.

Even if we don't have file based keepalives, the new function enables us
to calculate the network latency, so it seems worth exposing the function.

OTOH, I wonder whether we really need to send keepalive messages
periodically to calculate a network latency. ISTM we don't unless a network
latency varies from situation to situation so frequently and we'd like to
monitor that in almost real time.

Regards,

--
Fujii Masao

On Wed, May 30, 2012 at 12:17 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> OTOH, I wonder whether we really need to send keepalive messages
> periodically to calculate a network latency. ISTM we don't unless a network
> latency varies from situation to situation so frequently and we'd like to
> monitor that in almost real time.

I didn't look at this patch too carefully when it was committed.
Looking at it more carefully now, it looks to me like this patch does
two different things.  One is to add a function called
GetReplicationApplyDelay(), which returns the number of milliseconds
since replay was fully caught up.  So if you were last caught up 5
minutes ago and you have replayed 4 minutes and 50 seconds worth of
WAL during that time, this function will return 5 minutes, not 10
seconds.  That is not what I would call "apply delay", which I would
define as how far behind you are NOW, not how long it's been since you
weren't behind at all.

The second thing it does is add a function called
GetReplicationTransferLatency().  The return value of this function is
the difference between the slave's clock at the time the last master
keepalive was processed and the master's clock at the time that
keepalive was generated.  I think that in practice, unless network
time synchronization is in use, this is mostly going to be computing
the clock skew between the master and the slave. If time
synchronization is in use, then as you say it'll be a very jittery
measure of master-slave network latency, which can be monitored
perfectly well from outside PG.

Now, measuring time skew is potentially a useful thing to do, if we
believe that this will actually give us an accurate measurement of
what the time skew is, because there are a whole series of things that
people want to do which involve subtracting a slave timestamp from a
master timestamp.  Tom has persistently rebuffed all such proposals on
the grounds that there might be time skew, so in theory we could make
those things possible by having a way to measure time skew, which this
does.  Here's what we do: given a slave timestamp, add the estimated
time skew to find an equivalent master timestamp, and then subtract.
Using a method of this type would allow us to compute a *real* apply
delay.  Woohoo!  Unfortunately, if time synchronization IS in use,
then the system clocks are probably already synchronized three to six
orders of magnitude more precisely than what this method can measure,
so the effect of using GetReplicationTransferLatency() to adjust slave
timestamps will be to massively reduce the accuracy of such
calculations.  However, I've thus far been unable to convince anyone
that this is a bad idea, so maybe this is where we're gonna end up.

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

Robert Haas <robertmhaas@gmail.com> writes:
> Now, measuring time skew is potentially a useful thing to do, if we
> believe that this will actually give us an accurate measurement of
> what the time skew is, because there are a whole series of things that
> people want to do which involve subtracting a slave timestamp from a
> master timestamp.  Tom has persistently rebuffed all such proposals on
> the grounds that there might be time skew, so in theory we could make
> those things possible by having a way to measure time skew, which this
> does.  Here's what we do: given a slave timestamp, add the estimated
> time skew to find an equivalent master timestamp, and then subtract.
> Using a method of this type would allow us to compute a *real* apply
> delay.  Woohoo!  Unfortunately, if time synchronization IS in use,
> then the system clocks are probably already synchronized three to six
> orders of magnitude more precisely than what this method can measure,
> so the effect of using GetReplicationTransferLatency() to adjust slave
> timestamps will be to massively reduce the accuracy of such
> calculations.  However, I've thus far been unable to convince anyone
> that this is a bad idea, so maybe this is where we're gonna end up.

Hmm ... first question is do we actually care whether the clocks are
synced to the millisecond level, ie what is it you'd do differently
if you know that the master and slave clocks are synced more closely
than you can measure at the protocol level.

But if there is a reason to care, perhaps we could have a setting that
says "we're using NTP, so trust the clocks to be synced"?  What I object
to is assuming that without any evidence, or being unable to operate
correctly in an environment where it's not true.
        regards, tom lane

On Thu, May 31, 2012 at 11:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hmm ... first question is do we actually care whether the clocks are
> synced to the millisecond level, ie what is it you'd do differently
> if you know that the master and slave clocks are synced more closely
> than you can measure at the protocol level.
>
> But if there is a reason to care, perhaps we could have a setting that
> says "we're using NTP, so trust the clocks to be synced"?  What I object
> to is assuming that without any evidence, or being unable to operate
> correctly in an environment where it's not true.

In general, we are happy to leave to the operating system - or some
other operating service - those tasks which are best handled in that
way.  I don't understand why this should be an exception.  If we're
not going to implement a filesystem inside PostgreSQL - which is
actually relatively closely related to our core mission as a data
store - then why the heck do we want to implement time
synchronization?  If this were an easy problem I wouldn't care, but
it's not.  The solution Simon has implemented here, besides being
vulnerable to network jitter that can't be eliminated without
reimplementing some sort of complex ntp-like protocol inside the
backend - won't work with log shipping, which is why (or part of why?)
Simon proposed keepalive files to allow this information to be passed
through the archive.  To me, this is massive over-engineering.  I'll
support the keepalive feature if it's the only way to get you to agree
to adding the capabilities we need to be competitive with other
replication solutions - but that's about the only redeeming value it
has IMV.

Now, mind you, I am not saying that we should randomly and arbitrarily
make ourselves vulnerable to clock skew when there is a reasonable
alternative design.  For example, you were able to come up with a way
to make max_standby_delay work sensibly without having to compare
master and slave timestamps, and that's good.  But in cases where no
such design exists - and a time-based notion of replication delay
seems to be one of those times - I don't see any real merit in
reinventing the wheel, especially since it seems likely that the wheel
is going to be dodecagonal.  Aside from network jitter and the need
for archive keepalives, suppose the two machines really do have clocks
that are an hour off from each other.  And the master system is really
busy so the slave runs about a minute behind.  We detect the time skew
and correct for it, so the replication delay shows up correctly.  Life
is good.  But then the system administrator notices that there's a
problem and fires up ntpd to fix it.  Our keepalive system will now
notice and decide that the "replication transfer latency" is now 0 s
instead of +/- 3600 s.  However, we're replaying records from a minute
ago, before the time change, so now for the next minute our
replication delay is either 61 minutes or -59 minutes, depending on
the direction of the skew, and then it goes back to normal.  Not the
end of the world, but weird.  It's the sort of thing that we probably
won't even try to document, because it'll affect very few people, but
anyone who is affected will have to understand the system pretty
deeply to understand what's gone wrong.  IME, users hate that.

On the other hand, if we simply say "PostgreSQL computes the
replication delay by subtracting the time at which the WAL was
generated, as recorded on the master, from the time at which it is
replayed by the slave" then, hey, we still have a wart, but it's
pretty clear what the wart is and how to fix it, and we can easily
document that.  Again, if we could get rid of the failure modes and
make this really water-tight, I think I'd be in favor of that, but it
seems to me that we are in the process of expending a lot of energy
and an even larger amount of calendar time to create a system that
will misbehave in numerous subtle ways instead of one straightforward
one.  I don't see that as a good trade.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On the other hand, if we simply say "PostgreSQL computes the
> replication delay by subtracting the time at which the WAL was
> generated, as recorded on the master, from the time at which it is
> replayed by the slave" then, hey, we still have a wart, but it's
> pretty clear what the wart is and how to fix it, and we can easily
> document that.  Again, if we could get rid of the failure modes and
> make this really water-tight, I think I'd be in favor of that, but it
> seems to me that we are in the process of expending a lot of energy
> and an even larger amount of calendar time to create a system that
> will misbehave in numerous subtle ways instead of one straightforward
> one.  I don't see that as a good trade.

Well, okay, but let's document "if you use this feature, it's incumbent
on you to make sure the master and slave clocks are synced.  We
recommend running NTP." or words to that effect.
        regards, tom lane

On 6/2/12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On the other hand, if we simply say "PostgreSQL computes the
>> replication delay by subtracting the time at which the WAL was
>> generated, as recorded on the master, from the time at which it is
>> replayed by the slave" then, hey, we still have a wart, but it's
>> pretty clear what the wart is and how to fix it, and we can easily
>> document that.  Again, if we could get rid of the failure modes and
>> make this really water-tight, I think I'd be in favor of that, but it
>> seems to me that we are in the process of expending a lot of energy
>> and an even larger amount of calendar time to create a system that
>> will misbehave in numerous subtle ways instead of one straightforward
>> one.  I don't see that as a good trade.
>
> Well, okay, but let's document "if you use this feature, it's incumbent
> on you to make sure the master and slave clocks are synced.  We
> recommend running NTP." or words to that effect.

What if the two servers are in different time zones?
--
Mike Nolan

On Sat, Jun 2, 2012 at 12:01 PM, Michael Nolan <htfoot@gmail.com> wrote:
> On 6/2/12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On the other hand, if we simply say "PostgreSQL computes the
>>> replication delay by subtracting the time at which the WAL was
>>> generated, as recorded on the master, from the time at which it is
>>> replayed by the slave" then, hey, we still have a wart, but it's
>>> pretty clear what the wart is and how to fix it, and we can easily
>>> document that.  Again, if we could get rid of the failure modes and
>>> make this really water-tight, I think I'd be in favor of that, but it
>>> seems to me that we are in the process of expending a lot of energy
>>> and an even larger amount of calendar time to create a system that
>>> will misbehave in numerous subtle ways instead of one straightforward
>>> one.  I don't see that as a good trade.
>>
>> Well, okay, but let's document "if you use this feature, it's incumbent
>> on you to make sure the master and slave clocks are synced.  We
>> recommend running NTP." or words to that effect.
>
> What if the two servers are in different time zones?

NTP shouldn't have any problem; it uses UTC underneath.  As does
PostgreSQL, underneath.
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

On 24 May 2012 19:45, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, May 23, 2012 at 2:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Sat, Dec 31, 2011 at 10:34 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> Send new protocol keepalive messages to standby servers.
>>> Allows streaming replication users to calculate transfer latency
>>> and apply delay via internal functions. No external functions yet.
>>
>> Is there plan to implement such external functions before 9.2 release?
>> If not, keepalive protocol seems to be almost useless because there is
>> no use of it for a user and the increase in the number of packets might
>> increase the replication performance overhead slightly. No?
>
> Good point.  IMHO, this shouldn't really have been committed like
> this, but since it was, we had better fix it, either by reverting the
> change or forcing an initdb to expose the functionality.

OK, I've had a chance to review all the code and the preceding
discussions on this.

It's clear to me that I owe Fujii and Robert an apology for insisting
that a better solution was possible but then not fully delivering in
time. Obviously, I did mean to deliver but obviously I didn't, having
confused my action item to finish keepalives with the file-based
version. Worse, I was unaware of that until you raised it here, for
which you get apology number two and some activity to make up for
that.

I agree with Robert that the function GetReplicationApplyDelay()
doesn't return a very useful answer. But the answer it returns is
exactly what the standby currently uses for its delay calculation.
Whatever else we do, giving the user access to the same calculation
seems important. So until/unless we change the standby delay
calculation we need that function, though perhaps a name change might
be appropriate.

We might want to have a different definition of apply delay for
different purposes, so an improved definition of apply delay doesn't
necessarily mean changing standby delay mechanism.

An improved definition of apply delay would be, IMHO
if (XLByteLE(receivePtr, replayPtr))   return 0;
if (recoveryLastXTime > currentChunkStartTime) then LastKnownTS = LastAppliedTS else        LastKnownTS = StartChunkTS
ApplyDelay = TimestampDifference(LastKnownTS, GetCurrentTimestamp()….);

Which assumes the clocks are in sync. It also doesn't give very useful
answers when no commits are occurring, and can hide the effects of
large amounts of WAL generated by VACUUMs. So we need a better
definition.

Defining the delay as difference between master.lastCommittedXactTS
and standby.lastAppliedXactTS suffers from the same problems.

An even better definition of apply delay was intended, which would
allow delayed apply of WAL data. I don't see a way of doing that
without keepalives to identify exactly when WAL arrived, even when it
has no timestamps. But what I had in mind is too much, too late for
9.2

Let's look at what we can do.

1. Functions - it's fairly easy to add some functions. Initially, we
can add them as a contrib module, then if an initdb is forced
elsewhere we can include them in the main server.

pg_last_xlog_receive_timestamp() - lastMsgReceiptTime
pg_last_chunk_replay_timestamp() - currentChunkStartTime

pg_current_standby_delay() - same calc as GetReplicationApplyDelay()
is today, but we rename
pg_current_apply_delay() - the new apply delay calc suggested above
which has more to do with query/staleness wrt master
pg_current_transfer_delay() - GetReplicationTransferLatency

pg_last_repmsg_receive_timestamp() - last msg receipt time, even if not WAL

pg_is_xlog_replay_conflict() - true if currently waiting on a conflict
pg_xlog_replay_wait_timestamp() - time of last conflict or pause

2. Keepalive messages - My plan was to flesh this out more. Given
complaints about bandwidth, I don't think that keepalives should be
sent without an option to disable them. The bandwidth is fairly small,
but agree it should be optional nonetheless. If its too late to add an
option, then I accept that we should just turn off the keepalives in
this release.

Your thoughts, please.

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

Simon Riggs <simon@2ndQuadrant.com> writes:
> 1. Functions - it's fairly easy to add some functions. Initially, we
> can add them as a contrib module, then if an initdb is forced
> elsewhere we can include them in the main server.

While I dislike the idea of a forced initdb at this point, adding a
contrib module that we're going to delete again in the next release
seems considerably worse.  That pushes complexity onto everybody,
not just those beta testers who might wish to upgrade their test
installations to final release.  (And, as we've seen with Andrew's
attempts at back-porting json support, we don't really have that
great of an answer for handling functions migrating into core from
a contrib module.)

How badly do we really need these functions right now?
        regards, tom lane

On 5 June 2012 22:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> 1. Functions - it's fairly easy to add some functions. Initially, we
...
> How badly do we really need these functions right now?

We need a better way of taking these decisions than just black/white
yes/no. It makes the decision more taxing than it needs to be.

Can't we have a trial branch where quarantined patches can be placed
on trial for inclusion in main release?

Plus. if we have extensions, why does adding a function need to force
an initdb?? Why don't we use our own infrastructure?

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

Simon Riggs <simon@2ndQuadrant.com> writes:
> Can't we have a trial branch where quarantined patches can be placed
> on trial for inclusion in main release?

[ shrug... ]  You're welcome to publish a personal repo somewhere with
such things.  But even if we did that in the master repo, it would have
approximately nothing to do with released versions.  You might as well
just figure on submitting the patch into 9.3.

> Plus. if we have extensions, why does adding a function need to force
> an initdb?? Why don't we use our own infrastructure?

I thought I already pointed that out, but: we have *extensions*.  What
we don't have is a convenient method of dealing with functions that need
to be migrated across extensions, or from an extension to core, between
one major release and the next.  It would clearly be nice to have that
someday, but we don't have it now.  Designing on the assumption that 9.3
will be able to do that nicely, when the required infrastructure is
still barely at the handwaving stage, seems like folly to me.

(In fact, pg_upgrade has more or less broken the ability even to do
significant refactoring within an extension, as I was ranting about in
another thread recently.  We really need to fix that.  But let's not
assume it's going to happen on any particular schedule.)
        regards, tom lane

On 5 June 2012 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Can't we have a trial branch where quarantined patches can be placed
>> on trial for inclusion in main release?
>
> [ shrug... ]  You're welcome to publish a personal repo somewhere with
> such things.  But even if we did that in the master repo, it would have
> approximately nothing to do with released versions.  You might as well
> just figure on submitting the patch into 9.3.

As you know, many of us have personal repos on github and
git.postgresql.org already. Technically, the official repo is nothing
more than just another clone too, owing to git's distributed
architecture, and yet we don't give out commit bits to just anyone.
Simon is proposing precisely that such a branch *would* have something
to do with released versions, as the linux-next branch does, for
example, and that our processes would change to merge upstream patches
from this branch, to better match the processes of other large open
source projects, in particular, those of the Linux kernel.

One way to make the process of patch acceptance scale without
compromising on quality is to pipeline it, which is precisely what the
Linux guys did, and, I believe, is a major reason why Linus Torvalds
took the time to develop his own source control system.

You may disagree with the view that we should do this, and that's
obviously a perfectly valid view. However, process matters, and
ceremony matters, and the suggestion that an officially blessed repo
is essentially equivalent to something somebody privately produces is
simply not accurate. If it was, why would you even care?

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 5 June 2012 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> Plus. if we have extensions, why does adding a function need to force
>> an initdb?? Why don't we use our own infrastructure?
>
> I thought I already pointed that out, but: we have *extensions*.  What
> we don't have is a convenient method of dealing with functions that need
> to be migrated across extensions, or from an extension to core, between
> one major release and the next.  It would clearly be nice to have that
> someday, but we don't have it now.  Designing on the assumption that 9.3
> will be able to do that nicely, when the required infrastructure is
> still barely at the handwaving stage, seems like folly to me.
>
> (In fact, pg_upgrade has more or less broken the ability even to do
> significant refactoring within an extension, as I was ranting about in
> another thread recently.  We really need to fix that.  But let's not
> assume it's going to happen on any particular schedule.)

What I had in mind was adding a final step to initdb that registers a
few "system extensions". We then make the default way to add functions
to Postgres is to create a system extension in
src/backend/extensions/xxx, not a hard-assigned oid function via
pg_proc.

As far as the worlds knows the functions added in this way would be
part of default postgres. Called in a predictable sequence they would
have the same oids on all platforms.

It would be fairly simple to drop all existing "system extensions"
then re-add them again, as part of the beta upgrade process. And much
simpler to produce patches that add functions.

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

On Tue, Jun 5, 2012 at 4:51 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> We might want to have a different definition of apply delay for
> different purposes, so an improved definition of apply delay doesn't
> necessarily mean changing standby delay mechanism.
>
> An improved definition of apply delay would be, IMHO
> if (XLByteLE(receivePtr, replayPtr))
>    return 0;
> if (recoveryLastXTime > currentChunkStartTime)
>  then LastKnownTS = LastAppliedTS
>  else
>         LastKnownTS = StartChunkTS
> ApplyDelay = TimestampDifference(LastKnownTS, GetCurrentTimestamp()….);
>
> Which assumes the clocks are in sync. It also doesn't give very useful
> answers when no commits are occurring, and can hide the effects of
> large amounts of WAL generated by VACUUMs. So we need a better
> definition.

Another problem is that it sometimes subtracts two slave timestamps,
and sometimes subtracts a master timestamp from a slave timestamp.  If
we're assuming that the clocks must be in sync, you could argue that's
OK, but I think it will lead to weird edge-case behavior.

Suppose that we have the master guarantee that at least one
timestamped WAL record will be emitted every N seconds.  For the sake
of argument, let's say N = 5.  So, every 5 seconds, some process wakes
up on the master and checks whether any commit or abort records - or
any other kind of WAL record that carries a timestamp - has been
emitted in the last 5 seconds.  If so, then it does nothing.  If not,
it checks whether any WAL at all has been emitted since the last
timestamped record was generated.    If not, then it again does
nothing.  But if so, then it emits a WAL record when consists solely
of a master timestamp.

On the slave, every time we reach a commit record, an abort record, or
one of these new master-timestamp records, or any other record that
happens to have a timestamp, we update some shared memory area which
stores (a) the last master timestamp we saw during replay and (b) the
slave timestamp at the time we replayed it.  Apply delay (ignoring
time skew) can be calculated by subtracting the first value from the
second one, or we could expose the two values separately, which might
be even better, since users can then answer questions like "how long
has it been since we were able to recalculate the apply delay?".

I'm sure that at least one member of the audience will have some rocks
to throw at this proposal... fire away, but be gentle, since we are
all on the same team here.

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

Tom Lane <tgl@sss.pgh.pa.us> writes:
> I thought I already pointed that out, but: we have *extensions*.  What
> we don't have is a convenient method of dealing with functions that need
> to be migrated across extensions, or from an extension to core, between
> one major release and the next.  It would clearly be nice to have that
> someday, but we don't have it now.  Designing on the assumption that 9.3

Well, my patch for 9.2 called "Finer Extension Dependencies" was all
about supporting that. The idea is to be able to name a set of functions
(or other objects) then setup a dependency graph towards that arbitrary
name. Then it's possible to migrate that named set of objects from an
extension to another one, and it's possible for core to publish a list
of provided names of set of objects provided.
 https://commitfest.postgresql.org/action/patch_view?id=727

Inspiring my work from some other development facilities I enjoy
spending my time with, I called that set a "feature" and added ways for
extensions to "provide" and "require" them, like has been done in some
lisps for more than 3 decades now. I'm not wedded to those terms, which
have been bringing confusion on the table before, please suggest some
other ones if you think you like the feature.

I'm going to have this patch in the next CF so that we can talk about it
again, as I think it is actually designed to help us fix the problem
here.

The only missing part in the patch is allowing for the "core" to declare
a set of set of objects (a set of features in its current terminology)
that it brings on the table. Such a list already exists though, and is
using the same terminology as in my patch:
 http://www.postgresql.org/docs/9.2/static/features-sql-standard.html

We wouldn't only publish the standard compliant feature list with such a
mechanism though or it would be quite useless for our operations here.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On Mon, Jun 4, 2012 at 8:49 PM, Christopher Browne <cbbrowne@gmail.com> wrote:
>> What if the two servers are in different time zones?
>
> NTP shouldn't have any problem; it uses UTC underneath.  As does
> PostgreSQL, underneath.

As an aside, this is not strictly speaking true. NTP doesn't "use UTC"
-- afaik it doesn't know about time zones at all. Likewise Postgres's
underlying representation is not UTC either. They both use the number
of seconds that have passed since the epoch. That's simply a number,
not a time at all, and the number is the same regardless of what time
zone you're in.

--
greg

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I thought I already pointed that out, but: we have *extensions*.  What
>> we don't have is a convenient method of dealing with functions that need
>> to be migrated across extensions, or from an extension to core, between
>> one major release and the next.  It would clearly be nice to have that
>> someday, but we don't have it now.  Designing on the assumption that 9.3

> Well, my patch for 9.2 called "Finer Extension Dependencies" was all
> about supporting that. The idea is to be able to name a set of functions
> (or other objects) then setup a dependency graph towards that arbitrary
> name. Then it's possible to migrate that named set of objects from an
> extension to another one, and it's possible for core to publish a list
> of provided names of set of objects provided.

Well, TBH I didn't think that concept was a useful solution for this at
all.  I can't imagine that we would define "features" at a sufficiently
fine granularity, or with enough advance foresight, to solve the sort of
problem that's being faced here.  How would you deal with the need for,
say, some of contrib/xml2's functions to get migrated to core in 9.4 or
so?  When you didn't know that would happen, much less exactly which
functions, when 9.3 came out?  AFAICS the only way that "features" could
fix this would be if we always created a feature for every exposed
function, which is unmanageable.

> The only missing part in the patch is allowing for the "core" to declare
> a set of set of objects (a set of features in its current terminology)
> that it brings on the table. Such a list already exists though, and is
> using the same terminology as in my patch:
>   http://www.postgresql.org/docs/9.2/static/features-sql-standard.html
> We wouldn't only publish the standard compliant feature list with such a
> mechanism though or it would be quite useless for our operations here.

AFAICS, the SQL-standard features list is just about entirely irrelevant
to this discussion.  How often is it going to happen that we implement a
standard feature in a contrib module before migrating it into core?
I think almost every case in which we'll face this problem will involve
a PG-specific feature not mentioned in the SQL feature taxonomy.  The
case at hand (some proposed new functions for managing replication)
certainly isn't going to be found there.

And, quite aside from whether we could invent feature names that match
what we want to move from contrib to core, exactly how would having a
feature name help?  The problem we're actually facing is getting
pg_upgrade to not dump particular functions when it's doing a
binary-upgrade dump of an extension.  Maybe I've forgotten, but I do not
recall any exposed connection between feature names and particular SQL
objects in your proposal.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Well, TBH I didn't think that concept was a useful solution for this at
> all.  I can't imagine that we would define "features" at a sufficiently
> fine granularity, or with enough advance foresight, to solve the sort of
> problem that's being faced here.  How would you deal with the need for,
> say, some of contrib/xml2's functions to get migrated to core in 9.4 or
> so?  When you didn't know that would happen, much less exactly which
> functions, when 9.3 came out?  AFAICS the only way that "features" could

You can provide and require new feature names on the same dependency
graph, and the patch allows us to do so without foresight. The trade off
here is that we would need to edit the previous version of PostgreSQL to
add some new feature names.

In your example we would add a feature name to the contrib/xmls
extensions in a 9.3 minor release and add the same feature to 9.4 core
distribution. That requires minor upgrade before major upgrade for us to
have a chance to deal with lack of foresight.

> fix this would be if we always created a feature for every exposed
> function, which is unmanageable.

Agreed, exposing each function as a feature is not the way to go.

> AFAICS, the SQL-standard features list is just about entirely irrelevant
> to this discussion.  How often is it going to happen that we implement a
> standard feature in a contrib module before migrating it into core?

I wanted the standard to help me with the "core features" idea, I agree
it's not a smart move here.

> I think almost every case in which we'll face this problem will involve
> a PG-specific feature not mentioned in the SQL feature taxonomy.  The
> case at hand (some proposed new functions for managing replication)
> certainly isn't going to be found there.

Agreed.

> And, quite aside from whether we could invent feature names that match
> what we want to move from contrib to core, exactly how would having a
> feature name help?  The problem we're actually facing is getting
> pg_upgrade to not dump particular functions when it's doing a
> binary-upgrade dump of an extension.  Maybe I've forgotten, but I do not
> recall any exposed connection between feature names and particular SQL
> objects in your proposal.

We're talking about several distinct problems here, one is low level
upgrade mechanism to keep OIDs and the other is about upgrading to a
newer version of the same extension, which the content changed. And we
want to do both, of course.

The idea would be to implement the upgrade as you're proposing, or at
least my understanding of it, which is to just issue a create extension
command as part of the schema creation. That will run the new extension
script which will know not to install deprecated functions.

The problem with that is to conserve OIDs that might be stored in user
relations, such as types. If we want pg_upgrade to cope with upgrading
an extension to a new content, we have to change its implementation.

An idea would be to add hooks in the backend code at OID attribution
time so that pg_upgrade can attach code that will provide the OID. As
input it should have creating_extension, extension name, schema name and
object name, and also object "argument list" for more complex things
such as functions and aggregates.

I don't think the "extension features" dependency management patch
should be fixing the running the newer extension script while keeping
existing OID list by itself.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On 5 June 2012 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Can't we have a trial branch where quarantined patches can be placed
>> on trial for inclusion in main release?
>
> [ shrug... ]  You're welcome to publish a personal repo somewhere with
> such things.  But even if we did that in the master repo, it would have
> approximately nothing to do with released versions.  You might as well
> just figure on submitting the patch into 9.3.

In an effort to resolve the PG9.2 open item on keepalives, I have

* Ensured we have full and correct info about replication messages
available for analysis

* Turned off keepalives by default, keeping all code. It can be turned
on again by someone that knows how.

I will later do this

* Publish functions that can turn on keepalives and analyse info from
them, making them available as an Extension for 9.2

Later meaning not during the next two weeks, but by Sep 15.

That's about the simplest way of resolving it I can think of, so I
hope that's fine for y'all.

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

On Thu, Aug 9, 2012 at 6:11 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 5 June 2012 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>> Can't we have a trial branch where quarantined patches can be placed
>>> on trial for inclusion in main release?
>>
>> [ shrug... ]  You're welcome to publish a personal repo somewhere with
>> such things.  But even if we did that in the master repo, it would have
>> approximately nothing to do with released versions.  You might as well
>> just figure on submitting the patch into 9.3.
>
> In an effort to resolve the PG9.2 open item on keepalives, I have
>
> * Ensured we have full and correct info about replication messages
> available for analysis
>
> * Turned off keepalives by default, keeping all code. It can be turned
> on again by someone that knows how.
>
> I will later do this
>
> * Publish functions that can turn on keepalives and analyse info from
> them, making them available as an Extension for 9.2
>
> Later meaning not during the next two weeks, but by Sep 15.
>
> That's about the simplest way of resolving it I can think of, so I
> hope that's fine for y'all.

Ah, wrong order of messages.Here's the explanation, you can disregard
my last one.

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