Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 11:44 +0200, Peter Eisentraut wrote:
> On mån, 2009-12-14 at 08:54 +, Simon Riggs wrote:
> > Wednesday because that seemed a good delay to allow review. Josh and I
> > had discussed the value of getting patch into Alpha3, so that was my
> > wish and aim.
> > 
> > I'm not aware of any particular date for end of commitfest, though
> > possibly you are about to update me on that?
> 
> Commit fests for 8.5 have usually run from the 15th to the 15th of next
> month, but the CF manager may choose to vary that.
> 
> FWIW, the alpha release manager may also vary the release timeline of
> alpha3. ;-)

I'm hoping that the alpha release manager can wait until Wednesday,
please. 

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 16:39 -0500, Tom Lane wrote:
> Simon Riggs  writes:
> > What is the best way of restricting the hash table to a maximum size? 
> 
> There is nothing in dynahash that will enforce a maximum size against
> calling code that's not cooperating; and I'll resist any attempt to
> add such a thing, because it would create a serialization point across
> the whole hashtable.

No problem, just checking with you where you'd like stuff put.

> If you know that you need at most N entries in the hash table, you can
> preallocate that many at startup (note the second arg to ShmemInitHash)
> and be safe.  If your calling code might go past that, you'll need to
> fix the calling code.

It's easy enough to count em on the way in and count em on the way out.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Tom Lane]

Simon Riggs  writes:
> What is the best way of restricting the hash table to a maximum size? 

There is nothing in dynahash that will enforce a maximum size against
calling code that's not cooperating; and I'll resist any attempt to
add such a thing, because it would create a serialization point across
the whole hashtable.

If you know that you need at most N entries in the hash table, you can
preallocate that many at startup (note the second arg to ShmemInitHash)
and be safe.  If your calling code might go past that, you'll need to
fix the calling code.

regards, tom lane

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 15:24 -0500, Tom Lane wrote:
> Simon Riggs  writes:
> > On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
> >>> I have ensured that they are always the same size, by definition, so no
> >>> need to check.
> >> 
> >> How did you ensure that? The hash table has no hard size limit.
> 
> > The hash table is in shared memory and the entry size is fixed. My
> > understanding was that this meant the hash table was fixed in size and
> > could not grow beyond the allocation. If that assumption was wrong, then
> > yes we could get an error. Is it?
> 
> Entirely.  The only thing the hash table size enters into is the sizing
> of overall shared memory --- different hash tables then consume space
> from the common pool, which includes not only the computed space
> requirements but a pretty hefty slop overhead.  You can go beyond the
> original requested space if there is any slop left.

OK, thanks.

> For a number of shared hashtables that actually have a fixed set of
> entries, we avoid the risk of unexpected out-of-memory by forcing all
> the entries to come into existence during startup.  If your table
> doesn't work that way then you cannot be sure of the exact point where
> it will get an out-of-memory failure.

The data structure was originally a list of fixed size, though is now a
shared hash table.

What is the best way of restricting the hash table to a maximum size? 

Your last para makes me think there is a way, but I can't see it
directly. If there isn't a facility to do this and I need to add code,
should I add optional code into the dynahash.c to track size, or should
I add that in the data structure code that uses the hash functions (so,
internally or externally).

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Tom Lane]

Simon Riggs  writes:
> On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
>>> I have ensured that they are always the same size, by definition, so no
>>> need to check.
>> 
>> How did you ensure that? The hash table has no hard size limit.

> The hash table is in shared memory and the entry size is fixed. My
> understanding was that this meant the hash table was fixed in size and
> could not grow beyond the allocation. If that assumption was wrong, then
> yes we could get an error. Is it?

Entirely.  The only thing the hash table size enters into is the sizing
of overall shared memory --- different hash tables then consume space
from the common pool, which includes not only the computed space
requirements but a pretty hefty slop overhead.  You can go beyond the
original requested space if there is any slop left.

For a number of shared hashtables that actually have a fixed set of
entries, we avoid the risk of unexpected out-of-memory by forcing all
the entries to come into existence during startup.  If your table
doesn't work that way then you cannot be sure of the exact point where
it will get an out-of-memory failure.

regards, tom lane

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 14:14 -0500, Tom Lane wrote:
> Simon Riggs  writes:
> > On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
> >> Simon Riggs  writes:
> >>> * Disallow clustering system relations.  This will definitely NOT work
> >>> * for shared relations (we have no way to update pg_class rows in other
> >>> * databases), nor for nailed-in-cache relations (the relfilenode values
> >>> * for those are hardwired, see relcache.c).  It might work for other
> >>> * system relations, but I ain't gonna risk it.
> >> 
> >>> I would presume we would not want to relax the restriction on CLUSTER
> >>> working on these tables, even if new VACUUM FULL does.
> >> 
> >> Why not?  If we solve the problem of allowing these relations to change
> >> relfilenodes, then CLUSTER would work just fine on them.  Whether it's
> >> particularly useful is not ours to decide I think.
> 
> > I think you are probably right, but my wish to prove Schrodinger's Bug
> > does not exist is not high enough for me personally to open that box
> > this side of 8.6, especially when the previous code author saw it as a
> > risk worth documenting. 
> 
> You're talking to the "previous code author" ... or at least I'm pretty
> sure that comment is mine.

Yeh, I figured, but I'm just as scared now as you were back then. 

This might allow CLUSTER to work, but it is definitely not something
that I will enabling, testing and committing to fix *when* it breaks
because my time is already allocated on other stuff.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Tom Lane]

Simon Riggs  writes:
> On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
>> Simon Riggs  writes:
>>> * Disallow clustering system relations.  This will definitely NOT work
>>> * for shared relations (we have no way to update pg_class rows in other
>>> * databases), nor for nailed-in-cache relations (the relfilenode values
>>> * for those are hardwired, see relcache.c).  It might work for other
>>> * system relations, but I ain't gonna risk it.
>> 
>>> I would presume we would not want to relax the restriction on CLUSTER
>>> working on these tables, even if new VACUUM FULL does.
>> 
>> Why not?  If we solve the problem of allowing these relations to change
>> relfilenodes, then CLUSTER would work just fine on them.  Whether it's
>> particularly useful is not ours to decide I think.

> I think you are probably right, but my wish to prove Schrodinger's Bug
> does not exist is not high enough for me personally to open that box
> this side of 8.6, especially when the previous code author saw it as a
> risk worth documenting. 

You're talking to the "previous code author" ... or at least I'm pretty
sure that comment is mine.

regards, tom lane

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
> Simon Riggs  writes:
> >  * Disallow clustering system relations.  This will definitely NOT work
> >  * for shared relations (we have no way to update pg_class rows in other
> >  * databases), nor for nailed-in-cache relations (the relfilenode values
> >  * for those are hardwired, see relcache.c).  It might work for other
> >  * system relations, but I ain't gonna risk it.
> 
> > I would presume we would not want to relax the restriction on CLUSTER
> > working on these tables, even if new VACUUM FULL does.
> 
> Why not?  If we solve the problem of allowing these relations to change
> relfilenodes, then CLUSTER would work just fine on them.  Whether it's
> particularly useful is not ours to decide I think.

I think you are probably right, but my wish to prove Schrodinger's Bug
does not exist is not high enough for me personally to open that box
this side of 8.6, especially when the previous code author saw it as a
risk worth documenting. 

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:

> >> * You removed this comment from KnownAssignedXidsInit:
> >>
> >> -   /*
> >> -* XXX: We should check that we don't exceed maxKnownAssignedXids.
> >> -* Even though the hash table might hold a few more entries than that,
> >> -* we use fixed-size arrays of that size elsewhere and expected all
> >> -* entries in the hash table to fit.
> >> -*/
> >>
> >> but AFAICS you didn't address the issue. It's referring to the 'xids'
> >> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
> >> in without checking that it fits.
> > 
> > I have ensured that they are always the same size, by definition, so no
> > need to check.
> 
> How did you ensure that? The hash table has no hard size limit.

The hash table is in shared memory and the entry size is fixed. My
understanding was that this meant the hash table was fixed in size and
could not grow beyond the allocation. If that assumption was wrong, then
yes we could get an error. Is it? Do you know from experience, or from
code comments?

Incidentally just picked up two much easier issues in that stretch of
code. Thanks for making me look again!

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Tom Lane]

Simon Riggs  writes:
>  * Disallow clustering system relations.  This will definitely NOT work
>  * for shared relations (we have no way to update pg_class rows in other
>  * databases), nor for nailed-in-cache relations (the relfilenode values
>  * for those are hardwired, see relcache.c).  It might work for other
>  * system relations, but I ain't gonna risk it.

> I would presume we would not want to relax the restriction on CLUSTER
> working on these tables, even if new VACUUM FULL does.

Why not?  If we solve the problem of allowing these relations to change
relfilenodes, then CLUSTER would work just fine on them.  Whether it's
particularly useful is not ours to decide I think.

regards, tom lane

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Sun, 2009-12-13 at 22:25 +, Simon Riggs wrote:

> * Which exact tables are we talking about: just pg_class and the shared
> catalogs? Everything else is in pg_class, so if we can find it we're OK?
> formrdesc() tells me the list of nailed relations is: pg_database,
> pg_class, pg_attribute, pg_proc, and pg_type. Are the nailed relations
> the ones we care about, or are they just a subset? 

Comments in cluster.c's check_index_is_clusterable() suggest that the
list of tables to which this applies is nailed relations *and* shared
relations, plus their indexes.

/*
 * Disallow clustering system relations.  This will definitely NOT work
 * for shared relations (we have no way to update pg_class rows in other
 * databases), nor for nailed-in-cache relations (the relfilenode values
 * for those are hardwired, see relcache.c).  It might work for other
 * system relations, but I ain't gonna risk it.
 */

So that means we need to handle 3 cases: nailed-local, nailed-shared and
non-nailed-shared.

I would presume we would not want to relax the restriction on CLUSTER
working on these tables, even if new VACUUM FULL does.

Anyway, not going to be done for Alpha3, but seems fairly doable now.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 18:02 +, Greg Stark wrote:
> >> It doesn't seem any more wrong than using hash indexes right after
> >> recovery, crash recovery or otherwise. It's certainly broken, but I
> >> don't see much value in a partial fix. The bottom line is that hash
> >> indexes should be WAL-logged.
> >
> > I know that's your thought; I'm just checking its everyone else's as
> > well. We go to great lengths elsewhere in the patch to avoid queries
> > returning incorrect results and there is a loss of capability as a
> > result. I don't want Hash index users to view this as a feature. I
> don't
> > feel too strongly, but it can be argued both ways, at least.
> 
> This goes back to your pluggable rmgr point. Someone could add a new
> index method and get bogus results on their standby. And unlike hash
> indexes where there's some hope of addressing the problem there's
> nothing they can do to fix this.
> 
> It does seem like having a flag in the catalog to mark nonrecoverable
> indexes and make them unavailable to query plans on the standby would
> be worth its weight in code.

pg_am.amalmostworks or perhaps pg_am.amhalffinished... :-)

I wouldn't wish to literally persist that situation, especially since if
we had it we couldn't update it during recovery. We need to allow
pluggable indexes in full, not just partially. I think we should extend
pg_am so that rmgr routines can be defined for them also, with dynamic
assignment of rmgrids, recorded in file so we can use them during
recovery.

What we also need is a mechanism to identify and mark indexes as corrupt
while they are being rebuilt, so recovery can complete without them and
then rebuild automatically when recovery finishes. And so they can be
skipped during hot standby.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Heikki Linnakangas]

Simon Riggs wrote:
> On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>> * Are you planning to remove the recovery_connections setting before
>> release? The documentation makes it sound like it's a temporary hack
>> that we're not really sure is needed at all. That's not very comforting.
> 
> It has been requested and I agree, so its there. Saying it might be
> removed in future is no more than we do elsewhere and AFAIK we all hope
> it will be. Not sure why that is or isn't comforting.

Now that recovery_connections has a double-role, and does in the master
what the wal_standby_info used to do, the documentation probably should
be clarified that the whole parameter is not going to go away, just the
role in the master.

>> * You removed this comment from KnownAssignedXidsInit:
>>
>> -   /*
>> -* XXX: We should check that we don't exceed maxKnownAssignedXids.
>> -* Even though the hash table might hold a few more entries than that,
>> -* we use fixed-size arrays of that size elsewhere and expected all
>> -* entries in the hash table to fit.
>> -*/
>>
>> but AFAICS you didn't address the issue. It's referring to the 'xids'
>> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
>> in without checking that it fits.
> 
> I have ensured that they are always the same size, by definition, so no
> need to check.

How did you ensure that? The hash table has no hard size limit.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby, release candidate?

[Heikki Linnakangas]

Greg Smith wrote:
> Heikki Linnakangas wrote:
>> I note that if it was easy to run pgindent yourself on a patch before
>> committing/submitting, we wouldn't need to have this discussion. I don't
>> know hard it is to get it working right, but I recall I tried once and
>> gave up.
>>   
> What sort of problems did you run into?

I don't remember, unfortunately.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby, release candidate?

[Greg Stark]

On Mon, Dec 14, 2009 at 11:07 AM, Simon Riggs  wrote:
>> * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
>> on your transaction rate to begin with. Do we really want this setting
>> in its current form? Does it make sense as PGC_USERSET, as if one
>> backend uses a lower setting than others, that's the one that really
>> determines when transactions are killed in the standby? I think this
>> should be discussed and implemented as a separate patch.
>
> All the vacuum_*_age parameters have this characteristic, yet they
> exist.

