Re: [HACKERS] Hot Standby, release candidate?
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 http://wiki.postgresql.org/wiki/PostgreSQL_8.4_Development_Plan, 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 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?
On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
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 patch against a CVS checkout before submitting. Can you explain further? Last remaining points
Re: [HACKERS] Hot Standby, release candidate?
On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
On Mon, Dec 14, 2009 at 8:07 PM, Simon Riggs si...@2ndquadrant.com 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?
On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
On Mon, Dec 14, 2009 at 12:51, Robert Haas robertmh...@gmail.com wrote: On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
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?
On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs si...@2ndquadrant.com 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?
On Mon, 2009-12-14 at 06:51 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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?
On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs si...@2ndquadrant.com 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?
On Mon, Dec 14, 2009 at 13:47, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote: On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs si...@2ndquadrant.com 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?
On Mon, Dec 14, 2009 at 7:42 AM, Simon Riggs si...@2ndquadrant.com 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?
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?
Simon Riggs si...@2ndquadrant.com 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?
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?
Robert Haas robertmh...@gmail.com writes: On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs si...@2ndquadrant.com 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?
On Mon, Dec 14, 2009 at 10:08 AM, Simon Riggs si...@2ndquadrant.com 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 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?
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 heikki.linnakan...@enterprisedb.com 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?
On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote: Simon Riggs si...@2ndquadrant.com 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?
On Mon, Dec 14, 2009 at 11:09:49AM +0100, Magnus Hagander wrote: On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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 da...@fetter.org 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?
On Mon, Dec 14, 2009 at 10:49 AM, Simon Riggs si...@2ndquadrant.com wrote: On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote: Simon Riggs si...@2ndquadrant.com 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?
Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs si...@2ndquadrant.com 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 br...@momjian.ushttp://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?
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?
On Mon, Dec 14, 2009 at 11:07 AM, Simon Riggs si...@2ndquadrant.com 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 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?
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?
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?
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 si...@2ndquadrant.com 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?
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?
On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com 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 si...@2ndquadrant.com writes: On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com 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?
On Mon, 2009-12-14 at 14:14 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com writes: On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com 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?
Simon Riggs si...@2ndquadrant.com 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?
On Mon, 2009-12-14 at 15:24 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com 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?
Simon Riggs si...@2ndquadrant.com 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?
On Mon, 2009-12-14 at 16:39 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com 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?
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 wrote: 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. At this point we've got 5 small to medium sized patches in the Ready for Committer queue. Maybe that gets all done on Tuesday, maybe it slips to Wednesday or later. It's not like a bell goes off tomorrow and we're done; there's probably going to be just a little slip here. In any case, it's certainly not the case that this is all done right now such that the alpha gets packed on Tuesday just because it's the 15th. It sounds like the worst case is that alpha would have to wait a day for Hot Standby to be finally committed, which seems well worth doing if it means HS gets that much more testing on it. It would be a help to eliminate the merge conflict issues for the Streaming Replication team by giving them only one code base to worry about merges against. And on the PR side, announcing HS as hitting core and available in the alpha is huge. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com
Re: [HACKERS] Hot Standby, release candidate?
Simon Riggs si...@2ndquadrant.com 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, release candidate?
On Sun, 2009-12-13 at 15:45 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com 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, recent changes
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
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, misc issues
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, recent changes
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, recent changes
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
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
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
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 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
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
On Sun, Dec 6, 2009 at 3:06 PM, Simon Riggs si...@2ndquadrant.com 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
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
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, misc issues
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, misc issues
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 remaining issues
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 remaining issues
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
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 and removing VACUUM FULL
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 and removing VACUUM FULL
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
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
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 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
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 remaining issues
Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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
On Wed, 2009-11-25 at 03:12 +, Greg Stark wrote: On Wed, Nov 25, 2009 at 2:10 AM, Tom Lane t...@sss.pgh.pa.us 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 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
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
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
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 sri...@ebony.(none) 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 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 wrote: commit 02c3eadb766201db084b668daa271db4a900adc9 Author: Simon Riggs sri...@ebony.(none) 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
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 and cancelling idle queries
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 remaining issues
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) /listitem listitem para - 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. /para /listitem listitem @@ -2033,7 +2033,7 @@ if (!triggered) listitem para 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. /para /listitem listitem @@ -2110,10 +2110,10 @@ if (!triggered) para 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. /para para @@ -2207,15 +2207,16 @@ if (!triggered) para 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: itemizedlist listitem para - 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. /para /listitem listitem @@ -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. /para para @@ -2364,6 +2367,9 @@ LOG: database system is ready to accept read only connections para 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. /para para @@ -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. /para para 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
Re: [HACKERS] Hot Standby remaining issues
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 cancelling idle queries
Simon Riggs si...@2ndquadrant.com 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 and cancelling idle queries
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 removing VACUUM FULL
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
On Sat, 2009-11-21 at 23:00 +0200, Heikki Linnakangas wrote: Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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 si...@2ndquadrant.com 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
On Wed, Nov 25, 2009 at 2:10 AM, Tom Lane t...@sss.pgh.pa.us 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
Greg Stark gsst...@mit.edu 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
On Wed, 2009-11-25 at 03:12 +, Greg Stark wrote: On Wed, Nov 25, 2009 at 2:10 AM, Tom Lane t...@sss.pgh.pa.us 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
On Wed, Nov 25, 2009 at 3:26 AM, Tom Lane t...@sss.pgh.pa.us wrote: Greg Stark gsst...@mit.edu 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
Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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
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
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
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 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. -- 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 and removing VACUUM FULL
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: 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 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? 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
Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: 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 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. -- 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 heikki.linnakan...@enterprisedb.com 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. 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
Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com 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