I think it makes sense to have it be userset because someone could run
vacuum on one table with a different defer_cleanup_age for some
reason. I admit I'm having trouble coming up with a good use case.
Personally I'm fine with having parameters like this in alphas that
end up being renamed, or changed to different semantics, or even
removed by the time it's launched.


>> It doesn't seem any more wrong than using hash indexes right after
>> recovery, crash recovery or otherwise. It's certainly broken, but I
>> don't see much value in a partial fix. The bottom line is that hash
>> indexes should be WAL-logged.
>
> I know that's your thought; I'm just checking its everyone else's as
> well. We go to great lengths elsewhere in the patch to avoid queries
> returning incorrect results and there is a loss of capability as a
> result. I don't want Hash index users to view this as a feature. I don't
> feel too strongly, but it can be argued both ways, at least.

This goes back to your pluggable rmgr point. Someone could add a new
index method and get bogus results on their standby. And unlike hash
indexes where there's some hope of addressing the problem there's
nothing they can do to fix this.

It does seem like having a flag in the catalog to mark nonrecoverable
indexes and make them unavailable to query plans on the standby would
be worth its weight in code.

-- 
greg

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

Re: [HACKERS] Hot Standby, release candidate?

[Greg Smith]

Heikki Linnakangas wrote:

I note that if it was easy to run pgindent yourself on a patch before
committing/submitting, we wouldn't need to have this discussion. I don't
know hard it is to get it working right, but I recall I tried once and
gave up.
  

What sort of problems did you run into?

--
Greg Smith2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
g...@2ndquadrant.com  www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Bruce Momjian]

Tom Lane wrote:
> Robert Haas  writes:
> > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs  wrote:
> >> Why is (1) important, and if it is important, why is it being mentioned
> >> only now? Are we saying that all previous reviewers of my work (and
> >> others') removed these without ever mentioning they had done so?
> 
> > pgident will remove such white spaces and create merge conflicts for
> > everyone working on those areas of the code.
> 
> What I try really hard to remove from committed patches is spurious
> whitespace changes to pre-existing code.  Whether new code blocks
> exactly match pgindent's rules is less of a concern, but changing
> code you don't have to in a way that pgindent will undo later anyway
> is just useless creation of potential conflicts.
> 
> The whole thing would be a lot easier if someone would put together an
> easily-installable version of pgindent.  Bruce has posted the patches he
> uses but I don't know what version of indent they're against...

The entire indent tarball with patches is on our ftp site.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

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

Re: [HACKERS] Hot Standby, release candidate?

[Robert Haas]

On Mon, Dec 14, 2009 at 10:49 AM, Simon Riggs  wrote:
> On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote:
>> Simon Riggs  wrote:
>>
>> > If we are going to run pgindent anyway, what is the point?
>>
>> Perhaps it would take less time to run this than to argue the point?:
>>
>> sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *
>
> Not certain that is exactly correct, plus it doesn't only work against a
> current patch since there are already many examples of whitespace in CVS
> already. I do appreciate your attempts at resolution and an easy
> tool-based approach for the future, though I'll let someone else run
> with it.

Yeah, that would actually be a disaster, because it would actually add
deltas to the patch in the short term.

Seems to me that we would be better off figuring out which exact ident
Bruce is running and checking the typedef list into CVS.

...Robert

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

Re: [HACKERS] Hot Standby, release candidate?

[David Fetter]

On Mon, Dec 14, 2009 at 11:09:49AM +0100, Magnus Hagander wrote:
> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>  wrote:
> > * Please remove any spurious whitespace.  "git diff --color" makes
> > them stand out like a sore thumb, in red. (pgindent will fix them
> > but always better to fix them before committing, IMO).
> 
> +1 in general, not particularly for this patch (haven't checked that
> in this patch).
> 
> Actually, how about we add that to the page at
> http://wiki.postgresql.org/wiki/Submitting_a_Patch?

Done.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote:
> Simon Riggs  wrote:
>  
> > If we are going to run pgindent anyway, what is the point?
>  
> Perhaps it would take less time to run this than to argue the point?:
>  
> sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *

Not certain that is exactly correct, plus it doesn't only work against a
current patch since there are already many examples of whitespace in CVS
already. I do appreciate your attempts at resolution and an easy
tool-based approach for the future, though I'll let someone else run
with it.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Heikki Linnakangas]

Simon Riggs wrote:
> On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>>  wrote:
>>> * Please remove any spurious whitespace.  "git diff --color" makes them
>>> stand out like a sore thumb, in red. (pgindent will fix them but always
>>> better to fix them before committing, IMO).
>> +1 in general, not particularly for this patch (haven't checked that
>> in this patch).
>>
>> Actually, how about we add that to the page at
>> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
> 
> If we can define "spurious whitespace" it would help decide whether
> there is any action to take, and when.

There's two definitions that are useful:

- Anything that "git diff --color" shows up as glaring red. Important to
remove simply because the red whitespace is distracting when I review a
patch.

- Anything that pgindent would/will eventually fix. Because you might as
well fix them before committing and save the extra churn and potential
(although trivial) merge conflicts in people's outstanding patches when
pgindent is run. I don't run pgindent myself, so I wouldn't notice most
stuff, but I tend to fix things that I do notice.

> Why is (1) important, and if it is important, why is it being mentioned
> only now? Are we saying that all previous reviewers of my work (and
> others') removed these without ever mentioning they had done so?

I did it in one pass, Oct 15th according to git log.

I tend to silently fix whitespace issues like that in all patches I
commit. It's generally trivial enough to fix that it's easier to just
fix it myself than complain, explain, and look like a real nitpick.

I note that if it was easy to run pgindent yourself on a patch before
committing/submitting, we wouldn't need to have this discussion. I don't
know hard it is to get it working right, but I recall I tried once and
gave up. Any volunteers?

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby, release candidate?

[Alvaro Herrera]

Tom Lane escribió:

> The whole thing would be a lot easier if someone would put together an
> easily-installable version of pgindent.  Bruce has posted the patches he
> uses but I don't know what version of indent they're against...

And we're still unclear on the typedef list that's going to be used.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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

Re: [HACKERS] Hot Standby, release candidate?

[Robert Haas]

On Mon, Dec 14, 2009 at 10:08 AM, Simon Riggs  wrote:
> On Mon, 2009-12-14 at 09:32 -0500, Robert Haas wrote:
>> If every patch perfectly matched the pgident style, then the pgindent
>> run would change nothing and we would all be VERY happy.
>
> I've made all whitespace changes to the patch.
>
> I understand the reason for acting as you suggest, but we either police
> it, or we don't. If we don't police in all cases then I'm not personally
> happy to be policed.

I am doing my best to police it, and certainly will police it for
anything that I review or commit.  Heikki was the one who originally
pointed it on this thread, Magnus gave a +1, and I am pretty sure Tom
tries to keep an eye out for it, too, so generally I would say it is
project practice.  Obviously I am not able to control the actions of
all the other committers, and it does appear that some crap has crept
in since the last pgindent run.  :-(

At any rate I don't think you're being singled out for special treatment.

> About 90% of the whitespace in the patch were in docs, README or test
> output files. A great many of that type of file have numerous line
> ending whitespace, not introduced by me, so it seems to me that this has
> never been adequately policed in the way you say. If we really do care
> about this issue then I expect people to look a little further than just
> my patch.

pgindent won't reindent the docs or the README, but git diff --check
picks up on trailing whitespace, so it may be that Tom (who doesn't
use git but does commit a lot of patches) is less finicky about
trailing whitespace in those places.  If we move to git across the
board, I expect this to get more standardized handling, but I think we
have a ways to go on that yet.

> Portability of patches across releases isn't a huge issue for me, nor
> for the project, I think, but I am willing to commit to cleaning future
> patches in this way.

It's a pretty significant issue for me personally, and anyone who is
maintaining a fork of the main source base that has to be merged when
the pgindent run hits.  It would be nice if someone wanted to build a
better mousetrap here, but so far no volunteers.

...Robert

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

Re: [HACKERS] Hot Standby, release candidate?

[Tom Lane]

Robert Haas  writes:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs  wrote:
>> Why is (1) important, and if it is important, why is it being mentioned
>> only now? Are we saying that all previous reviewers of my work (and
>> others') removed these without ever mentioning they had done so?

> pgident will remove such white spaces and create merge conflicts for
> everyone working on those areas of the code.

What I try really hard to remove from committed patches is spurious
whitespace changes to pre-existing code.  Whether new code blocks
exactly match pgindent's rules is less of a concern, but changing
code you don't have to in a way that pgindent will undo later anyway
is just useless creation of potential conflicts.

The whole thing would be a lot easier if someone would put together an
easily-installable version of pgindent.  Bruce has posted the patches he
uses but I don't know what version of indent they're against...

regards, tom lane

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 13:56 +0100, Magnus Hagander wrote:

> Same issue can be it in git - did you do a "git pull" before? You may
> need merging with what's on there, and for that to work you must pull
> before you can push.

Found some merge conflicts and resolved them. I did fetch and merge at
different times, so that seems to be the source.

I've resolved the other git issues, so latest version on git now.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Kevin Grittner]

Simon Riggs  wrote:
 
> If we are going to run pgindent anyway, what is the point?
 
Perhaps it would take less time to run this than to argue the point?:
 
sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *
 
-Kevin

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 09:32 -0500, Robert Haas wrote:

> If every patch perfectly matched the pgident style, then the pgindent
> run would change nothing and we would all be VERY happy.

I've made all whitespace changes to the patch.

I understand the reason for acting as you suggest, but we either police
it, or we don't. If we don't police in all cases then I'm not personally
happy to be policed.

About 90% of the whitespace in the patch were in docs, README or test
output files. A great many of that type of file have numerous line
ending whitespace, not introduced by me, so it seems to me that this has
never been adequately policed in the way you say. If we really do care
about this issue then I expect people to look a little further than just
my patch.

Portability of patches across releases isn't a huge issue for me, nor
for the project, I think, but I am willing to commit to cleaning future
patches in this way.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Robert Haas]

On Mon, Dec 14, 2009 at 7:42 AM, Simon Riggs  wrote:
> There are no changes in this patch that are purely whitespace changes.
> Those were removed prior to patch submission. This is all about code I
> am adding or changing. My additions may disrupt their patches, but not
> because of the whitespace.
>
> If we are going to run pgindent anyway, what is the point? Seems like we
> need a tool to fix patches, not a tool to fix the code.
>
> I've made some changes to the larger files.

I'll try to explain this again because it is a little bit subtle.

Whenever someone commits ANY patch, there is a danger of disrupting
other outstanding patches that touch the same code.  That's basically
unavoidable, although certainly it's a reason to avoid superfluous
changes like whitespace adjustments or useless identifier renaming.

However, there's a second, completely independent issue, which is that
eventually we will run pgindent for 8.5, and when we do that, it's
going to reformat stuff, and that reformatting is going to break
things.  The amount of reformatting that it does (and therefore the
amount of breakage that it causes) is directly related to the extent
to which people have committed patches throughout the release cycle
that don't conform to the layout that pgident is going to want.  If
every patch perfectly matched the pgident style, then the pgindent run
would change nothing and we would all be VERY happy.  The more
deviations there are, the more stuff breaks.

So I agree with you: we need a tool to fix patches.  However, since we
haven't got one, we owe it to other contributors not to make the
problem any worse than necessary by adhering to the project's
formatting conventions as best we're able when committing things,
especially with regards to trivial things like trailing white-space
that git diff --check can easily find.  Actually, git apply has an
option to fix these simple types of problems, so it's possible to fix
it diffing the patch set against the master branch, applying it to a
separate copy of the master branch using git apply --whitespace=fix,
then diffing that against the original batch and applying the changes.
 Although that's usually overkill unless it's really bad.

There's some interesting discussion on whitespace more generally from
Tom Lane here:

http://archives.postgresql.org/pgsql-hackers/2009-08/msg01001.php

...Robert

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

Re: [HACKERS] Hot Standby, release candidate?

[Magnus Hagander]

On Mon, Dec 14, 2009 at 13:47, Simon Riggs  wrote:
> On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote:
>> On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs  wrote:
>> > I am now unable to push these changes to the shared repo. What is
>> > happening?
>>
>> Perhaps you need to run cvs update on your local copy?
>>
>> (I find this flavor the most useful: "cvs -q update -d"  YMMV.)
>>
>> If that's not it, error message?
>
> It was a question for Heikki only, not a CVS issue.
>
> git push ssh://g...@git.postgresql.org/users/heikki/postgres.git
> hs-simon:hs-riggs
> To ssh://g...@git.postgresql.org/users/heikki/postgres.git
>  ! [rejected]        hs-simon -> hs-riggs (non-fast forward)
> error: failed to push some refs to
> 'ssh://g...@git.postgresql.org/users/heikki/postgres.git'

Same issue can be it in git - did you do a "git pull" before? You may
need merging with what's on there, and for that to work you must pull
before you can push.

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

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs  wrote:
> > I am now unable to push these changes to the shared repo. What is
> > happening?
> 
> Perhaps you need to run cvs update on your local copy?
> 
> (I find this flavor the most useful: "cvs -q update -d"  YMMV.)
> 
> If that's not it, error message?

It was a question for Heikki only, not a CVS issue.

git push ssh://g...@git.postgresql.org/users/heikki/postgres.git
hs-simon:hs-riggs
To ssh://g...@git.postgresql.org/users/heikki/postgres.git
 ! [rejected]hs-simon -> hs-riggs (non-fast forward)
error: failed to push some refs to
'ssh://g...@git.postgresql.org/users/heikki/postgres.git'

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 06:51 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs  wrote:
> > On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
> >> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs  wrote:
> >> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
> >> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
> >> >>  wrote:
> >> >> > * Please remove any spurious whitespace.  "git diff --color" makes 
> >> >> > them
> >> >> > stand out like a sore thumb, in red. (pgindent will fix them but 
> >> >> > always
> >> >> > better to fix them before committing, IMO).
> >> >>
> >> >> +1 in general, not particularly for this patch (haven't checked that
> >> >> in this patch).
> >> >>
> >> >> Actually, how about we add that to the page at
> >> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
> >> >
> >> > If we can define "spurious whitespace" it would help decide whether
> >> > there is any action to take, and when.
> >>
> >> git defines it as either (1) extra whitespace at the end of a line or
> >> (2) an initial indent that uses spaces followed by tabs (typically
> >> something like space-tab, where tab alone would have produced the same
> >> result).  git diff --check master tends to be useful here.
> >
> > (2) is a problem that has been discussed before on hackers, anything
> > like that should be changed.
> >
> > Why is (1) important, and if it is important, why is it being mentioned
> > only now? Are we saying that all previous reviewers of my work (and
> > others') removed these without ever mentioning they had done so?
> 
> pgident will remove such white spaces and create merge conflicts for
> everyone working on those areas of the code.  I certainly mention this
> in any review I do where it's applicable, and have been doing so for
> some time.  I also will certainly fix it for any code I commit.  I
> also mentioned it in the review that I did of Hot Standby.

I don't recall you mentioning that.

There are no changes in this patch that are purely whitespace changes.
Those were removed prior to patch submission. This is all about code I
am adding or changing. My additions may disrupt their patches, but not
because of the whitespace.

If we are going to run pgindent anyway, what is the point? Seems like we
need a tool to fix patches, not a tool to fix the code.

I've made some changes to the larger files.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Robert Haas]

On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs  wrote:
> I am now unable to push these changes to the shared repo. What is
> happening?

Perhaps you need to run cvs update on your local copy?

(I find this flavor the most useful: "cvs -q update -d"  YMMV.)

If that's not it, error message?

...Robert

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 11:07 +, Simon Riggs wrote:
> Thanks for the further review, much appreciated.
> 
> On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
> > Simon Riggs wrote:
> > > Enclose latest version of Hot Standby. 
>  
> > * LockAcquireExtended needs a function comment. Or at least something
> > explaining what report_memory_error does. And perhaps the argument
> > should be named "reportMemoryError" to be in line with the other arguments.
> 
> OK

Done

> > * We tend to not add remarks about authors in code (comments in standby.c).
> 
> OK

Done

> > * This optimization in GetConflictingVirtualXIDs():
> > 
> > > +   /*
> > > +* If we don't know the TransactionId that created the conflict, set
> > > +* it to latestCompletedXid which is the latest possible value.
> > > +*/
> > > +   if (!TransactionIdIsValid(limitXmin))
> > > +   limitXmin = ShmemVariableCache->latestCompletedXid;
> > > +
> > 
> > needs a lot more explanation. It took me a very long time to figure out
> > why using latest completed xid is safe.
> 
> OK. Took me a long time as well.

Done

> > * Can you merge with CVS HEAD, please? There's some merge conflicts.
> 
> Huh? I did. And tested patch against a CVS checkout before submitting.
> Can you explain further?

Still not sure what conflicts you see or where they might come from...

I am now unable to push these changes to the shared repo. What is
happening?

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Magnus Hagander]

On Mon, Dec 14, 2009 at 12:51, Robert Haas  wrote:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs  wrote:
>> On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
>>> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs  wrote:
>>> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>>> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>>> >>  wrote:
>>> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
>>> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
>>> >> > better to fix them before committing, IMO).
>>> >>
>>> >> +1 in general, not particularly for this patch (haven't checked that
>>> >> in this patch).
>>> >>
>>> >> Actually, how about we add that to the page at
>>> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>>> >
>>> > If we can define "spurious whitespace" it would help decide whether
>>> > there is any action to take, and when.
>>>
>>> git defines it as either (1) extra whitespace at the end of a line or
>>> (2) an initial indent that uses spaces followed by tabs (typically
>>> something like space-tab, where tab alone would have produced the same
>>> result).  git diff --check master tends to be useful here.
>>
>> (2) is a problem that has been discussed before on hackers, anything
>> like that should be changed.
>>
>> Why is (1) important, and if it is important, why is it being mentioned
>> only now? Are we saying that all previous reviewers of my work (and
>> others') removed these without ever mentioning they had done so?
>
> pgident will remove such white spaces and create merge conflicts for
> everyone working on those areas of the code.  I certainly mention this
> in any review I do where it's applicable, and have been doing so for
> some time.  I also will certainly fix it for any code I commit.  I
> also mentioned it in the review that I did of Hot Standby.

I also always do this when committing other peoples patches (which I
don't do as often as I should, but when I *do* do it..)


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

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

Re: [HACKERS] Hot Standby, release candidate?

[Robert Haas]

On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs  wrote:
> On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
>> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs  wrote:
>> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>> >>  wrote:
>> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
>> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
>> >> > better to fix them before committing, IMO).
>> >>
>> >> +1 in general, not particularly for this patch (haven't checked that
>> >> in this patch).
>> >>
>> >> Actually, how about we add that to the page at
>> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>> >
>> > If we can define "spurious whitespace" it would help decide whether
>> > there is any action to take, and when.
>>
>> git defines it as either (1) extra whitespace at the end of a line or
>> (2) an initial indent that uses spaces followed by tabs (typically
>> something like space-tab, where tab alone would have produced the same
>> result).  git diff --check master tends to be useful here.
>
> (2) is a problem that has been discussed before on hackers, anything
> like that should be changed.
>
> Why is (1) important, and if it is important, why is it being mentioned
> only now? Are we saying that all previous reviewers of my work (and
> others') removed these without ever mentioning they had done so?

pgident will remove such white spaces and create merge conflicts for
everyone working on those areas of the code.  I certainly mention this
in any review I do where it's applicable, and have been doing so for
some time.  I also will certainly fix it for any code I commit.  I
also mentioned it in the review that I did of Hot Standby.

...Robert

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs  wrote:
> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
> >>  wrote:
> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
> >> > better to fix them before committing, IMO).
> >>
> >> +1 in general, not particularly for this patch (haven't checked that
> >> in this patch).
> >>
> >> Actually, how about we add that to the page at
> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
> >
> > If we can define "spurious whitespace" it would help decide whether
> > there is any action to take, and when.
> 
> git defines it as either (1) extra whitespace at the end of a line or
> (2) an initial indent that uses spaces followed by tabs (typically
> something like space-tab, where tab alone would have produced the same
> result).  git diff --check master tends to be useful here.

(2) is a problem that has been discussed before on hackers, anything
like that should be changed.

Why is (1) important, and if it is important, why is it being mentioned
only now? Are we saying that all previous reviewers of my work (and
others') removed these without ever mentioning they had done so?

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Robert Haas]

On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs  wrote:
> On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>>  wrote:
>> > * Please remove any spurious whitespace.  "git diff --color" makes them
>> > stand out like a sore thumb, in red. (pgindent will fix them but always
>> > better to fix them before committing, IMO).
>>
>> +1 in general, not particularly for this patch (haven't checked that
>> in this patch).
>>
>> Actually, how about we add that to the page at
>> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>
> If we can define "spurious whitespace" it would help decide whether
> there is any action to take, and when.

git defines it as either (1) extra whitespace at the end of a line or
(2) an initial indent that uses spaces followed by tabs (typically
something like space-tab, where tab alone would have produced the same
result).  git diff --check master tends to be useful here.

...Robert

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

Re: [HACKERS] Hot Standby, release candidate?

[Fujii Masao]

On Mon, Dec 14, 2009 at 8:07 PM, Simon Riggs  wrote:
>> * Please remove any spurious whitespace.  "git diff --color" makes them
>> stand out like a sore thumb, in red. (pgindent will fix them but always
>> better to fix them before committing, IMO).
>
> What is your definition of spurious whitespace? I removed all
> additions/deletions of individual lines. git diff colours many things
> red, and it seems like a waste of time to spend hours fiddling with
> spaces manually if there is a utility that does this anyway.

I guess it's a trailing whitespace. How about using "git diff --check"
instead of "--color"?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>  wrote:
> > * Please remove any spurious whitespace.  "git diff --color" makes them
> > stand out like a sore thumb, in red. (pgindent will fix them but always
> > better to fix them before committing, IMO).
> 
> +1 in general, not particularly for this patch (haven't checked that
> in this patch).
> 
> Actually, how about we add that to the page at
> http://wiki.postgresql.org/wiki/Submitting_a_Patch?

If we can define "spurious whitespace" it would help decide whether
there is any action to take, and when.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

Thanks for the further review, much appreciated.

On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Enclose latest version of Hot Standby. This is the "basic" patch, with
> > no must-fix issues and no known bugs. Further additions will follow
> > after commit, primarily around usability, which will include additional
> > control functions for use in testing. Various thoughts discussed here
> > http://wiki.postgresql.org/wiki/Hot_Standby_TODO
> 
> I still consider it highly important to be able to start standby from a
> shutdown checkpoint. If you remove it, at the very least put it back on
> the TODO.

Happy to put it back on TODO, but I'm not likely to do this without a
good reason. IMHO your arguments as to why that is useful were not
convincing, especially when it introduces further complications and
requirements for tests.

> But as it is, StandbyRecoverPreparedTransactions() is dead code, and the
> changes to PrescanPreparedTransactions() are not needed either.
> 
> > Patch now includes a set of regression tests that can be run against a
> > standby server using "make standbycheck"
> 
> Nice!
> 
> > (requires setup, see src/test/regress/standby_schedule). 
> 
> Any chance of automating that?

Future, yes. 

I view standbycheck as similar to installcheck - it requires some setup
before it can run, so allows you to test an existing server. I see the
same need here.

> > Complete with full docs and comments.
> > 
> > Barring resolving a few points and subject to even more testing, this is
> > the version I expect to commit to CVS on Wednesday. I would appreciate
> > further objective testing before commit, if possible.
> 
> * Please remove any spurious whitespace.  "git diff --color" makes them
> stand out like a sore thumb, in red. (pgindent will fix them but always
> better to fix them before committing, IMO).

What is your definition of spurious whitespace? I removed all
additions/deletions of individual lines. git diff colours many things
red, and it seems like a waste of time to spend hours fiddling with
spaces manually if there is a utility that does this anyway.

> * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
> on your transaction rate to begin with. Do we really want this setting
> in its current form? Does it make sense as PGC_USERSET, as if one
> backend uses a lower setting than others, that's the one that really
> determines when transactions are killed in the standby? I think this
> should be discussed and implemented as a separate patch.

All the vacuum_*_age parameters have this characteristic, yet they
exist.

I would like to provide simple features for conflict avoidance in the
first alpha release. If we find that nobody found it useful then it can
be removed easily enough.

It's a USERSET now, but it could also be other things. This patch isn't
the end of discussion, for many people it will be the start.

> * Are you planning to remove the recovery_connections setting before
> release? The documentation makes it sound like it's a temporary hack
> that we're not really sure is needed at all. That's not very comforting.

It has been requested and I agree, so its there. Saying it might be
removed in future is no more than we do elsewhere and AFAIK we all hope
it will be. Not sure why that is or isn't comforting.

> * You removed this comment from KnownAssignedXidsInit:
> 
> -   /*
> -* XXX: We should check that we don't exceed maxKnownAssignedXids.
> -* Even though the hash table might hold a few more entries than that,
> -* we use fixed-size arrays of that size elsewhere and expected all
> -* entries in the hash table to fit.
> -*/
> 
> but AFAICS you didn't address the issue. It's referring to the 'xids'
> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
> in without checking that it fits.

I have ensured that they are always the same size, by definition, so no
need to check.

> * LockAcquireExtended needs a function comment. Or at least something
> explaining what report_memory_error does. And perhaps the argument
> should be named "reportMemoryError" to be in line with the other arguments.

OK

> * We tend to not add remarks about authors in code (comments in standby.c).

OK

> * This optimization in GetConflictingVirtualXIDs():
> 
> > +   /*
> > +* If we don't know the TransactionId that created the conflict, set
> > +* it to latestCompletedXid which is the latest possible value.
> > +*/
> > +   if (!TransactionIdIsValid(limitXmin))
> > +   limitXmin = ShmemVariableCache->latestCompletedXid;
> > +
> 
> needs a lot more explanation. It took me a very long time to figure out
> why using latest completed xid is safe.

OK. Took me a long time as well.

> * Are you going to leave the trace_recovery GUC in?

For now, at least. I have a later proposal around that to follow.

> * Can you merge with CVS HEAD, please? There's some merge conflicts.

Huh? I did. And tested p

Re: [HACKERS] Hot Standby, release candidate?

[Magnus Hagander]

On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
 wrote:
> * Please remove any spurious whitespace.  "git diff --color" makes them
> stand out like a sore thumb, in red. (pgindent will fix them but always
> better to fix them before committing, IMO).

+1 in general, not particularly for this patch (haven't checked that
in this patch).

Actually, how about we add that to the page at
http://wiki.postgresql.org/wiki/Submitting_a_Patch?


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

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

Re: [HACKERS] Hot Standby, release candidate?

[Heikki Linnakangas]

Simon Riggs wrote:
> Enclose latest version of Hot Standby. This is the "basic" patch, with
> no must-fix issues and no known bugs. Further additions will follow
> after commit, primarily around usability, which will include additional
> control functions for use in testing. Various thoughts discussed here
> http://wiki.postgresql.org/wiki/Hot_Standby_TODO

I still consider it highly important to be able to start standby from a
shutdown checkpoint. If you remove it, at the very least put it back on
the TODO.

But as it is, StandbyRecoverPreparedTransactions() is dead code, and the
changes to PrescanPreparedTransactions() are not needed either.

> Patch now includes a set of regression tests that can be run against a
> standby server using "make standbycheck"

Nice!

> (requires setup, see src/test/regress/standby_schedule). 

Any chance of automating that?

> Complete with full docs and comments.
> 
> Barring resolving a few points and subject to even more testing, this is
> the version I expect to commit to CVS on Wednesday. I would appreciate
> further objective testing before commit, if possible.

* Please remove any spurious whitespace.  "git diff --color" makes them
stand out like a sore thumb, in red. (pgindent will fix them but always
better to fix them before committing, IMO).

* vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
on your transaction rate to begin with. Do we really want this setting
in its current form? Does it make sense as PGC_USERSET, as if one
backend uses a lower setting than others, that's the one that really
determines when transactions are killed in the standby? I think this
should be discussed and implemented as a separate patch.

* Are you planning to remove the recovery_connections setting before
release? The documentation makes it sound like it's a temporary hack
that we're not really sure is needed at all. That's not very comforting.

* You removed this comment from KnownAssignedXidsInit:

-   /*
-* XXX: We should check that we don't exceed maxKnownAssignedXids.
-* Even though the hash table might hold a few more entries than that,
-* we use fixed-size arrays of that size elsewhere and expected all
-* entries in the hash table to fit.
-*/

but AFAICS you didn't address the issue. It's referring to the 'xids'
array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
in without checking that it fits.

* LockAcquireExtended needs a function comment. Or at least something
explaining what report_memory_error does. And perhaps the argument
should be named "reportMemoryError" to be in line with the other arguments.

* We tend to not add remarks about authors in code (comments in standby.c).

* This optimization in GetConflictingVirtualXIDs():

> +   /*
> +* If we don't know the TransactionId that created the conflict, set
> +* it to latestCompletedXid which is the latest possible value.
> +*/
> +   if (!TransactionIdIsValid(limitXmin))
> +   limitXmin = ShmemVariableCache->latestCompletedXid;
> +

needs a lot more explanation. It took me a very long time to figure out
why using latest completed xid is safe.

* Are you going to leave the trace_recovery GUC in?

* Can you merge with CVS HEAD, please? There's some merge conflicts.

> Last remaining points
> 
> * NonTransactionalInvalidation logging has been removed following
> review, but AFAICS that means VACUUM FULL doesn't work correctly on
> catalog tables, which regrettably will be the only ones still standing
> even after we apply VFI patch. Did I misunderstand the original intent?
> Was it just buggy somehow? Or is this hoping VF goes completely, which
> seems unlikely in this release. Just noticed this, decided better to get
> stuff out there now.

I removed it in the hope that VF is gone before beta.

> * Are we OK with using hash indexes in standby plans, even when we know
> the indexes are stale and could return incorrect results?

It doesn't seem any more wrong than using hash indexes right after
recovery, crash recovery or otherwise. It's certainly broken, but I
don't see much value in a partial fix. The bottom line is that hash
indexes should be WAL-logged.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby, release candidate?

[Peter Eisentraut]

On mån, 2009-12-14 at 08:54 +, Simon Riggs wrote:
> Wednesday because that seemed a good delay to allow review. Josh and I
> had discussed the value of getting patch into Alpha3, so that was my
> wish and aim.
> 
> I'm not aware of any particular date for end of commitfest, though
> possibly you are about to update me on that?

Commit fests for 8.5 have usually run from the 15th to the 15th of next
month, but the CF manager may choose to vary that.

FWIW, the alpha release manager may also vary the release timeline of
alpha3. ;-)

> (Perhaps we really need a single Project Management page that lists all
> the dates that have been agreed, so that everybody can go there and be
> clear. Commitfest start and end dates, beta dates, de-support dates etc.
> BTW, it is absolutely brilliant that we have these now.)

We had
, but I
don't think we ever actually agreed on a schedule for 8.5.



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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Mon, 2009-12-14 at 10:11 +0200, Peter Eisentraut wrote:
> On sön, 2009-12-13 at 19:20 +, Simon Riggs wrote:
> > Barring resolving a few points and subject to even more testing, this
> > is the version I expect to commit to CVS on Wednesday.
> 
> To clarify: Did you pick Wednesday so that it gets included before the
> end of the commit fest (and thus into alpha3) or so that it gets into
> CVS as early as possible after the commit fest (and thus not into
> alpha3)?

Wednesday because that seemed a good delay to allow review. Josh and I
had discussed the value of getting patch into Alpha3, so that was my
wish and aim.

I'm not aware of any particular date for end of commitfest, though
possibly you are about to update me on that?

(Perhaps we really need a single Project Management page that lists all
the dates that have been agreed, so that everybody can go there and be
clear. Commitfest start and end dates, beta dates, de-support dates etc.
BTW, it is absolutely brilliant that we have these now.)

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Peter Eisentraut]

On sön, 2009-12-13 at 19:20 +, Simon Riggs wrote:
> Barring resolving a few points and subject to even more testing, this
> is the version I expect to commit to CVS on Wednesday.

To clarify: Did you pick Wednesday so that it gets included before the
end of the commit fest (and thus into alpha3) or so that it gets into
CVS as early as possible after the commit fest (and thus not into
alpha3)?


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

Re: [HACKERS] Hot Standby, release candidate?

[Simon Riggs]

On Sun, 2009-12-13 at 15:45 -0500, Tom Lane wrote:
> Simon Riggs  writes:
> > * NonTransactionalInvalidation logging has been removed following
> > review, but AFAICS that means VACUUM FULL doesn't work correctly on
> > catalog tables, which regrettably will be the only ones still standing
> > even after we apply VFI patch. Did I misunderstand the original intent?
> > Was it just buggy somehow? Or is this hoping VF goes completely, which
> > seems unlikely in this release.
> 
> For my money, the only reason VF is still around is there hasn't been
> an urgent reason to get rid of it.  If it doesn't play with HS, I think
> we'd be better served to put work into getting rid of it than to put
> work into fixing it.

I see the logic, though it has many implications. I'll step up, if I can
get some help from you and Itagaki on the VF side.

You have a rough design here
http://archives.postgresql.org/message-id/19750.1252094...@sss.pgh.pa.us

Some thoughts and some further work on a detailed design

* Which exact tables are we talking about: just pg_class and the shared
catalogs? Everything else is in pg_class, so if we can find it we're OK?
formrdesc() tells me the list of nailed relations is: pg_database,
pg_class, pg_attribute, pg_proc, and pg_type. Are the nailed relations
the ones we care about, or are they just a subset? 

* Restrict set of operations to *only* VACUUM FULL. Is there a need for
anything else to do this, at least in this release?

* Each backend needs to access two map files: shared and local

* Get relcache to read map files at startup in formrdesc(). Rather than
use RelationInitPhysicalAddr() set relation->rd_node.relNode directly

* Get VF to write a new type of invalidation message that means re-read
the two map files to overwrite the relation->rd_node.relNode in the
nailed relations

* Map files would have a very structured format, so each table listed
has its exact place. Sounds like best place for shared catalogs is
pg_control. We only need a few additional bytes for that and everything
else to manipulate it already exists.

* Map files for specific databases would be called pg_database_control,
with roughly same concepts as pg_control. It's then an obvious place to
add any further db specific things in future, if we need them.

* Protect all map files reading/writing using ControlFileLock. Sequence
of update is acquire lock, send invalidation, rewrite file, release lock
all inside a critical section. Readers would take shared, writers
exclusive.

* Work would be in two tranches: add new way of working then later
remove code we don't need; I would actually rather do the second part at
start of next dev cycle.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby, release candidate?

[Tom Lane]

Simon Riggs  writes:
> * NonTransactionalInvalidation logging has been removed following
> review, but AFAICS that means VACUUM FULL doesn't work correctly on
> catalog tables, which regrettably will be the only ones still standing
> even after we apply VFI patch. Did I misunderstand the original intent?
> Was it just buggy somehow? Or is this hoping VF goes completely, which
> seems unlikely in this release.

For my money, the only reason VF is still around is there hasn't been
an urgent reason to get rid of it.  If it doesn't play with HS, I think
we'd be better served to put work into getting rid of it than to put
work into fixing it.

regards, tom lane

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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Mon, 2009-12-07 at 10:02 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Sun, 2009-12-06 at 17:26 -0500, Robert Haas wrote:
> > 
> >> For what it's worth, this doesn't seem particularly unlikely or
> >> unusual to me.
> > 
> > I don't know many people who shutdown both nodes of a highly available
> > application at the same time. If they did, I wouldn't expect them to
> > complain they couldn't run queries on the standby when an two obvious
> > and simple workarounds exist to allow them to access their data: start
> > the master again, or make the standby switchover, both of which are part
> > of standard operating procedures.
> 
> It might not be common or expected in a typical HA installation, but it
> would be a very strange limitation in my mind. It might well happen e.g
> in a standby used for reporting, or when you do PITR.

Yes, its possible that the standby can be shutdown while disconnected
from a master. If that occurs, all we are saying is that we cannot use
shutdown checkpoints as starting points. If the primary was set up in
default mode, then wal_standby_info = on and so there will be
running_xact WAL records immediately after each checkpoint to use as
starting points. We don't need shutdown checkpoints as well.

So adding shutdown checkpoints as a valid starting place does not help
in this case, it just makes the code harder to test.

> > It doesn't seem very high up the list of additional features, at least.
> 
> Well, it's in the patch now. I'm just asking you to not break it.

There's been many things in the patch that have been removed. This is by
far the least important of the things removed in the name of getting
this done as soon as possible, with good quality. I see no reason for
your eagerness to include this feature. I have removed it; as you say,
its in the repo if someone wants to add it in later.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Heikki Linnakangas]

Simon Riggs wrote:
> On Sun, 2009-12-06 at 17:26 -0500, Robert Haas wrote:
> 
>> For what it's worth, this doesn't seem particularly unlikely or
>> unusual to me.
> 
> I don't know many people who shutdown both nodes of a highly available
> application at the same time. If they did, I wouldn't expect them to
> complain they couldn't run queries on the standby when an two obvious
> and simple workarounds exist to allow them to access their data: start
> the master again, or make the standby switchover, both of which are part
> of standard operating procedures.

It might not be common or expected in a typical HA installation, but it
would be a very strange limitation in my mind. It might well happen e.g
in a standby used for reporting, or when you do PITR.

> It doesn't seem very high up the list of additional features, at least.

Well, it's in the patch now. I'm just asking you to not break it.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 17:26 -0500, Robert Haas wrote:

> For what it's worth, this doesn't seem particularly unlikely or
> unusual to me.

I don't know many people who shutdown both nodes of a highly available
application at the same time. If they did, I wouldn't expect them to
complain they couldn't run queries on the standby when an two obvious
and simple workarounds exist to allow them to access their data: start
the master again, or make the standby switchover, both of which are part
of standard operating procedures.

It doesn't seem very high up the list of additional features, at least.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Dimitri Fontaine]

Le 6 déc. 2009 à 23:26, Robert Haas a écrit :
>>> Consider this scenario:
>>> 
>>> 0. You have a master and a standby configured properly, and up and running.
>>> 1. You shut down master for some reason.
>>> 2. You restart standby. For some reason. Maybe by accident, or you want
>>> to upgrade minor version or whatever.
>>> 3. Standby won't accept connections until the master is started too.
>>> Admin says "WTF?"
>> 
>> I would rather document it as a known caveat and be done.

+1

> For what it's worth, this doesn't seem particularly unlikely or
> unusual to me.

I'm sorry to have to disagree here. Shutting down the master in my book means 
you're upgrading it, minor or major or the box under it. It's not a frequent 
event. More frequent than a crash but still.

Now the master is offline, you have a standby, and you're restarting it too, 
but you don't mean that as a switchover. I'm with Simon here, the WTF is "are 
you in search of trouble?" more than anything else. I don't think shutting down 
the standby while the master is offline is considered as a good practice if 
your goal is HA...

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

Re: [HACKERS] Hot standby, recent changes

[Robert Haas]

On Sun, Dec 6, 2009 at 3:06 PM, Simon Riggs  wrote:
> On Sun, 2009-12-06 at 20:32 +0200, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>> > On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:
>> >> 4. Need to handle the case where master is started up with
>> >> wal_standby_info=true, shut down, and restarted with
>> >> wal_standby_info=false, while the standby server runs continuously. And
>> >> the code in StartupXLog() to initialize recovery snapshot from a
>> >> shutdown checkpoint needs to check that too.
>> >
>> > I don't really understand the use case for shutting down the server and
>> > then using it as a HS base backup. Why would anyone do that? Why would
>> > they have their server down for potentially hours, when they can take
>> > the backup while the server is up? If the server is idle, it can be
>> > up-and-idle just as easily as down-and-idle, in which case we wouldn't
>> > need to support this at all. Adding yards of code for this capability
>> > isn't important to me. I'd rather strip the whole lot out than keep
>> > fiddling with a low priority area. Please justify this as a real world
>> > solution before we continue trying to support it.
>>
>> This affects using a shutdown checkpoint as a recovery start point
>> (restore point) in general, not just a fresh backup taken from a shut
>> down database.
>>
>> Consider this scenario:
>>
>> 0. You have a master and a standby configured properly, and up and running.
>> 1. You shut down master for some reason.
>> 2. You restart standby. For some reason. Maybe by accident, or you want
>> to upgrade minor version or whatever.
>> 3. Standby won't accept connections until the master is started too.
>> Admin says "WTF?"
>
> OK, I accept that as a possible scenario.
>
> I'm concerned that the number of possible scenarios we are trying to
> support in the first version is too high, increasing the likelihood that
> some of them do not work correctly because the test scenarios didn't
> re-create them exactly.
>
> In the above scenario, if it is of serious concern the admin can
> failover to the standby and then access the standby for queries. If the
> master was down for any length of time that's exactly what we'd expect
> to happen anyway.
>
> So I don't see the scenario as very likely; I'm sure it will happen to
> somebody. In any case, it is in the opposite direction to the main
> feature, which is a High Availability server, so any admin that argued
> that he wanted to have his HA standby stay as a standby in this case
> would likely be in a minority.
>
> I would rather document it as a known caveat and be done.

For what it's worth, this doesn't seem particularly unlikely or
unusual to me.  But on the other hand, I do really want to get this
committed soon, and I don't have time to help at the moment, so maybe
I should keep my mouth shut.

...Robert

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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 20:32 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:
> >> 4. Need to handle the case where master is started up with
> >> wal_standby_info=true, shut down, and restarted with
> >> wal_standby_info=false, while the standby server runs continuously. And
> >> the code in StartupXLog() to initialize recovery snapshot from a
> >> shutdown checkpoint needs to check that too.
> > 
> > I don't really understand the use case for shutting down the server and
> > then using it as a HS base backup. Why would anyone do that? Why would
> > they have their server down for potentially hours, when they can take
> > the backup while the server is up? If the server is idle, it can be
> > up-and-idle just as easily as down-and-idle, in which case we wouldn't
> > need to support this at all. Adding yards of code for this capability
> > isn't important to me. I'd rather strip the whole lot out than keep
> > fiddling with a low priority area. Please justify this as a real world
> > solution before we continue trying to support it.
> 
> This affects using a shutdown checkpoint as a recovery start point
> (restore point) in general, not just a fresh backup taken from a shut
> down database.
> 
> Consider this scenario:
> 
> 0. You have a master and a standby configured properly, and up and running.
> 1. You shut down master for some reason.
> 2. You restart standby. For some reason. Maybe by accident, or you want
> to upgrade minor version or whatever.
> 3. Standby won't accept connections until the master is started too.
> Admin says "WTF?"

OK, I accept that as a possible scenario.

I'm concerned that the number of possible scenarios we are trying to
support in the first version is too high, increasing the likelihood that
some of them do not work correctly because the test scenarios didn't
re-create them exactly.

In the above scenario, if it is of serious concern the admin can
failover to the standby and then access the standby for queries. If the
master was down for any length of time that's exactly what we'd expect
to happen anyway. 

So I don't see the scenario as very likely; I'm sure it will happen to
somebody. In any case, it is in the opposite direction to the main
feature, which is a High Availability server, so any admin that argued
that he wanted to have his HA standby stay as a standby in this case
would likely be in a minority.

I would rather document it as a known caveat and be done.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Heikki Linnakangas]

Simon Riggs wrote:
> On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:
>> 4. Need to handle the case where master is started up with
>> wal_standby_info=true, shut down, and restarted with
>> wal_standby_info=false, while the standby server runs continuously. And
>> the code in StartupXLog() to initialize recovery snapshot from a
>> shutdown checkpoint needs to check that too.
> 
> I don't really understand the use case for shutting down the server and
> then using it as a HS base backup. Why would anyone do that? Why would
> they have their server down for potentially hours, when they can take
> the backup while the server is up? If the server is idle, it can be
> up-and-idle just as easily as down-and-idle, in which case we wouldn't
> need to support this at all. Adding yards of code for this capability
> isn't important to me. I'd rather strip the whole lot out than keep
> fiddling with a low priority area. Please justify this as a real world
> solution before we continue trying to support it.

This affects using a shutdown checkpoint as a recovery start point
(restore point) in general, not just a fresh backup taken from a shut
down database.

Consider this scenario:

0. You have a master and a standby configured properly, and up and running.
1. You shut down master for some reason.
2. You restart standby. For some reason. Maybe by accident, or you want
to upgrade minor version or whatever.
3. Standby won't accept connections until the master is started too.
Admin says "WTF?"

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 10:51 +, Simon Riggs wrote:

> > 5. You removed this comment from KnownAssignedXidsAdd:
> > 
> > -   /*
> > -* XXX: We should check that we don't exceed maxKnownAssignedXids.
> > -* Even though the hash table might hold a few more entries than that,
> > -* we use fixed-size arrays of that size elsewhere and expected all
> > -* entries in the hash table to fit.
> > -*/
> > 
> > I think the issue still exists. The comment refers to
> > KnownAssignedXidsGet, which takes as an argument an array that has to be
> > large enough to hold all entries in the known-assigned hash table. If
> > there's more entries than expected in the hash table,
> > KnownAssignedXidsGet will overrun the array. Perhaps
> > KnownAssignedXidsGet should return a palloc'd array instead or
> > something, but I don't think it's fine as it is.
> 
> Same issue perhaps, different place.

Well, its not same issue. One was about entering things into a shared
memory hash table, one is about reading values into an locally malloc'd
array. The array is sized as the max size of KnownAssignedXids, so there
is no danger that there would ever be an overrun.

One caller does not set the max size explicitly, so I will add a
comment/code changes to make that clearer.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 11:20 +, Simon Riggs wrote:
> On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:
> 
> > 3. The "Out of lock mem killer" in StandbyAcquireAccessExclusiveLock is
> > quite harsh. It aborts all read-only transactions. It should be enough
> > to kill just one random one, or maybe the one that's holding most locks.
> > Also, if there still isn't enough shared memory after killing all
> > backends, it goes into an infinite loop. I guess that shouldn't happen,
> > but it seems a bit squishy anyway. It would be nice to differentiate
> > between "out of shared mem" and "someone else is holding the lock" more
> > accurately. Maybe add a new return value to LockAcquire() for "out of
> > shared mem".
> 
> OK, will abort infinite loop by adding new return value.

You had me for a minute, but there is no infinite loop. Once we start
killing people it reverts to throwing an ERROR on an out of memory
condition.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:

> 3. The "Out of lock mem killer" in StandbyAcquireAccessExclusiveLock is
> quite harsh. It aborts all read-only transactions. It should be enough
> to kill just one random one, or maybe the one that's holding most locks.
> Also, if there still isn't enough shared memory after killing all
> backends, it goes into an infinite loop. I guess that shouldn't happen,
> but it seems a bit squishy anyway. It would be nice to differentiate
> between "out of shared mem" and "someone else is holding the lock" more
> accurately. Maybe add a new return value to LockAcquire() for "out of
> shared mem".

OK, will abort infinite loop by adding new return value.

If people don't like having everything killed, they can add more lock
memory. It isn't worth adding more code because its hard to test and
unlikely to be an issue in real usage, and it has a clear workaround
that is already mentioned in a hint.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:
> 1. The XLogFlush() call you added to dbase_redo doesn't help where it
> is. You need to call XLogFlush() after the *commit* record of the DROP
> DATABASE. The idea is minimize the window where the files have already
> been deleted but the entry in pg_database is still visible, if someone
> kills the standby and starts it up as a new master. This isn't really
> hot standby's fault, you have the same window with PITR, so I think it
> would be better to handle this as a separate patch. Also, please handle
> all the commands that use ForceSyncCommit().

I think I'll just add a flag to the commit record to do this. That way
anybody that sets ForceSyncCommit will do as you propose.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, recent changes

[Simon Riggs]

On Sun, 2009-12-06 at 12:32 +0200, Heikki Linnakangas wrote:

> 2. "Allow RULEs ON INSERT, ON UPDATE and ON DELETE iff they generate
> only SELECT statements that act INSTEAD OF the actual event." This
> affects any read-only transaction, not just hot standby, so I think this
> should be discussed and changed as separate patch.

OK

> 4. Need to handle the case where master is started up with
> wal_standby_info=true, shut down, and restarted with
> wal_standby_info=false, while the standby server runs continuously. And
> the code in StartupXLog() to initialize recovery snapshot from a
> shutdown checkpoint needs to check that too.

I don't really understand the use case for shutting down the server and
then using it as a HS base backup. Why would anyone do that? Why would
they have their server down for potentially hours, when they can take
the backup while the server is up? If the server is idle, it can be
up-and-idle just as easily as down-and-idle, in which case we wouldn't
need to support this at all. Adding yards of code for this capability
isn't important to me. I'd rather strip the whole lot out than keep
fiddling with a low priority area. Please justify this as a real world
solution before we continue trying to support it.

> 5. You removed this comment from KnownAssignedXidsAdd:
> 
> -   /*
> -* XXX: We should check that we don't exceed maxKnownAssignedXids.
> -* Even though the hash table might hold a few more entries than that,
> -* we use fixed-size arrays of that size elsewhere and expected all
> -* entries in the hash table to fit.
> -*/
> 
> I think the issue still exists. The comment refers to
> KnownAssignedXidsGet, which takes as an argument an array that has to be
> large enough to hold all entries in the known-assigned hash table. If
> there's more entries than expected in the hash table,
> KnownAssignedXidsGet will overrun the array. Perhaps
> KnownAssignedXidsGet should return a palloc'd array instead or
> something, but I don't think it's fine as it is.

Same issue perhaps, different place.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, misc issues

[Simon Riggs]

On Sat, 2009-12-05 at 22:56 +0200, Heikki Linnakangas wrote:

> So that RecordKnownAssignedTransactionIds() call needs to be put back.

OK

> BTW, if you want to resurrect the check in KnownAssignedXidsRemove(),
> you also need to not complain before you reach the running-xacts record
> and open up for read-only connections.

Yep

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby, misc issues

[Heikki Linnakangas]

Simon Riggs wrote:
> On Fri, 2009-12-04 at 10:23 +0200, Heikki Linnakangas wrote:
>>> @Heikki: Why is error checking in KnownAssignedXidsRemove() #ifdef'd
>> out?? 
>>
>> It's explained in the comment:
>> /* XXX: This can still happen: If a transaction with a subtransaction
>>  * that haven't been reported yet aborts, and no WAL records have been
>>  * written using the subxid, the abort record will contain that subxid
>>  * and we haven't seen it before.
>>  */
> 
> Just realised that this occurs again because the call to
> RecordKnownAssignedTransactionIds() was removed from
> xact_commit_abort().
> 
> I'm guessing you didn't like the call in that place for some reason,
> since I smile while I remember it has been removed twice(!) even though
> I put "do not remove" comments on it to describe this corner case.
> 
> Not going to put it back a third time.

:-). Well, it does seem pointless to add entries to the hash table, just
to remove them at the very next line. But you're right, we should still
advance latestObservedXid, and if we do that, we need to memorize any
not-yet-seen XIDs in the known-assigned xids array. So that
RecordKnownAssignedTransactionIds() call needs to be put back.

BTW, if you want to resurrect the check in KnownAssignedXidsRemove(),
you also need to not complain before you reach the running-xacts record
and open up for read-only connections.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby, misc issues

[Simon Riggs]

On Fri, 2009-12-04 at 10:23 +0200, Heikki Linnakangas wrote:
> 
> > @Heikki: Why is error checking in KnownAssignedXidsRemove() #ifdef'd
> out?? 
> 
> It's explained in the comment:
> /* XXX: This can still happen: If a transaction with a subtransaction
>  * that haven't been reported yet aborts, and no WAL records have been
>  * written using the subxid, the abort record will contain that subxid
>  * and we haven't seen it before.
>  */

Just realised that this occurs again because the call to
RecordKnownAssignedTransactionIds() was removed from
xact_commit_abort().

I'm guessing you didn't like the call in that place for some reason,
since I smile while I remember it has been removed twice(!) even though
I put "do not remove" comments on it to describe this corner case.

Not going to put it back a third time.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Kevin Grittner]

Heikki Linnakangas  wrote:
 
> If the system is completely idle, and no WAL is written, we skip
> xlog switches too, even if archive_timeout is set . It would be
> pointless to create a stream of WAL files with no content except
> for the XLOG_SWITCH records.
 
It's not pointless if you want to monitor that your backup system is
healthy.  This was previously mentioned a couple years ago:
 
http://archives.postgresql.org/pgsql-general/2007-10/msg01448.php
 
It turns out that it's been working fine under 8.3.  Of course, we
can always add a crontab job to do some small bogus update to force
WAL switches, so it's not the end of the world if we lose the 8.3
behavior; but my preference would be that if a WAL switch interval
is specified, the WAL files switch at least that often.
 
-Kevin

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Fri, 2009-12-04 at 13:31 +0200, Heikki Linnakangas wrote:

> b) seems much simpler to me.

OK. Least ugly wins, but she ain't pretty.

> > 2. (In HS recovery) When we see first commit record for the VF xid we
> > commit the transaction in clog, yet maintain locks and KnownAssigned
> > xids
> > 
> > 3. (In HS recovery) When we see second commit record for the VF xid we
> > skip clog updates but then perform remaining parts of commit.
> 
> I's harmless to set a clog entry as committed twice, so you can treat
> the 2nd commit record the same as a regular commit record.

Yeh, OK, it is harmless and makes code cleaner.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Heikki Linnakangas]

Simon Riggs wrote:
> ISTM premature to remove all traces of VF from code. We may yet need it
> for some reason, especially if doing so creates complex dependencies on
> important features.

Well, it's still in the repository.

> So modified proposal looks like this
> 
> 1. (In normal running) Provide information to HS so it can identify VF
> commit records.
> Implement in code either
> a) Just before we issue the first VFI commit we send a new type of WAL
> message to indicate VFI-in-progress, including the xid. 
> b) Alter the API of RecordTransactionCommit(), and send the info within
> the commit record. This was pretty much how we did that before. 
> I prefer (a) now because the ugliness is better isolated.

With a), you need to keep track of the seen VFI-in-progress records,
remember to expire the state at a shutdown record etc. And you have to
deal with the possibility that a checkpoint happens between the
VFI-in-progress record and the commit record; a recovery starting from
the checkpoint/running-xacts record must see both records or it will
release the locks prematurely.

b) seems much simpler to me.

> 2. (In HS recovery) When we see first commit record for the VF xid we
> commit the transaction in clog, yet maintain locks and KnownAssigned
> xids
> 
> 3. (In HS recovery) When we see second commit record for the VF xid we
> skip clog updates but then perform remaining parts of commit.

I's harmless to set a clog entry as committed twice, so you can treat
the 2nd commit record the same as a regular commit record.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Fri, 2009-12-04 at 11:22 +0200, Heikki Linnakangas wrote:

> > My answer is it doesn't, we will still have the problem noted above for
> > catalog tables. So we still have a must-fix issue for HS, though that is
> > no barrier to the new VF patch.
> 
> I think the VACUUM FULL patch will have to address that. Either with the
> flat-file approach Tom suggested, or by disallowing VACUUM FULL for
> catalog tables altogether, requiring you to use the to-be-written online
> tool that uses dummy UPDATEs to move tuples.

I don't see we need either of those, with the solution below.

ISTM premature to remove all traces of VF from code. We may yet need it
for some reason, especially if doing so creates complex dependencies on
important features.

> > Proposal:
> > 
> > * (In normal running) Just before we issue the first VFI commit we send
> > a new type of WAL message to indicate VFI-in-progress, including the xid
> > 
> > * (In HS recovery) When we see first commit record for the VF xid we
> > ignore it, as we did in the original patch
> > 
> > * (In HS recovery) When we see relation truncate for the xid we ignore
> > it for now, but defer until after the second commit is processed
> > 
> > It ain't that great, but it means all of the "hack" is isolated to
> > specific HS-only code, which can be turned off if required.
> 
> Could you just mark the transaction as committed when you see the 1st
> commit record, but leave the XID in the known-assigned list and not
> release locks? That would emulate pretty closely what happens in the master.

So modified proposal looks like this

1. (In normal running) Provide information to HS so it can identify VF
commit records.
Implement in code either
a) Just before we issue the first VFI commit we send a new type of WAL
message to indicate VFI-in-progress, including the xid. 
b) Alter the API of RecordTransactionCommit(), and send the info within
the commit record. This was pretty much how we did that before. 
I prefer (a) now because the ugliness is better isolated.

2. (In HS recovery) When we see first commit record for the VF xid we
commit the transaction in clog, yet maintain locks and KnownAssigned
xids

3. (In HS recovery) When we see second commit record for the VF xid we
skip clog updates but then perform remaining parts of commit.

Conceptually this splits a VF commit into two parts.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Fri, 2009-12-04 at 11:22 +0200, Heikki Linnakangas wrote:

> Could you just mark the transaction as committed when you see the 1st
> commit record, but leave the XID in the known-assigned list and not
> release locks? That would emulate pretty closely what happens in the master.

OK, works for me. Thanks.

I thought of that before and dismissed it, clearly before my morning
coffee, but it works because we still hold the lock on the table so
nobody can actually read the now visible new rows. Cool.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Heikki Linnakangas]

Simon Riggs wrote:
> I've just reviewed the VACUUM FULL patch to see if it does all we need
> it to do, i.e. does the removal of HS code match the new VF patch.

Oh, good!

> My answer is it doesn't, we will still have the problem noted above for
> catalog tables. So we still have a must-fix issue for HS, though that is
> no barrier to the new VF patch.

I think the VACUUM FULL patch will have to address that. Either with the
flat-file approach Tom suggested, or by disallowing VACUUM FULL for
catalog tables altogether, requiring you to use the to-be-written online
tool that uses dummy UPDATEs to move tuples.

> Requirement is that
> 
> * we ignore first commit, since it has an identical xid to second
> commit, so requires a special case to avoid breaking other checks
> 
> * we musn't truncate until we are certain transaction completes,
> otherwise we will have a data loss situation (isolated to this
> particular case only)
> 
> Proposal:
> 
> * (In normal running) Just before we issue the first VFI commit we send
> a new type of WAL message to indicate VFI-in-progress, including the xid
> 
> * (In HS recovery) When we see first commit record for the VF xid we
> ignore it, as we did in the original patch
> 
> * (In HS recovery) When we see relation truncate for the xid we ignore
> it for now, but defer until after the second commit is processed
> 
> It ain't that great, but it means all of the "hack" is isolated to
> specific HS-only code, which can be turned off if required.

Could you just mark the transaction as committed when you see the 1st
commit record, but leave the XID in the known-assigned list and not
release locks? That would emulate pretty closely what happens in the master.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Sat, 2009-11-21 at 20:20 +0200, Heikki Linnakangas wrote:

> VACUUM FULL does a peculiar hack: once it's done moving tuples, and
> before it truncates the relation, it calls RecordTransactionCommit to
> mark the transaction as committed in clog and WAL, but the transaction
> is still kept open in proc array. After it's done with truncating and
> other cleanup, normal transaction commit performs
> RecordTransactionCommit again as part of normal commit processing.
> 
> That causes some headaches for Hot Standby, ie. it needs to know to not
> release locks yet when it sees the first commit record. At the moment,
> it simply ignores the first commit record, but that's very dangerous. If
> failover happens after the standby has seen the truncation record, but
> not the 2nd commit record for the transaction, all data moved from the
> truncated part will lost.

...

> So I guess what I'm asking is: Does anyone see any show-stoppers in
> removing VACUUM FULL, and does anyone want to step up to the plate and
> promise to do it before release?

I've just reviewed the VACUUM FULL patch to see if it does all we need
it to do, i.e. does the removal of HS code match the new VF patch.

My answer is it doesn't, we will still have the problem noted above for
catalog tables. So we still have a must-fix issue for HS, though that is
no barrier to the new VF patch.

Requirement is that

* we ignore first commit, since it has an identical xid to second
commit, so requires a special case to avoid breaking other checks

* we musn't truncate until we are certain transaction completes,
otherwise we will have a data loss situation (isolated to this
particular case only)

Proposal:

* (In normal running) Just before we issue the first VFI commit we send
a new type of WAL message to indicate VFI-in-progress, including the xid

* (In HS recovery) When we see first commit record for the VF xid we
ignore it, as we did in the original patch

* (In HS recovery) When we see relation truncate for the xid we ignore
it for now, but defer until after the second commit is processed

It ain't that great, but it means all of the "hack" is isolated to
specific HS-only code, which can be turned off if required.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Simon Riggs wrote:
> On Fri, 2009-12-04 at 10:37 +0200, Heikki Linnakangas wrote:
>> Regarding this item from the wiki page:
>>> The "standby delay" is measured as current timestamp - timestamp of last 
>>> replayed commit record. If there's little activity in the master, that can 
>>> lead to surprising results. For example, imagine that max_standby_delay is 
>>> set to 8 hours. The standby is fully up-to-date with the master, and 
>>> there's no write activity in master. After 10 hours, a long reporting query 
>>> is started in the standby. Ten minutes later, a small transaction is 
>>> executed in the master that conflicts with the reporting query. I would 
>>> expect the reporting query to be canceled 8 hours after the conflicting 
>>> transaction began, but it is in fact canceled immediately, because it's 
>>> over 8 hours since the last commit record was replayed.
>>>
>>> * Simon says... changed to allow checkpoints to update 
>>> recoveryLastXTime (Simon DONE) 
>> Update recoveryLastXTime at checkpoints doesn't help when the master is
>> completely idle, because we skip checkpoints in that case. It's better
>> than nothing, of course.
> 
> Not if archive_timeout is set, which it would be in warm standby case.
> We can do even better than this with SR.

If the system is completely idle, and no WAL is written, we skip xlog
switches too, even if archive_timeout is set . It would be pointless to
create a stream of WAL files with no content except for the XLOG_SWITCH
records.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby remaining issues

[Simon Riggs]

On Fri, 2009-12-04 at 10:37 +0200, Heikki Linnakangas wrote:
> Regarding this item from the wiki page:
> > The "standby delay" is measured as current timestamp - timestamp of last 
> > replayed commit record. If there's little activity in the master, that can 
> > lead to surprising results. For example, imagine that max_standby_delay is 
> > set to 8 hours. The standby is fully up-to-date with the master, and 
> > there's no write activity in master. After 10 hours, a long reporting query 
> > is started in the standby. Ten minutes later, a small transaction is 
> > executed in the master that conflicts with the reporting query. I would 
> > expect the reporting query to be canceled 8 hours after the conflicting 
> > transaction began, but it is in fact canceled immediately, because it's 
> > over 8 hours since the last commit record was replayed.
> > 
> > * Simon says... changed to allow checkpoints to update 
> > recoveryLastXTime (Simon DONE) 
> 
> Update recoveryLastXTime at checkpoints doesn't help when the master is
> completely idle, because we skip checkpoints in that case. It's better
> than nothing, of course.

Not if archive_timeout is set, which it would be in warm standby case.
We can do even better than this with SR.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Regarding this item from the wiki page:
> The "standby delay" is measured as current timestamp - timestamp of last 
> replayed commit record. If there's little activity in the master, that can 
> lead to surprising results. For example, imagine that max_standby_delay is 
> set to 8 hours. The standby is fully up-to-date with the master, and there's 
> no write activity in master. After 10 hours, a long reporting query is 
> started in the standby. Ten minutes later, a small transaction is executed in 
> the master that conflicts with the reporting query. I would expect the 
> reporting query to be canceled 8 hours after the conflicting transaction 
> began, but it is in fact canceled immediately, because it's over 8 hours 
> since the last commit record was replayed.
> 
> * Simon says... changed to allow checkpoints to update recoveryLastXTime 
> (Simon DONE) 

Update recoveryLastXTime at checkpoints doesn't help when the master is
completely idle, because we skip checkpoints in that case. It's better
than nothing, of course.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Wed, 2009-11-25 at 03:12 +, Greg Stark wrote:
> On Wed, Nov 25, 2009 at 2:10 AM, Tom Lane  wrote:
> > As long as there's not anything the master actually does differently
> > then I can't see where there'd be any performance testing to do.  What's
> > bothering me about this is that it seems likely that we'll find places
> > where the master has to do things differently.  I'd rather we made the
> > status visible; if we get through a release cycle without needing to
> > check it, we can always take the function out again.  But if we don't,
> > and then find out midway through the 8.5 release cycle that we need to
> > be able to check it, things could be a bit sticky.
> 
> Well the only thing that's been discussed is having vacuum require a
> minimum age before considering a transaction visible to all to reduce
> the chance of conflicts on cleanup records. But that would require an
> actual tunable, not just a flag. And it's something that could
> conceivably be desirable even if you're not running a HS setup (if
> someone ever reimplements time travel for example).

I've added "vacuum_delay_cleanup_age = N", default 0 to implement this.
New name please.

This alters the return value of GetOldestXmin() and the setting of
RecentGlobalXmin by the value requested.

I've made this a SIGHUP, since it only has value if it affects all
users.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Simon Riggs wrote:
> Hmm, what happens if someone enables wal_standby_info in postgresql.conf
> while the server is shutdown. It would still be a valid starting point
> in that case.

Yeah, true.

> I'll just make a note, I think.

Yeah, a manual (or automatic, if you just wait) checkpoint will produce
a new checkpoint record showing that it's safe to start standby again.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby remaining issues

[Simon Riggs]

On Wed, 2009-12-02 at 16:41 +, Simon Riggs wrote:
> On Tue, 2009-12-01 at 20:26 +0200, Heikki Linnakangas wrote:
> > Simon Riggs wrote:
> > > commit 02c3eadb766201db084b668daa271db4a900adc9
> > > Author: Simon Riggs 
> > > Date:   Sat Nov 28 06:23:33 2009 +
> > > 
> > > Added wal_standby_info GUC to turn RM_STANDBY_ID messages on/off.
> > > Various comments added also.
> > > 
> > 
> > This patch makes it unsafe to start hot standby mode from a shutdown
> > checkpoint, because we don't know if wal_standby_info was enabled in the
> > master.

Hmm, what happens if someone enables wal_standby_info in postgresql.conf
while the server is shutdown. It would still be a valid starting point
in that case. I'll just make a note, I think.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Simon Riggs]

On Tue, 2009-12-01 at 20:26 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > commit 02c3eadb766201db084b668daa271db4a900adc9
> > Author: Simon Riggs 
> > Date:   Sat Nov 28 06:23:33 2009 +
> > 
> > Added wal_standby_info GUC to turn RM_STANDBY_ID messages on/off.
> > Various comments added also.
> > 
> 
> This patch makes it unsafe to start hot standby mode from a shutdown
> checkpoint, because we don't know if wal_standby_info was enabled in the
> master.

> I still don't think we need the GUC. 

If that's a good plan we can remove it in late beta. Let's keep it there
for now.

> But for future-proofing, perhaps we
> should add a flag to shutdown checkpoint records, indicating whether
> it's safe to start hot standby from it. That way, if we decide to add a
> GUC like that at a later stage, we don't need to change the on-disk format.

OK, I understand this now. Taken me a while, even though obvious now I
see.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Simon Riggs wrote:
> On Wed, 2009-12-02 at 12:49 +0200, Heikki Linnakangas wrote:
> 
>> If a read-only transaction holds a lot of locks, consuming so much
>> lock space that there's none left for the startup process to hold the
>> lock it wants, it will abort and bring down postmaster. The patch
>> attempts to kill any conflicting lockers, but those are handled fine
>> already (if there's any conflicting locks, LockAcquire will return
>> LOCKACQUIRE_NOT_AVAIL anyway). The problem is with non-conflicting
>> locks using up the lock space.
> 
> Oh dear, another "nuke 'em all from orbit" scenario. Will do.

Yeah. This case is much like the OOM killer on Linux. Not really "nuke
'em all" but "nuke someone, don't care who"..

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby remaining issues

[Simon Riggs]

On Wed, 2009-12-02 at 12:49 +0200, Heikki Linnakangas wrote:

> If a read-only transaction holds a lot of locks, consuming so much
> lock space that there's none left for the startup process to hold the
> lock it wants, it will abort and bring down postmaster. The patch
> attempts to kill any conflicting lockers, but those are handled fine
> already (if there's any conflicting locks, LockAcquire will return
> LOCKACQUIRE_NOT_AVAIL anyway). The problem is with non-conflicting
> locks using up the lock space.

Oh dear, another "nuke 'em all from orbit" scenario. Will do.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Heikki Linnakangas wrote:
> Simon Riggs wrote:
>> @@ -654,10 +656,13 @@ LockAcquire(const LOCKTAG *locktag,
>>  elog(PANIC, "lock table corrupted");
>>  }
>>  LWLockRelease(partitionLock);
>> -ereport(ERROR,
>> -(errcode(ERRCODE_OUT_OF_MEMORY),
>> - errmsg("out of shared memory"),
>> -  errhint("You might need to increase 
>> max_locks_per_transaction.")));
>> +if (reportLockTableError)
>> +ereport(ERROR,
>> +(errcode(ERRCODE_OUT_OF_MEMORY),
>> + errmsg("out of shared memory"),
>> +  errhint("You might need to increase 
>> max_locks_per_transaction.")));
>> +else
>> +return LOCKACQUIRE_NOT_AVAIL;
>>  }
>>  locallock->proclock = proclock;
>>  
> 
> That seems dangerous when dontWait==false.

Ah, I see now that you're only setting reportLockTableError just before
you call LockAcquire, and reset it afterwards. It's safe then, but it
should rather be another argument to the function, as how the global
variable is really being used.

The patch doesn't actually fix the issue it was supposed to fix. If a
read-only transaction holds a lot of locks, consuming so much lock space
that there's none left for the startup process to hold the lock it
wants, it will abort and bring down postmaster. The patch attempts to
kill any conflicting lockers, but those are handled fine already (if
there's any conflicting locks, LockAcquire will return
LOCKACQUIRE_NOT_AVAIL anyway). The problem is with non-conflicting locks
using up the lock space.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Simon Riggs wrote:
> commit 02c3eadb766201db084b668daa271db4a900adc9
> Author: Simon Riggs 
> Date:   Sat Nov 28 06:23:33 2009 +
> 
> Added wal_standby_info GUC to turn RM_STANDBY_ID messages on/off.
> Various comments added also.
> 

This patch makes it unsafe to start hot standby mode from a shutdown
checkpoint, because we don't know if wal_standby_info was enabled in the
master.

I still don't think we need the GUC. But for future-proofing, perhaps we
should add a flag to shutdown checkpoint records, indicating whether
it's safe to start hot standby from it. That way, if we decide to add a
GUC like that at a later stage, we don't need to change the on-disk format.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby remaining issues

[Heikki Linnakangas]

Simon Riggs wrote:
> @@ -654,10 +656,13 @@ LockAcquire(const LOCKTAG *locktag,
>   elog(PANIC, "lock table corrupted");
>   }
>   LWLockRelease(partitionLock);
> - ereport(ERROR,
> - (errcode(ERRCODE_OUT_OF_MEMORY),
> -  errmsg("out of shared memory"),
> -   errhint("You might need to increase 
> max_locks_per_transaction.")));
> + if (reportLockTableError)
> + ereport(ERROR,
> + (errcode(ERRCODE_OUT_OF_MEMORY),
> +  errmsg("out of shared memory"),
> +   errhint("You might need to increase 
> max_locks_per_transaction.")));
> + else
> + return LOCKACQUIRE_NOT_AVAIL;
>   }
>   locallock->proclock = proclock;
>  

That seems dangerous when dontWait==false.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby remaining issues

[Simon Riggs]

On Wed, 2009-11-25 at 13:00 +0200, Heikki Linnakangas wrote:
> I've put up a wiki page with the issues I see with the patch as it
> stands. They're roughly categorized by seriousness.
> 
> http://wiki.postgresql.org/wiki/Hot_Standby_TODO
> 
> New issues can and probably will still pop up, let's add them to the
> list as they're found so that we know what still needs to be done.
> 
> You had a list of work items at the hot standby main page, but I believe
> it's badly out-of-date. Please move any still relevant items to the
> above list, if any.

4 changes on TODO included here, including all must-fix issues. This is
a combined patch. Will push changes to git also, so each commit is
visible separately.

Lots of wiki comments added.

-- 
 Simon Riggs   www.2ndQuadrant.com
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 75aca23..5af05fe 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1985,7 +1985,7 @@ if (!triggered)
  
 	 
 	  
-   LOCK TABLE, though only when explicitly IN ACCESS SHARE MODE
+   LOCK TABLE, though only when explicitly in one of these modes: ACCESS SHARE, ROW SHARE or ROW EXCLUSIVE.
   
  
 	 
@@ -2033,7 +2033,7 @@ if (!triggered)
 	 
 	  
LOCK TABLE, in short default form, since it requests ACCESS EXCLUSIVE MODE.
-   LOCK TABLE that explicitly requests a lock other than ACCESS SHARE MODE.
+   LOCK TABLE that explicitly requests a mode higher than ROW EXCLUSIVE MODE.
   
  
 	 
@@ -2110,10 +2110,10 @@ if (!triggered)
 

 	In recovery, transactions will not be permitted to take any table lock
-	higher than AccessShareLock. In addition, transactions may never assign
+	higher than RowExclusiveLock. In addition, transactions may never assign
 	a TransactionId and may never write WAL.
 	Any LOCK TABLE command that runs on the standby and requests a specific
-	lock type other than AccessShareLock will be rejected.
+	lock mode higher than ROW EXCLUSIVE MODE will be rejected.

 

@@ -2207,15 +2207,16 @@ if (!triggered)
 

 	We have a number of choices for resolving query conflicts.  The default
-	is that we wait and hope the query completes. The server will wait automatically until the lag between
-	primary and standby is at most max_standby_delay seconds. Once that grace
-	period expires, we take one of the following actions:
+	is that we wait and hope the query completes. The server will wait
+	automatically until the lag between primary and standby is at most 
+	max_standby_delay seconds. Once that grace period expires, we take one
+	of the following actions:
 
 	  
 	   
 	
-		 If the conflict is caused by a lock, we cancel the standby transaction
-		 immediately, even if it is idle-in-transaction.
+		 If the conflict is caused by a lock, we cancel the conflicting standby
+		 transaction immediately, even if it is idle-in-transaction.
 	
 	   
 	   
@@ -2224,7 +2225,9 @@ if (!triggered)
 		 that a conflict has occurred and that it must cancel itself to avoid the
 		 risk that it silently fails to read relevant data because
 		 that data has been removed. (This is very similar to the much feared
-		 error message "snapshot too old").
+		 error message "snapshot too old"). Some cleanup records only cause
+		 conflict with older queries, though some types of cleanup record
+		 affect all queries.
 	
 
 	
@@ -2364,6 +2367,9 @@ LOG:  database system is ready to accept read only connections

 	As a result, you cannot create additional indexes that exist solely
 	on the standby, nor can statistics that exist solely on the standby.
+	If these administrator commands are needed they should be executed
+	on the primary so that the changes will propagate through to the
+	standby.

 

@@ -2373,7 +2379,8 @@ LOG:  database system is ready to accept read only connections
 	managed by the Startup process that owns all AccessExclusiveLocks held
 	by transactions being replayed by recovery. pg_stat_activity does not
 	show an entry for the Startup process, nor do recovering transactions
-	show as active.
+	show as active. Note that Startup process does not acquire locks to
+	make database changes and thus no locks are shown in pg_locks.

 

diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 364756d..05e1c5e 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -659,7 +659,10 @@ _bt_page_recyclable(Page page)
  * order when replaying the effects of a VACUUM, just as we do for the
  * original VACUUM itself. lastBlockVacuumed allows us to tell whether an
  * intermediate range of blocks has had no changes at all by VACUUM,
- * and so must be scanned anyway during replay.
+ * and so must be scanned anyway during replay. We always write a WAL record
+ * for the last block in the index, whether or not it contained any items
+ * to be removed. This allows us to scan right up to end of index 

Re: [HACKERS] Hot Standby and cancelling idle queries

[Simon Riggs]

On Thu, 2009-11-26 at 08:30 +0200, Heikki Linnakangas wrote:
> I suspect you missed the context of this change. It's about the code
> in tablespc.c, to kill all backends that might have a temporary file
> in a tablespace that's being dropped. It's not about tuple visibility
> but temporary files.

Got you now.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot Standby and cancelling idle queries

[Heikki Linnakangas]

Simon Riggs wrote:
> Recent change:
> 
> An idle-in-transaction transaction can also hold a temporary file. Think
> of an open cursor, for example. Therefore, remove the distinction
> between CONFLICT_MODE_ERROR and CONFLICT_MODE_ERROR_IF_NOT_IDLE,
> idle-in-transaction backends need to be killed too when a tablespace is
> dropped.
> 
> Open cursors still have snapshots, so they would not be treated as idle
> in transaction.

A backend is idle-in-transaction whenever a transaction is open and the
backend is waiting for a command from the client. Whether it has active
snapshots or open cursors doesn't affect that.

> If the user has a held cursor then they can keep it,
> since it has already read the database and released the snapshot.

A held cursor can keep a temp file open.

I suspect you missed the context of this change. It's about the code in
tablespc.c, to kill all backends that might have a temporary file in a
tablespace that's being dropped. It's not about tuple visibility but
temporary files.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot Standby and cancelling idle queries

[Tom Lane]

Simon Riggs  writes:
> An idle-in-transaction transaction can also hold a temporary file. Think
> of an open cursor, for example. Therefore, remove the distinction
> between CONFLICT_MODE_ERROR and CONFLICT_MODE_ERROR_IF_NOT_IDLE,
> idle-in-transaction backends need to be killed too when a tablespace is
> dropped.

Um ... I think you have forgotten about WITH HOLD cursors, which will
also have temp files.  Implication: whatever you are thinking of here
would require killing EVERY session.  Conclusion: you need a different
design.

regards, tom lane

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

Re: [HACKERS] Hot Standby remaining issues

[Simon Riggs]

On Wed, 2009-11-25 at 13:00 +0200, Heikki Linnakangas wrote:
> I've put up a wiki page with the issues I see with the patch as it
> stands. They're roughly categorized by seriousness.
> 
> http://wiki.postgresql.org/wiki/Hot_Standby_TODO
> 
> New issues can and probably will still pop up, let's add them to the
> list as they're found so that we know what still needs to be done.
> 
> You had a list of work items at the hot standby main page, but I believe
> it's badly out-of-date. Please move any still relevant items to the
> above list, if any.

I've linked the two pages together and identified the ones I'm currently
working on, plus added a few comments.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Greg Stark]

On Wed, Nov 25, 2009 at 3:26 AM, Tom Lane  wrote:
> Greg Stark  writes:
>> Well the only thing that's been discussed is having vacuum require a
>> minimum age before considering a transaction visible to all to reduce
>> the chance of conflicts on cleanup records.
>
> [ shrug... ]  Call me Cassandra.  I am not concerned about what has or
> has not been discussed.  I am concerned about what effects we are going
> to be blindsided by, a few months from now when it is too late to
> conveniently add a way to detect that the system is being run as an HS
> master.  If we design it in, perhaps we won't need it --- but if we
> design it out, we will need it.  You have heard of Finagle's law, no?

Well the point here was that the only inkling of a possible need for
this that we have is going to require more than an on/off switch
anyways. That's likely to be true of any need which arises.

And you didn't answer my questions about the semantics of this switch
will be. That a replica which starts up while reading wal logs
generated by this database will refuse connections even if it's
configured to allow them? How will it determine what the switch was on
the master? The value of the switch at what point in time? The answers
to these questions seem to depend on what the need which triggered the
existence of the switch was.

-- 
greg

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Wed, 2009-11-25 at 03:12 +, Greg Stark wrote:
> On Wed, Nov 25, 2009 at 2:10 AM, Tom Lane  wrote:
> > As long as there's not anything the master actually does differently
> > then I can't see where there'd be any performance testing to do.  What's
> > bothering me about this is that it seems likely that we'll find places
> > where the master has to do things differently.  I'd rather we made the
> > status visible; if we get through a release cycle without needing to
> > check it, we can always take the function out again.  But if we don't,
> > and then find out midway through the 8.5 release cycle that we need to
> > be able to check it, things could be a bit sticky.
> 
> Well the only thing that's been discussed is having vacuum require a
> minimum age before considering a transaction visible to all to reduce
> the chance of conflicts on cleanup records. But that would require an
> actual tunable, not just a flag. And it's something that could
> conceivably be desirable even if you're not running a HS setup (if
> someone ever reimplements time travel for example).

I will add this also, if it looks simple to do so. Even if we yank it
out later better to have the code for discussion purposes than just a
conceptual bikeshed.

> So I'm not sure adding a flag before there's an actual need for it is
> necessarily going to be helpful. It may turn out to be insufficient
> even if we have a flag.

Same situation as in archiving.

The debate was eventually carried that we should have
archive_mode = on
archive_ =  for additional parameters

> And then there's the question of what the slave should do if the
> master was running without the flag. Do we make it throw an error?

Well, it can't even enter HS mode, so no error needed.

> Does that mean the master needs to insert information to that effect
> in the wal logs? What if you shut down the master switch the flag and
> start it up again and you had a standby reading those logs all along.
> Will it be able to switch to HS mode now? We won't know until we know
> why this flag was necessary and what change in behaviour it might have
> caused.

I'm more comfortable running a new machine when it has an "off" switch.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Tom Lane]

Greg Stark  writes:
> Well the only thing that's been discussed is having vacuum require a
> minimum age before considering a transaction visible to all to reduce
> the chance of conflicts on cleanup records.

[ shrug... ]  Call me Cassandra.  I am not concerned about what has or
has not been discussed.  I am concerned about what effects we are going
to be blindsided by, a few months from now when it is too late to
conveniently add a way to detect that the system is being run as an HS
master.  If we design it in, perhaps we won't need it --- but if we
design it out, we will need it.  You have heard of Finagle's law, no?

regards, tom lane

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Greg Stark]

On Wed, Nov 25, 2009 at 2:10 AM, Tom Lane  wrote:
> As long as there's not anything the master actually does differently
> then I can't see where there'd be any performance testing to do.  What's
> bothering me about this is that it seems likely that we'll find places
> where the master has to do things differently.  I'd rather we made the
> status visible; if we get through a release cycle without needing to
> check it, we can always take the function out again.  But if we don't,
> and then find out midway through the 8.5 release cycle that we need to
> be able to check it, things could be a bit sticky.

Well the only thing that's been discussed is having vacuum require a
minimum age before considering a transaction visible to all to reduce
the chance of conflicts on cleanup records. But that would require an
actual tunable, not just a flag. And it's something that could
conceivably be desirable even if you're not running a HS setup (if
someone ever reimplements time travel for example).

So I'm not sure adding a flag before there's an actual need for it is
necessarily going to be helpful. It may turn out to be insufficient
even if we have a flag.

And then there's the question of what the slave should do if the
master was running without the flag. Do we make it throw an error?
Does that mean the master needs to insert information to that effect
in the wal logs? What if you shut down the master switch the flag and
start it up again and you had a standby reading those logs all along.
Will it be able to switch to HS mode now? We won't know until we know
why this flag was necessary and what change in behaviour it might have
caused.



-- 
greg

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Tom Lane]

Simon Riggs  writes:
> Tom Lane wrote:
>> There's no equivalent of XLogArchivingActive()?

> We've tried hard to have it "just work". But I wonder whether we should
> have a parameter to allow performance testing on the master? If nobody
> finds any issues then we can remove it again, or at least make it a
> hidden developer option.

As long as there's not anything the master actually does differently
then I can't see where there'd be any performance testing to do.  What's
bothering me about this is that it seems likely that we'll find places
where the master has to do things differently.  I'd rather we made the
status visible; if we get through a release cycle without needing to
check it, we can always take the function out again.  But if we don't,
and then find out midway through the 8.5 release cycle that we need to
be able to check it, things could be a bit sticky.

regards, tom lane

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Sat, 2009-11-21 at 23:00 +0200, Heikki Linnakangas wrote:
> Tom Lane wrote:
> > Heikki Linnakangas  writes:
> >> Tom Lane wrote:
> >>> There's no equivalent of XLogArchivingActive()?
> > 
> >> XLogArchivingMode() == false enables us to skip WAL-logging in
> >> operations like CLUSTER or COPY, which is a big optimization. I don't
> >> see anything like that in Hot Standby. There is a few small things that
> >> could be skipped, but nothing noticeable.
> > 
> > Huh?  Surely HS requires XLogArchivingMode as a prerequisite ...
> 
> Oh, sure! But there's no switch that needs to be enabled in the master
> in addition to that.

We've tried hard to have it "just work". But I wonder whether we should
have a parameter to allow performance testing on the master? If nobody
finds any issues then we can remove it again, or at least make it a
hidden developer option.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Simon Riggs]

On Sat, 2009-11-21 at 20:20 +0200, Heikki Linnakangas wrote:

> That causes some headaches for Hot Standby

I say leave HS as it is and we can clean up when we do the VFectomy.

It isn't really a headache, the code works easily enough. I agree its
ugly and it should eventually be removed.

Let's not make this any harder, or get involved with promises that we
may not be able to keep. I'd rather we had HS + SR than HS - VF for
example. VF is ugly but it isn't a priority.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Hannu Krosing]

On Tue, 2009-11-24 at 09:24 +0200, Heikki Linnakangas wrote:
> Greg Smith wrote:
> > Heikki Linnakangas wrote:
> >> So I guess what I'm asking is: Does anyone see any show-stoppers in
> >> removing VACUUM FULL
> > Here's the disclaimers attached to the new VACUUM REPLACE implementation
> > from Itagaki:
> > 
> > "We still need traditional VACUUM FULL behavior for system catalog
> > because we cannot change relfilenode for them. Also, VACUUM FULL REPLACE
> > is not always better than traditional VACUUM FULL; the new version
> > requires additional disk space and might be slower if we have a few dead
> > tuples."
> > 
> > That first part seems like it would limit the ability to completely
> > discard the current behavior.
> 
> For system catalog,s you could still use a utility like the one I
> experimented with at
> http://archives.postgresql.org/message-id/4ab15065.1000...@enterprisedb.com.
> Essentially, do a bunch of dummy UPDATEs on the rows that you want to
> move. It can cause serialization errors in concurrent updaters, like any
> UPDATE, but I think it would be good enough for the narrow remaining use
> case of shrinking system catalogs.

call it VACUUM SHRINK ;)

and you will need to do a REINDEX after using it to get full benefit,
same as ordinary VACUUM or VACUUM FULL.

> -- 
>   Heikki Linnakangas
>   EnterpriseDB   http://www.enterprisedb.com
> 



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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Heikki Linnakangas]

Greg Smith wrote:
> Heikki Linnakangas wrote:
>> So I guess what I'm asking is: Does anyone see any show-stoppers in
>> removing VACUUM FULL
> Here's the disclaimers attached to the new VACUUM REPLACE implementation
> from Itagaki:
> 
> "We still need traditional VACUUM FULL behavior for system catalog
> because we cannot change relfilenode for them. Also, VACUUM FULL REPLACE
> is not always better than traditional VACUUM FULL; the new version
> requires additional disk space and might be slower if we have a few dead
> tuples."
> 
> That first part seems like it would limit the ability to completely
> discard the current behavior.

For system catalog,s you could still use a utility like the one I
experimented with at
http://archives.postgresql.org/message-id/4ab15065.1000...@enterprisedb.com.
Essentially, do a bunch of dummy UPDATEs on the rows that you want to
move. It can cause serialization errors in concurrent updaters, like any
UPDATE, but I think it would be good enough for the narrow remaining use
case of shrinking system catalogs.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Heikki Linnakangas]

Itagaki Takahiro wrote:
> VACUUM FULL is still reserved for system catalogs in my patch
> because we cannot modify relfilenodes for the catalog tables.
> Do you have solutions for it?

Tom had an idea on that:

http://archives.postgresql.org/message-id/19750.1252094...@sss.pgh.pa.us

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Itagaki Takahiro]

Heikki Linnakangas  wrote:

> So I guess what I'm asking is: Does anyone see any show-stoppers in
> removing VACUUM FULL, and does anyone want to step up to the plate and
> promise to do it before release?

I'm working on "New VACUUM FULL" patch, that shrinks tables
using CLUSTER-like rewrites.
https://commitfest.postgresql.org/action/patch_view?id=202

What can I do for the Hot Standby issue?
VACUUM FULL is still reserved for system catalogs in my patch
because we cannot modify relfilenodes for the catalog tables.
Do you have solutions for it?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center



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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Heikki Linnakangas]

Tom Lane wrote:
> Heikki Linnakangas  writes:
>> Tom Lane wrote:
>>> There's no equivalent of XLogArchivingActive()?
> 
>> XLogArchivingMode() == false enables us to skip WAL-logging in
>> operations like CLUSTER or COPY, which is a big optimization. I don't
>> see anything like that in Hot Standby. There is a few small things that
>> could be skipped, but nothing noticeable.
> 
> Huh?  Surely HS requires XLogArchivingMode as a prerequisite ...

Oh, sure! But there's no switch that needs to be enabled in the master
in addition to that.

> (Do we have a list of open issues somewhere, so that this
> won't get forgotten?)

I'm keeping one as I bump into things. I'll post it again soon.
Actually, I should probably put it on the wiki.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Tom Lane]

Heikki Linnakangas  writes:
> Tom Lane wrote:
>> There's no equivalent of XLogArchivingActive()?

> XLogArchivingMode() == false enables us to skip WAL-logging in
> operations like CLUSTER or COPY, which is a big optimization. I don't
> see anything like that in Hot Standby. There is a few small things that
> could be skipped, but nothing noticeable.

Huh?  Surely HS requires XLogArchivingMode as a prerequisite ...

> It's great from usability point of view that you don't need to enable it
> beforehand,

... which also means that I don't understand this statement.

> Anyway, I think I have enough courage now to just rip out the VACUUM
> FULL support from HS. If VACUUM FULL is still there when we're ready to
> go to beta, we can introduce a "do you want VACUUM FULL or hot standby?"
> switch in the master, or some other ugly workaround.

Agreed, we have more than enough worries without worrying about making
that work.  (Do we have a list of open issues somewhere, so that this
won't get forgotten?)

regards, tom lane

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

Re: [HACKERS] Hot standby and removing VACUUM FULL

[Heikki Linnakangas]

Tom Lane wrote:
> Heikki Linnakangas  writes:
>> Tom Lane wrote:
>>> I don't see much problem with rejecting VAC FULL in a HS master,
>>> whether or not it gets removed altogether.  Why not just do that
>>> rather than write a lot of kluges?
> 
>> Hmm. At the moment, no action is required in the master to allow hot
>> standby in the slave, except for turning on archiving. The additional
>> overhead of the extra logging that's needed in the master is small
>> enough that there has been no need for a switch.
> 
> There's no equivalent of XLogArchivingActive()?  I think there probably
> should be.  I find it really hard to believe that there won't be any
> places where we need to know that we're an HS master.  The original
> design of WAL archiving didn't think we needed to know we were archiving
> WAL, either, and look how many cases there are for that now.

XLogArchivingMode() == false enables us to skip WAL-logging in
operations like CLUSTER or COPY, which is a big optimization. I don't
see anything like that in Hot Standby. There is a few small things that
could be skipped, but nothing noticeable.

It's great from usability point of view that you don't need to enable it
beforehand, especially because HS is also very useful when doing PITR
from a backup; it's too late to turn on the switch at that point. As
soon as we have a good reason to introduce a switch, let's do so, but
not before that.

If we want cop out of VACUUM FULL and HS issues, maybe we could just
kick out all clients from the standby when we see a WAL record from
VACUUM FULL. Not very elegant, but should work.

Anyway, I think I have enough courage now to just rip out the VACUUM
FULL support from HS. If VACUUM FULL is still there when we're ready to
go to beta, we can introduce a "do you want VACUUM FULL or hot standby?"
switch in the master, or some other ugly workaround.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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